From bac95e6e1e99b1e364c5ebd39887a8e24bc1ad9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 18 Mar 2014 19:41:03 +0100 Subject: [PATCH 1/5] reflog: more comprehensive HEAD tests The existing ones lack checking zeroed ids when switching back from an unborn branch as well as what happens when detaching. The reflog appending function mistakenly wrote zeros when dealing with a detached HEAD. This explicitly checks for those situations and fixes them. --- src/refdb_fs.c | 8 +++--- tests/repo/head.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 4 deletions(-) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index f494afa71..0c3011083 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -1555,7 +1555,7 @@ success: /* Append to the reflog, must be called under reference lock */ static int reflog_append(refdb_fs_backend *backend, const git_reference *ref, const git_oid *old, const git_oid *new, const git_signature *who, const char *message) { - int error, is_symbolic, was_symbolic = 0; + int error, is_symbolic, currently_exists; git_oid old_id = {{0}}, new_id = {{0}}; git_buf buf = GIT_BUF_INIT, path = GIT_BUF_INIT; git_repository *repo = backend->repo; @@ -1573,14 +1573,14 @@ static int reflog_append(refdb_fs_backend *backend, const git_reference *ref, co if (error < 0 && error != GIT_ENOTFOUND) return error; - if (current_ref) - was_symbolic = current_ref->type == GIT_REF_SYMBOLIC; + currently_exists = !!current_ref; + git_reference_free(current_ref); /* From here on is_symoblic also means that it's HEAD */ if (old) { git_oid_cpy(&old_id, old); - } else if (!was_symbolic) { + } else if (currently_exists) { error = git_reference_name_to_id(&old_id, repo, ref->name); if (error == GIT_ENOTFOUND) { memset(&old_id, 0, sizeof(git_oid)); diff --git a/tests/repo/head.c b/tests/repo/head.c index d88fd90d1..a246e6086 100644 --- a/tests/repo/head.c +++ b/tests/repo/head.c @@ -269,3 +269,75 @@ void test_repo_head__setting_head_updates_reflog(void) git_object_free(tag); git_signature_free(sig); } + +static void assert_head_reflog(git_repository *repo, size_t idx, + const char *old_id, const char *new_id, const char *message) +{ + git_reflog *log; + const git_reflog_entry *entry; + char id_str[GIT_OID_HEXSZ + 1] = {0}; + + cl_git_pass(git_reflog_read(&log, repo, GIT_HEAD_FILE)); + entry = git_reflog_entry_byindex(log, idx); + + git_oid_fmt(id_str, git_reflog_entry_id_old(entry)); + cl_assert_equal_s(old_id, id_str); + + git_oid_fmt(id_str, git_reflog_entry_id_new(entry)); + cl_assert_equal_s(new_id, id_str); + + cl_assert_equal_s(message, git_reflog_entry_message(entry)); + + git_reflog_free(log); +} + +void test_repo_head__detaching_writes_reflog(void) +{ + git_signature *sig; + git_oid id; + const char *msg; + + cl_git_pass(git_signature_now(&sig, "me", "foo@example.com")); + + msg = "message1"; + git_oid_fromstr(&id, "e90810b8df3e80c413d903f631643c716887138d"); + cl_git_pass(git_repository_set_head_detached(repo, &id, sig, msg)); + assert_head_reflog(repo, 0, "a65fedf39aefe402d3bb6e24df4d4f5fe4547750", + "e90810b8df3e80c413d903f631643c716887138d", msg); + + msg = "message2"; + cl_git_pass(git_repository_set_head(repo, "refs/heads/haacked", sig, msg)); + assert_head_reflog(repo, 0, "e90810b8df3e80c413d903f631643c716887138d", + "258f0e2a959a364e40ed6603d5d44fbb24765b10", msg); + + git_signature_free(sig); +} + +void test_repo_head__orphan_branch_does_not_count(void) +{ + git_signature *sig; + git_oid id; + const char *msg; + + cl_git_pass(git_signature_now(&sig, "me", "foo@example.com")); + + /* Have something known */ + msg = "message1"; + git_oid_fromstr(&id, "e90810b8df3e80c413d903f631643c716887138d"); + cl_git_pass(git_repository_set_head_detached(repo, &id, sig, msg)); + assert_head_reflog(repo, 0, "a65fedf39aefe402d3bb6e24df4d4f5fe4547750", + "e90810b8df3e80c413d903f631643c716887138d", msg); + + /* Switching to an orphan branch does not write tot he reflog */ + cl_git_pass(git_repository_set_head(repo, "refs/heads/orphan", sig, "ignored message")); + assert_head_reflog(repo, 0, "a65fedf39aefe402d3bb6e24df4d4f5fe4547750", + "e90810b8df3e80c413d903f631643c716887138d", msg); + + /* And coming back, we set the source to zero */ + msg = "message2"; + cl_git_pass(git_repository_set_head(repo, "refs/heads/haacked", sig, msg)); + assert_head_reflog(repo, 0, "0000000000000000000000000000000000000000", + "258f0e2a959a364e40ed6603d5d44fbb24765b10", msg); + + git_signature_free(sig); +} From 1afe1400433f010734ae4c43bf35dcc94edcc9de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 18 Mar 2014 22:16:58 +0100 Subject: [PATCH 2/5] refdb: don't update when there's no need If the caller wants to update a ref to point to the same target as it currently has, we should return early and avoid writing to the reflog. --- src/refdb_fs.c | 17 +++++++++++++++++ tests/refs/reflog/reflog.c | 24 ++++++++++++++++++++++++ tests/repo/head.c | 27 +++++++++++++++++++++++++++ 3 files changed, 68 insertions(+) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index 0c3011083..4bcc5fac3 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -1021,6 +1021,8 @@ static int refdb_fs_backend__write( refdb_fs_backend *backend = (refdb_fs_backend *)_backend; git_filebuf file = GIT_FILEBUF_INIT; int error = 0, cmp = 0; + const char *new_target = NULL; + const git_oid *new_id = NULL; assert(backend); @@ -1041,6 +1043,21 @@ static int refdb_fs_backend__write( goto on_error; } + if (ref->type == GIT_REF_SYMBOLIC) + new_target = ref->target.symbolic; + else + new_id = &ref->target.oid; + + error = cmp_old_ref(&cmp, _backend, ref->name, new_id, new_target); + if (error < 0 && error != GIT_ENOTFOUND) + goto on_error; + + /* Don't update if we have the same value */ + if (!error && !cmp) { + error = 0; + goto on_error; /* not really error */ + } + if (should_write_reflog(backend->repo, ref->name)) { if ((error = reflog_append(backend, ref, NULL, NULL, who, message)) < 0) goto on_error; diff --git a/tests/refs/reflog/reflog.c b/tests/refs/reflog/reflog.c index 149e98273..a50d40aac 100644 --- a/tests/refs/reflog/reflog.c +++ b/tests/refs/reflog/reflog.c @@ -237,3 +237,27 @@ void test_refs_reflog_reflog__append_to_HEAD_when_changing_current_branch(void) cl_assert_equal_i(nlogs_after, nlogs + 1); } + +void test_refs_reflog_reflog__do_not_append_when_no_update(void) +{ + size_t nlogs, nlogs_after; + git_reference *ref, *ref2; + git_reflog *log; + + cl_git_pass(git_reflog_read(&log, g_repo, "HEAD")); + nlogs = git_reflog_entrycount(log); + git_reflog_free(log); + + cl_git_pass(git_reference_lookup(&ref, g_repo, "refs/heads/master")); + cl_git_pass(git_reference_create(&ref2, g_repo, "refs/heads/master", + git_reference_target(ref), 1, NULL, NULL)); + + git_reference_free(ref); + git_reference_free(ref2); + + cl_git_pass(git_reflog_read(&log, g_repo, "HEAD")); + nlogs_after = git_reflog_entrycount(log); + git_reflog_free(log); + + cl_assert_equal_i(nlogs_after, nlogs); +} diff --git a/tests/repo/head.c b/tests/repo/head.c index a246e6086..130ed8588 100644 --- a/tests/repo/head.c +++ b/tests/repo/head.c @@ -341,3 +341,30 @@ void test_repo_head__orphan_branch_does_not_count(void) git_signature_free(sig); } + +void test_repo_head__set_to_current_target(void) +{ + git_signature *sig; + const char *msg; + git_reflog *log; + size_t nentries, nentries_after; + + cl_git_pass(git_reflog_read(&log, repo, GIT_HEAD_FILE)); + nentries = git_reflog_entrycount(log); + git_reflog_free(log); + + cl_git_pass(git_signature_now(&sig, "me", "foo@example.com")); + + msg = "message 1"; + cl_git_pass(git_repository_set_head(repo, "refs/heads/haacked", sig, msg)); + cl_git_pass(git_repository_set_head(repo, "refs/heads/haacked", sig, msg)); + + cl_git_pass(git_reflog_read(&log, repo, GIT_HEAD_FILE)); + nentries_after = git_reflog_entrycount(log); + git_reflog_free(log); + + cl_assert_equal_i(nentries + 1, nentries_after); + + git_signature_free(sig); + +} From afc57eb48fc69d3e4808648c090aa6f91f9b29aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 19 Mar 2014 06:59:09 +0100 Subject: [PATCH 3/5] reflog: simplify the append logic Remove some duplicated logic. --- src/refdb_fs.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index 4bcc5fac3..f2edf0c56 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -1572,11 +1572,10 @@ success: /* Append to the reflog, must be called under reference lock */ static int reflog_append(refdb_fs_backend *backend, const git_reference *ref, const git_oid *old, const git_oid *new, const git_signature *who, const char *message) { - int error, is_symbolic, currently_exists; + int error, is_symbolic; git_oid old_id = {{0}}, new_id = {{0}}; git_buf buf = GIT_BUF_INIT, path = GIT_BUF_INIT; git_repository *repo = backend->repo; - git_reference *current_ref = NULL; is_symbolic = ref->type == GIT_REF_SYMBOLIC; @@ -1586,37 +1585,23 @@ static int reflog_append(refdb_fs_backend *backend, const git_reference *ref, co !(old && new)) return 0; - error = git_reference_lookup(¤t_ref, repo, ref->name); - if (error < 0 && error != GIT_ENOTFOUND) - return error; - - currently_exists = !!current_ref; - - git_reference_free(current_ref); /* From here on is_symoblic also means that it's HEAD */ if (old) { git_oid_cpy(&old_id, old); - } else if (currently_exists) { + } else { error = git_reference_name_to_id(&old_id, repo, ref->name); - if (error == GIT_ENOTFOUND) { - memset(&old_id, 0, sizeof(git_oid)); - error = 0; - } - - if (error < 0) + if (error < 0 && error != GIT_ENOTFOUND) return error; } if (is_symbolic) { error = git_reference_name_to_id(&new_id, repo, git_reference_symbolic_target(ref)); - if (error != 0 && error != GIT_ENOTFOUND) - goto cleanup; + if (error < 0 && error != GIT_ENOTFOUND) + return error; /* detaching HEAD does not create an entry */ - if (error == GIT_ENOTFOUND) { - error = 0; - goto cleanup; - } + if (error == GIT_ENOTFOUND) + return 0; giterr_clear(); } From 6aaae94a7060834a68f3fd24d8006dbfe769e04a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 19 Mar 2014 16:30:37 +0100 Subject: [PATCH 4/5] reflog: handle the birth of a branch The reflog append function was overzealous in its checking. When passed an old and new ids, it should not do any checking, but just serialize the data to a reflog entry. --- src/refdb_fs.c | 44 ++++++++++++++++++++++++------------------- tests/repo/head.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 19 deletions(-) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index f2edf0c56..905113226 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -992,18 +992,24 @@ out: static int maybe_append_head(refdb_fs_backend *backend, const git_reference *ref, const git_signature *who, const char *message) { int error; - git_oid old_id; + git_oid old_id = {{0}}; git_reference *head; - error = git_reference_name_to_id(&old_id, backend->repo, ref->name); - if (!git_branch_is_head(ref)) - return 0; + /* if we can't resolve, we use {0}*40 as old id */ + git_reference_name_to_id(&old_id, backend->repo, ref->name); if ((error = git_reference_lookup(&head, backend->repo, "HEAD")) < 0) return error; + if (git_reference_type(head) == GIT_REF_OID) + goto cleanup; + + if (strcmp(git_reference_symbolic_target(head), ref->name)) + goto cleanup; + error = reflog_append(backend, head, &old_id, git_reference_target(ref), who, message); +cleanup: git_reference_free(head); return error; } @@ -1595,22 +1601,22 @@ static int reflog_append(refdb_fs_backend *backend, const git_reference *ref, co return error; } - if (is_symbolic) { - error = git_reference_name_to_id(&new_id, repo, git_reference_symbolic_target(ref)); - if (error < 0 && error != GIT_ENOTFOUND) - return error; - /* detaching HEAD does not create an entry */ - if (error == GIT_ENOTFOUND) - return 0; - - giterr_clear(); - } - - - if (new) + if (new) { git_oid_cpy(&new_id, new); - else if (!is_symbolic) - git_oid_cpy(&new_id, git_reference_target(ref)); + } else { + if (!is_symbolic) { + git_oid_cpy(&new_id, git_reference_target(ref)); + } else { + error = git_reference_name_to_id(&new_id, repo, git_reference_symbolic_target(ref)); + if (error < 0 && error != GIT_ENOTFOUND) + return error; + /* detaching HEAD does not create an entry */ + if (error == GIT_ENOTFOUND) + return 0; + + giterr_clear(); + } + } if ((error = serialize_reflog_entry(&buf, &old_id, &new_id, who, message)) < 0) goto cleanup; diff --git a/tests/repo/head.c b/tests/repo/head.c index 130ed8588..d68a5a61e 100644 --- a/tests/repo/head.c +++ b/tests/repo/head.c @@ -368,3 +368,51 @@ void test_repo_head__set_to_current_target(void) git_signature_free(sig); } + +void test_repo_head__branch_birth(void) +{ + git_signature *sig; + git_oid id; + git_tree *tree; + git_reference *ref; + const char *msg; + git_reflog *log; + size_t nentries, nentries_after; + + cl_git_pass(git_reflog_read(&log, repo, GIT_HEAD_FILE)); + nentries = git_reflog_entrycount(log); + git_reflog_free(log); + + cl_git_pass(git_signature_now(&sig, "me", "foo@example.com")); + + cl_git_pass(git_repository_head(&ref, repo)); + cl_git_pass(git_reference_peel((git_object **) &tree, ref, GIT_OBJ_TREE)); + git_reference_free(ref); + + msg = "message 1"; + cl_git_pass(git_repository_set_head(repo, "refs/heads/orphan", sig, msg)); + + cl_git_pass(git_reflog_read(&log, repo, GIT_HEAD_FILE)); + nentries_after = git_reflog_entrycount(log); + git_reflog_free(log); + + cl_assert_equal_i(nentries, nentries_after); + + msg = "message 2"; + cl_git_pass(git_commit_create(&id, repo, "HEAD", sig, sig, NULL, msg, tree, 0, NULL)); + + git_tree_free(tree); + + cl_git_pass(git_reflog_read(&log, repo, "refs/heads/orphan")); + cl_assert_equal_i(1, git_reflog_entrycount(log)); + git_reflog_free(log); + + cl_git_pass(git_reflog_read(&log, repo, GIT_HEAD_FILE)); + nentries_after = git_reflog_entrycount(log); + git_reflog_free(log); + + cl_assert_equal_i(nentries + 1, nentries_after); + + git_signature_free(sig); + +} From 99797c96cd57a616ada009a2359390c605d33929 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 19 Mar 2014 18:14:35 +0100 Subject: [PATCH 5/5] reflog: handle symref chains Given HEAD -> master -> foo, when updating foo's reflog we should also update HEAD's, as it's considered the current branch. --- src/refdb_fs.c | 36 ++++++++++++++++++++++++++++++--- tests/repo/head.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 3 deletions(-) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index 905113226..25316fe46 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -993,23 +993,53 @@ static int maybe_append_head(refdb_fs_backend *backend, const git_reference *ref { int error; git_oid old_id = {{0}}; - git_reference *head; + git_reference *tmp = NULL, *head = NULL, *peeled = NULL; + const char *name; + + if (ref->type == GIT_REF_SYMBOLIC) + return 0; /* if we can't resolve, we use {0}*40 as old id */ git_reference_name_to_id(&old_id, backend->repo, ref->name); - if ((error = git_reference_lookup(&head, backend->repo, "HEAD")) < 0) + if ((error = git_reference_lookup(&head, backend->repo, GIT_HEAD_FILE)) < 0) return error; if (git_reference_type(head) == GIT_REF_OID) goto cleanup; - if (strcmp(git_reference_symbolic_target(head), ref->name)) + if ((error = git_reference_lookup(&tmp, backend->repo, GIT_HEAD_FILE)) < 0) + goto cleanup; + + /* Go down the symref chain until we find the branch */ + while (git_reference_type(tmp) == GIT_REF_SYMBOLIC) { + error = git_reference_lookup(&peeled, backend->repo, git_reference_symbolic_target(tmp)); + if (error < 0) + break; + + git_reference_free(tmp); + tmp = peeled; + } + + if (error == GIT_ENOTFOUND) { + error = 0; + name = git_reference_symbolic_target(tmp); + } else if (error < 0) { + goto cleanup; + } else { + name = git_reference_name(tmp); + } + + if (error < 0) + goto cleanup; + + if (strcmp(name, ref->name)) goto cleanup; error = reflog_append(backend, head, &old_id, git_reference_target(ref), who, message); cleanup: + git_reference_free(tmp); git_reference_free(head); return error; } diff --git a/tests/repo/head.c b/tests/repo/head.c index d68a5a61e..79892a3ea 100644 --- a/tests/repo/head.c +++ b/tests/repo/head.c @@ -416,3 +416,54 @@ void test_repo_head__branch_birth(void) git_signature_free(sig); } + +static size_t entrycount(git_repository *repo, const char *name) +{ + git_reflog *log; + size_t ret; + + cl_git_pass(git_reflog_read(&log, repo, name)); + ret = git_reflog_entrycount(log); + git_reflog_free(log); + + return ret; +} + +void test_repo_head__symref_chain(void) +{ + git_signature *sig; + git_oid id; + git_tree *tree; + git_reference *ref; + const char *msg; + size_t nentries, nentries_master; + + nentries = entrycount(repo, GIT_HEAD_FILE); + + cl_git_pass(git_signature_now(&sig, "me", "foo@example.com")); + + cl_git_pass(git_repository_head(&ref, repo)); + cl_git_pass(git_reference_peel((git_object **) &tree, ref, GIT_OBJ_TREE)); + git_reference_free(ref); + + nentries_master = entrycount(repo, "refs/heads/master"); + + msg = "message 1"; + cl_git_pass(git_reference_symbolic_create(&ref, repo, "refs/heads/master", "refs/heads/foo", 1, sig, msg)); + git_reference_free(ref); + + cl_assert_equal_i(0, entrycount(repo, "refs/heads/foo")); + cl_assert_equal_i(nentries, entrycount(repo, GIT_HEAD_FILE)); + cl_assert_equal_i(nentries_master, entrycount(repo, "refs/heads/master")); + + msg = "message 2"; + cl_git_pass(git_commit_create(&id, repo, "HEAD", sig, sig, NULL, msg, tree, 0, NULL)); + git_tree_free(tree); + + cl_assert_equal_i(1, entrycount(repo, "refs/heads/foo")); + cl_assert_equal_i(nentries +1, entrycount(repo, GIT_HEAD_FILE)); + cl_assert_equal_i(nentries_master, entrycount(repo, "refs/heads/master")); + + git_signature_free(sig); + +}