Merge pull request #2190 from libgit2/cmn/current-branch-reflog

refs: append to the HEAD reflog when updating the current branch

9 files changed, 124 insertions, 56 deletions

diff --git a/include/git2/branch.h b/include/git2/branch.h
index 46aef3206..d2762019b 100644
--- a/include/git2/branch.h
+++ b/include/git2/branch.h
@@ -238,7 +238,7 @@ GIT_EXTERN(int) git_branch_upstream_name(
  * error code otherwise.
  */
 GIT_EXTERN(int) git_branch_is_head(
-	git_reference *branch);
+	const git_reference *branch);
 
 /**
  * Return the name of remote that the remote tracking branch belongs to.
diff --git a/include/git2/refs.h b/include/git2/refs.h
index 65449e69e..1bbb4ca46 100644
--- a/include/git2/refs.h
+++ b/include/git2/refs.h
@@ -586,7 +586,7 @@ GIT_EXTERN(int) git_reference_ensure_log(git_repository *repo, const char *refna
  * @return 1 when the reference lives in the refs/heads
  * namespace; 0 otherwise.
  */
-GIT_EXTERN(int) git_reference_is_branch(git_reference *ref);
+GIT_EXTERN(int) git_reference_is_branch(const git_reference *ref);
 
 /**
  * Check if a reference is a remote tracking branch
diff --git a/src/branch.c b/src/branch.c
index 7c888729d..df665a469 100644
--- a/src/branch.c
+++ b/src/branch.c
@@ -588,7 +588,7 @@ on_error:
 }
 
 int git_branch_is_head(
-		git_reference *branch)
+		const git_reference *branch)
 {
 	git_reference *head;
 	bool is_same = false;
diff --git a/src/refdb_fs.c b/src/refdb_fs.c
index 8a26bec0b..f494afa71 100644
--- a/src/refdb_fs.c
+++ b/src/refdb_fs.c
@@ -21,6 +21,7 @@
 #include <git2/tag.h>
 #include <git2/object.h>
 #include <git2/refdb.h>
+#include <git2/branch.h>
 #include <git2/sys/refdb_backend.h>
 #include <git2/sys/refs.h>
 #include <git2/sys/reflog.h>
@@ -920,7 +921,7 @@ fail:
 	return -1;
 }
 
-static int reflog_append(refdb_fs_backend *backend, const git_reference *ref, const git_signature *author, const char *message);
+static int reflog_append(refdb_fs_backend *backend, const git_reference *ref, const git_oid *old, const git_oid *new, const git_signature *author, const char *message);
 static int has_reflog(git_repository *repo, const char *name);
 
 /* We only write if it's under heads/, remotes/ or notes/ or if it already has a log */
@@ -974,6 +975,40 @@ out:
 	return error;
 }
 
+/*
+ * The git.git comment regarding this, for your viewing pleasure:
+ *
+ * Special hack: If a branch is updated directly and HEAD
+ * points to it (may happen on the remote side of a push
+ * for example) then logically the HEAD reflog should be
+ * updated too.
+ * A generic solution implies reverse symref information,
+ * but finding all symrefs pointing to the given branch
+ * would be rather costly for this rare event (the direct
+ * update of a branch) to be worth it.  So let's cheat and
+ * check with HEAD only which should cover 99% of all usage
+ * scenarios (even 100% of the default ones).
+ */
+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_reference *head;
+
+	error = git_reference_name_to_id(&old_id, backend->repo, ref->name);
+	if (!git_branch_is_head(ref))
+		return 0;
+
+	if ((error = git_reference_lookup(&head, backend->repo, "HEAD")) < 0)
+		return error;
+
+	error = reflog_append(backend, head, &old_id, git_reference_target(ref), who, message);
+
+	git_reference_free(head);
+	return error;
+}
+
+
 static int refdb_fs_backend__write(
 	git_refdb_backend *_backend,
 	const git_reference *ref,
@@ -1006,10 +1041,11 @@ static int refdb_fs_backend__write(
 		goto on_error;
 	}
 
-	if (should_write_reflog(backend->repo, ref->name) &&
-	    (error = reflog_append(backend, ref, who, message)) < 0) {
-		git_filebuf_cleanup(&file);
-		return error;
+	if (should_write_reflog(backend->repo, ref->name)) {
+		if ((error = reflog_append(backend, ref, NULL, NULL, who, message)) < 0)
+			goto on_error;
+		if ((error = maybe_append_head(backend, ref, who, message)) < 0)
+			goto on_error;
 	}
 
 	return loose_commit(&file, ref);
@@ -1128,7 +1164,7 @@ static int refdb_fs_backend__rename(
 	/* Try to rename the refog; it's ok if the old doesn't exist */
 	error = refdb_reflog_fs__rename(_backend, old_name, new_name);
 	if (((error == 0) || (error == GIT_ENOTFOUND)) &&
-	    ((error = reflog_append(backend, new, who, message)) < 0)) {
+	    ((error = reflog_append(backend, new, NULL, NULL, who, message)) < 0)) {
 		git_reference_free(new);
 		git_filebuf_cleanup(&file);
 		return error;
@@ -1517,34 +1553,61 @@ 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_signature *who, const char *message)
+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;
-	git_oid old_id, new_id = {{0}};
+	int error, is_symbolic, was_symbolic = 0;
+	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;
 
-	/* Creation of a symbolic reference doesn't get a reflog entry, except for
-	 * HEAD. git_repository_set_head and friends go through here. */
-	if (ref->type == GIT_REF_SYMBOLIC &&
-	    0 != strcmp(ref->name, GIT_HEAD_FILE))
+	is_symbolic = ref->type == GIT_REF_SYMBOLIC;
+
+	/* "normal" symbolic updates do not write */
+	if (is_symbolic &&
+	    strcmp(ref->name, GIT_HEAD_FILE) &&
+	    !(old && new))
 		return 0;
 
-	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)
+	error = git_reference_lookup(&current_ref, repo, ref->name);
+	if (error < 0 && error != GIT_ENOTFOUND)
 		return error;
 
-	if (git_reference_symbolic_target(ref) != NULL) {
+	if (current_ref)
+		was_symbolic = current_ref->type == GIT_REF_SYMBOLIC;
+
+	/* From here on is_symoblic also means that it's HEAD */
+
+	if (old) {
+		git_oid_cpy(&old_id, old);
+	} else if (!was_symbolic) {
+		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)
+			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;
+		/* detaching HEAD does not create an entry */
+		if (error == GIT_ENOTFOUND) {
+			error = 0;
+			goto cleanup;
+		}
+
 		giterr_clear();
 	}
-	else if (git_reference_target(ref) != NULL)
+
+
+	if (new)
+		git_oid_cpy(&new_id, new);
+	else if (!is_symbolic)
 		git_oid_cpy(&new_id, git_reference_target(ref));
 
 	if ((error = serialize_reflog_entry(&buf, &old_id, &new_id, who, message)) < 0)
diff --git a/src/refs.c b/src/refs.c
index e63796c94..8b6e09a33 100644
--- a/src/refs.c
+++ b/src/refs.c
@@ -1137,7 +1137,7 @@ int git_reference__is_branch(const char *ref_name)
 	return git__prefixcmp(ref_name, GIT_REFS_HEADS_DIR) == 0;
 }
 
-int git_reference_is_branch(git_reference *ref)
+int git_reference_is_branch(const git_reference *ref)
 {
 	assert(ref);
 	return git_reference__is_branch(ref->name);
diff --git a/tests/refs/reflog/reflog.c b/tests/refs/reflog/reflog.c
index 3f7d7d777..149e98273 100644
--- a/tests/refs/reflog/reflog.c
+++ b/tests/refs/reflog/reflog.c
@@ -214,3 +214,26 @@ void test_refs_reflog_reflog__write_when_explicitly_active(void)
 	git_reference_free(ref);
 	assert_has_reflog(true, "refs/tags/foo");
 }
+
+void test_refs_reflog_reflog__append_to_HEAD_when_changing_current_branch(void)
+{
+	size_t nlogs, nlogs_after;
+	git_reference *ref;
+	git_reflog *log;
+	git_oid id;
+
+	cl_git_pass(git_reflog_read(&log, g_repo, "HEAD"));
+	nlogs = git_reflog_entrycount(log);
+	git_reflog_free(log);
+
+	/* Move it back */
+	git_oid_fromstr(&id, "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
+	cl_git_pass(git_reference_create(&ref, g_repo, "refs/heads/master", &id, 1, NULL, NULL));
+	git_reference_free(ref);
+
+	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 + 1);
+}
diff --git a/tests/repo/head.c b/tests/repo/head.c
index c5965fac6..d88fd90d1 100644
--- a/tests/repo/head.c
+++ b/tests/repo/head.c
@@ -262,32 +262,10 @@ void test_repo_head__setting_head_updates_reflog(void)
 	cl_git_pass(git_repository_set_head_detached(repo, git_object_id(tag), sig, "message3"));
 	cl_git_pass(git_repository_set_head(repo, "refs/heads/haacked", sig, "message4"));
 
-	test_reflog(repo, 3, NULL, "refs/heads/haacked", "foo@example.com", "message1");
-	test_reflog(repo, 2, "refs/heads/haacked", NULL, "foo@example.com", "message2");
+	test_reflog(repo, 2, NULL, "refs/heads/haacked", "foo@example.com", "message1");
 	test_reflog(repo, 1, NULL, "tags/test^{commit}", "foo@example.com", "message3");
 	test_reflog(repo, 0, "tags/test^{commit}", "refs/heads/haacked", "foo@example.com", "message4");
 
 	git_object_free(tag);
 	git_signature_free(sig);
 }
-
-void test_repo_head__setting_creates_head_ref(void)
-{
-	git_reference *head;
-	git_reflog *log;
-	const git_reflog_entry *entry;
-
-	cl_git_pass(git_reference_lookup(&head, repo, "HEAD"));
-	cl_git_pass(git_reference_delete(head));
-	cl_git_pass(git_reflog_delete(repo, "HEAD"));
-
-	cl_git_pass(git_repository_set_head(repo, "refs/heads/haacked", NULL, "create HEAD"));
-
-	cl_git_pass(git_reflog_read(&log, repo, "HEAD"));
-	cl_assert_equal_i(1, git_reflog_entrycount(log));
-	entry = git_reflog_entry_byindex(log, 0);
-	cl_assert_equal_s("create HEAD", git_reflog_entry_message(entry));
-
-	git_reflog_free(log);
-	git_reference_free(head);
-}
diff --git a/tests/reset/hard.c b/tests/reset/hard.c
index 36120ee63..c6bf7a8ac 100644
--- a/tests/reset/hard.c
+++ b/tests/reset/hard.c
@@ -215,16 +215,18 @@ void test_reset_hard__reflog_is_correct(void)
 	git_object_free(target);
 
 	/* Moved branch, expect default message */
+	exp_msg = "reset: moving";
 	cl_git_pass(git_revparse_single(&target, repo, "HEAD~^{commit}"));
 	cl_git_pass(git_reset(repo, target, GIT_RESET_HARD, NULL, NULL));
-	reflog_check(repo, "HEAD", 3, "emeric.fermas@gmail.com", exp_msg);
-	reflog_check(repo, "refs/heads/master", 4, NULL, "reset: moving");
+	reflog_check(repo, "HEAD", 4, NULL, exp_msg);
+	reflog_check(repo, "refs/heads/master", 4, NULL, exp_msg);
 
 	git_object_free(target);
 
 	/* Moved branch, expect custom message */
+	exp_msg = "message1";
 	cl_git_pass(git_revparse_single(&target, repo, "HEAD~^{commit}"));
 	cl_git_pass(git_reset(repo, target, GIT_RESET_HARD, NULL, "message1"));
-	reflog_check(repo, "HEAD", 3, "emeric.fermas@gmail.com", exp_msg);
-	reflog_check(repo, "refs/heads/master", 5, NULL, "message1");
+	reflog_check(repo, "HEAD", 5, NULL, exp_msg);
+	reflog_check(repo, "refs/heads/master", 5, NULL, exp_msg);
 }
diff --git a/tests/reset/mixed.c b/tests/reset/mixed.c
index 5d8ff63b4..55b8a2f88 100644
--- a/tests/reset/mixed.c
+++ b/tests/reset/mixed.c
@@ -65,17 +65,19 @@ void test_reset_mixed__reflog_is_correct(void)
 	target = NULL;
 
 	/* Moved branch, expect default message */
+	exp_msg = "reset: moving";
 	cl_git_pass(git_revparse_single(&target, repo, "HEAD~^{commit}"));
 	cl_git_pass(git_reset(repo, target, GIT_RESET_MIXED, NULL, NULL));
-	reflog_check(repo, "HEAD", 9, "yoram.harmelin@gmail.com", exp_msg);
-	reflog_check(repo, "refs/heads/master", 10, NULL, "reset: moving");
+	reflog_check(repo, "HEAD", 10, NULL, exp_msg);
+	reflog_check(repo, "refs/heads/master", 10, NULL, exp_msg);
 
 	git_object_free(target);
 	target = NULL;
 
 	/* Moved branch, expect custom message */
+	exp_msg = "message1";
 	cl_git_pass(git_revparse_single(&target, repo, "HEAD~^{commit}"));
 	cl_git_pass(git_reset(repo, target, GIT_RESET_MIXED, NULL, "message1"));
-	reflog_check(repo, "HEAD", 9, "yoram.harmelin@gmail.com", exp_msg);
-	reflog_check(repo, "refs/heads/master", 11, NULL, "message1");
+	reflog_check(repo, "HEAD", 11, NULL, exp_msg);
+	reflog_check(repo, "refs/heads/master", 11, NULL, exp_msg);
 }