[PATCH v5 1/4] merge: keep stash[] a local variable

[PATCH v5 1/4] merge: keep stash[] a local variable

[Nguyễn Thái Ngọc Duy]

A stash is created by save_state() and used by restore_state(). Pass
SHA-1 explicitly for clarity and keep stash[] to cmd_merge().

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 v4 was http://thread.gmane.org/gmane.comp.version-control.git/179375/focus=179706

 builtin/merge.c |   33 ++++++++++++++++-----------------
 1 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 325891e..a068660 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -50,7 +50,7 @@ static int fast_forward_only;
 static int allow_trivial = 1, have_message;
 static struct strbuf merge_msg;
 static struct commit_list *remoteheads;
-static unsigned char head[20], stash[20];
+static unsigned char head[20];
 static struct strategy **use_strategies;
 static size_t use_strategies_nr, use_strategies_alloc;
 static const char **xopts;
@@ -217,7 +217,7 @@ static void drop_save(void)
 	unlink(git_path("MERGE_MODE"));
 }
 
-static void save_state(void)
+static int save_state(unsigned char *stash)
 {
 	int len;
 	struct child_process cp;
@@ -236,11 +236,12 @@ static void save_state(void)
 
 	if (finish_command(&cp) || len < 0)
 		die(_("stash failed"));
-	else if (!len)
-		return;
+	else if (!len)		/* no changes */
+		return -1;
 	strbuf_setlen(&buffer, buffer.len-1);
 	if (get_sha1(buffer.buf, stash))
 		die(_("not a valid object: %s"), buffer.buf);
+	return 0;
 }
 
 static void read_empty(unsigned const char *sha1, int verbose)
@@ -278,7 +279,7 @@ static void reset_hard(unsigned const char *sha1, int verbose)
 		die(_("read-tree failed"));
 }
 
-static void restore_state(void)
+static void restore_state(const unsigned char *stash)
 {
 	struct strbuf sb = STRBUF_INIT;
 	const char *args[] = { "stash", "apply", NULL, NULL };
@@ -1010,6 +1011,7 @@ static int setup_with_upstream(const char ***argv)
 int cmd_merge(int argc, const char **argv, const char *prefix)
 {
 	unsigned char result_tree[20];
+	unsigned char stash[20];
 	struct strbuf buf = STRBUF_INIT;
 	const char *head_arg;
 	int flag, head_invalid = 0, i;
@@ -1320,21 +1322,18 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * sync with the head commit.  The strategies are responsible
 	 * to ensure this.
 	 */
-	if (use_strategies_nr != 1) {
-		/*
-		 * Stash away the local changes so that we can try more
-		 * than one.
-		 */
-		save_state();
-	} else {
-		memcpy(stash, null_sha1, 20);
-	}
+	if (use_strategies_nr == 1 ||
+	    /*
+	     * Stash away the local changes so that we can try more than one.
+	     */
+	    save_state(stash))
+		hashcpy(stash, null_sha1);
 
 	for (i = 0; i < use_strategies_nr; i++) {
 		int ret;
 		if (i) {
 			printf(_("Rewinding the tree to pristine...\n"));
-			restore_state();
+			restore_state(stash);
 		}
 		if (use_strategies_nr != 1)
 			printf(_("Trying merge strategy %s...\n"),
@@ -1395,7 +1394,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * it up.
 	 */
 	if (!best_strategy) {
-		restore_state();
+		restore_state(stash);
 		if (use_strategies_nr > 1)
 			fprintf(stderr,
 				_("No merge strategy handled the merge.\n"));
@@ -1407,7 +1406,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		; /* We already have its result in the working tree. */
 	else {
 		printf(_("Rewinding the tree to pristine...\n"));
-		restore_state();
+		restore_state(stash);
 		printf(_("Using the %s to prepare resolving by hand.\n"),
 			best_strategy);
 		try_merge_strategy(best_strategy, common, head_arg);
-- 
1.7.3.1.256.g2539c.dirty

[PATCH v5 2/4] merge: use return value of resolve_ref() to determine if HEAD is invalid

[Nguyễn Thái Ngọc Duy]

resolve_ref() only updates "head" when it returns non NULL value (it
may update "head" even when returning NULL, but not in all cases).

Because "head" is not initialized before the call, is_null_sha1() is
not enough. Check also resolve_ref() return value.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 > Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
 >> -     if (is_null_sha1(head))
 >> -             head_invalid = 1;
 >> +     if (!is_null_sha1(head)) {
 >> +             head_commit = lookup_commit(head);
 >> +             if (!head_commit)
 >> +                     die(_("could not parse HEAD"));
 >> +     }
 >
 > Is this is_null_sha1() valid without first clearing head[]?

 No it's not.

 builtin/merge.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index a068660..c371484 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1030,7 +1030,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	branch = resolve_ref("HEAD", head, 0, &flag);
 	if (branch && !prefixcmp(branch, "refs/heads/"))
 		branch += 11;
-	if (is_null_sha1(head))
+	if (!branch || is_null_sha1(head))
 		head_invalid = 1;
 
 	git_config(git_merge_config, NULL);
-- 
1.7.3.1.256.g2539c.dirty

[PATCH v5 3/4] merge: remove global variable head[]

[Nguyễn Thái Ngọc Duy]

Also kill head_invalid in favor of "head_commit == NULL".

Local variable "head" in cmd_merge() is renamed to "head_sha1" to make
sure I don't miss any access because this variable should not be used
after head_commit is set (use head_commit->object.sha1 instead).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/merge.c |   97 ++++++++++++++++++++++++++++++-------------------------
 1 files changed, 53 insertions(+), 44 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index c371484..f5eb3f5 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -50,7 +50,6 @@ static int fast_forward_only;
 static int allow_trivial = 1, have_message;
 static struct strbuf merge_msg;
 static struct commit_list *remoteheads;
-static unsigned char head[20];
 static struct strategy **use_strategies;
 static size_t use_strategies_nr, use_strategies_alloc;
 static const char **xopts;
@@ -279,7 +278,8 @@ static void reset_hard(unsigned const char *sha1, int verbose)
 		die(_("read-tree failed"));
 }
 
-static void restore_state(const unsigned char *stash)
+static void restore_state(const unsigned char *head,
+			  const unsigned char *stash)
 {
 	struct strbuf sb = STRBUF_INIT;
 	const char *args[] = { "stash", "apply", NULL, NULL };
@@ -309,10 +309,9 @@ static void finish_up_to_date(const char *msg)
 	drop_save();
 }
 
-static void squash_message(void)
+static void squash_message(struct commit *commit)
 {
 	struct rev_info rev;
-	struct commit *commit;
 	struct strbuf out = STRBUF_INIT;
 	struct commit_list *j;
 	int fd;
@@ -327,7 +326,6 @@ static void squash_message(void)
 	rev.ignore_merges = 1;
 	rev.commit_format = CMIT_FMT_MEDIUM;
 
-	commit = lookup_commit(head);
 	commit->object.flags |= UNINTERESTING;
 	add_pending_object(&rev, &commit->object, NULL);
 
@@ -356,9 +354,11 @@ static void squash_message(void)
 	strbuf_release(&out);
 }
 
-static void finish(const unsigned char *new_head, const char *msg)
+static void finish(struct commit *head_commit,
+		   const unsigned char *new_head, const char *msg)
 {
 	struct strbuf reflog_message = STRBUF_INIT;
+	const unsigned char *head = head_commit->object.sha1;
 
 	if (!msg)
 		strbuf_addstr(&reflog_message, getenv("GIT_REFLOG_ACTION"));
@@ -369,7 +369,7 @@ static void finish(const unsigned char *new_head, const char *msg)
 			getenv("GIT_REFLOG_ACTION"), msg);
 	}
 	if (squash) {
-		squash_message();
+		squash_message(head_commit);
 	} else {
 		if (verbosity >= 0 && !merge_msg.len)
 			printf(_("No merge message -- not updating HEAD\n"));
@@ -664,7 +664,7 @@ int try_merge_command(const char *strategy, size_t xopts_nr,
 }
 
 static int try_merge_strategy(const char *strategy, struct commit_list *common,
-			      const char *head_arg)
+			      struct commit *head, const char *head_arg)
 {
 	int index_fd;
 	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
@@ -710,7 +710,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 			commit_list_insert(j->item, &reversed);
 
 		index_fd = hold_locked_index(lock, 1);
-		clean = merge_recursive(&o, lookup_commit(head),
+		clean = merge_recursive(&o, head,
 				remoteheads->item, reversed, &result);
 		if (active_cache_changed &&
 				(write_cache(index_fd, active_cache, active_nr) ||
@@ -861,25 +861,26 @@ static void run_prepare_commit_msg(void)
 	read_merge_msg();
 }
 
-static int merge_trivial(void)
+static int merge_trivial(struct commit *head)
 {
 	unsigned char result_tree[20], result_commit[20];
 	struct commit_list *parent = xmalloc(sizeof(*parent));
 
 	write_tree_trivial(result_tree);
 	printf(_("Wonderful.\n"));
-	parent->item = lookup_commit(head);
+	parent->item = head;
 	parent->next = xmalloc(sizeof(*parent->next));
 	parent->next->item = remoteheads->item;
 	parent->next->next = NULL;
 	run_prepare_commit_msg();
 	commit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL);
-	finish(result_commit, "In-index merge");
+	finish(head, result_commit, "In-index merge");
 	drop_save();
 	return 0;
 }
 
-static int finish_automerge(struct commit_list *common,
+static int finish_automerge(struct commit *head,
+			    struct commit_list *common,
 			    unsigned char *result_tree,
 			    const char *wt_strategy)
 {
@@ -890,12 +891,12 @@ static int finish_automerge(struct commit_list *common,
 	free_commit_list(common);
 	if (allow_fast_forward) {
 		parents = remoteheads;
-		commit_list_insert(lookup_commit(head), &parents);
+		commit_list_insert(head, &parents);
 		parents = reduce_heads(parents);
 	} else {
 		struct commit_list **pptr = &parents;
 
-		pptr = &commit_list_insert(lookup_commit(head),
+		pptr = &commit_list_insert(head,
 				pptr)->next;
 		for (j = remoteheads; j; j = j->next)
 			pptr = &commit_list_insert(j->item, pptr)->next;
@@ -905,7 +906,7 @@ static int finish_automerge(struct commit_list *common,
 	run_prepare_commit_msg();
 	commit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL);
 	strbuf_addf(&buf, "Merge made by %s.", wt_strategy);
-	finish(result_commit, buf.buf);
+	finish(head, result_commit, buf.buf);
 	strbuf_release(&buf);
 	drop_save();
 	return 0;
@@ -939,7 +940,8 @@ static int suggest_conflicts(int renormalizing)
 	return 1;
 }
 
-static struct commit *is_old_style_invocation(int argc, const char **argv)
+static struct commit *is_old_style_invocation(int argc, const char **argv,
+					      const unsigned char *head)
 {
 	struct commit *second_token = NULL;
 	if (argc > 2) {
@@ -1012,9 +1014,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 {
 	unsigned char result_tree[20];
 	unsigned char stash[20];
+	unsigned char head_sha1[20];
+	struct commit *head_commit;
 	struct strbuf buf = STRBUF_INIT;
 	const char *head_arg;
-	int flag, head_invalid = 0, i;
+	int flag, i;
 	int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
 	struct commit_list *common = NULL;
 	const char *best_strategy = NULL, *wt_strategy = NULL;
@@ -1027,11 +1031,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * Check if we are _not_ on a detached HEAD, i.e. if there is a
 	 * current branch.
 	 */
-	branch = resolve_ref("HEAD", head, 0, &flag);
+	branch = resolve_ref("HEAD", head_sha1, 0, &flag);
 	if (branch && !prefixcmp(branch, "refs/heads/"))
 		branch += 11;
-	if (!branch || is_null_sha1(head))
-		head_invalid = 1;
+	if (!branch || is_null_sha1(head_sha1))
+		head_commit = NULL;
+	else {
+		head_commit = lookup_commit(head_sha1);
+		if (!head_commit)
+			die(_("could not parse HEAD"));
+	}
 
 	git_config(git_merge_config, NULL);
 
@@ -1112,12 +1121,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * additional safety measure to check for it.
 	 */
 
-	if (!have_message && is_old_style_invocation(argc, argv)) {
+	if (!have_message && head_commit &&
+	    is_old_style_invocation(argc, argv, head_commit->object.sha1)) {
 		strbuf_addstr(&merge_msg, argv[0]);
 		head_arg = argv[1];
 		argv += 2;
 		argc -= 2;
-	} else if (head_invalid) {
+	} else if (!head_commit) {
 		struct object *remote_head;
 		/*
 		 * If the merged head is a valid one there is no reason
@@ -1164,7 +1174,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	if (head_invalid || !argc)
+	if (!head_commit || !argc)
 		usage_with_options(builtin_merge_usage,
 			builtin_merge_options);
 
@@ -1205,17 +1215,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	}
 
 	if (!remoteheads->next)
-		common = get_merge_bases(lookup_commit(head),
-				remoteheads->item, 1);
+		common = get_merge_bases(head_commit, remoteheads->item, 1);
 	else {
 		struct commit_list *list = remoteheads;
-		commit_list_insert(lookup_commit(head), &list);
+		commit_list_insert(head_commit, &list);
 		common = get_octopus_merge_bases(list);
 		free(list);
 	}
 
-	update_ref("updating ORIG_HEAD", "ORIG_HEAD", head, NULL, 0,
-		DIE_ON_ERR);
+	update_ref("updating ORIG_HEAD", "ORIG_HEAD", head_commit->object.sha1,
+		   NULL, 0, DIE_ON_ERR);
 
 	if (!common)
 		; /* No common ancestors found. We need a real merge. */
@@ -1229,13 +1238,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		return 0;
 	} else if (allow_fast_forward && !remoteheads->next &&
 			!common->next &&
-			!hashcmp(common->item->object.sha1, head)) {
+			!hashcmp(common->item->object.sha1, head_commit->object.sha1)) {
 		/* Again the most common case of merging one remote. */
 		struct strbuf msg = STRBUF_INIT;
 		struct object *o;
 		char hex[41];
 
-		strcpy(hex, find_unique_abbrev(head, DEFAULT_ABBREV));
+		strcpy(hex, find_unique_abbrev(head_commit->object.sha1, DEFAULT_ABBREV));
 
 		if (verbosity >= 0)
 			printf(_("Updating %s..%s\n"),
@@ -1251,10 +1260,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		if (!o)
 			return 1;
 
-		if (checkout_fast_forward(head, remoteheads->item->object.sha1))
+		if (checkout_fast_forward(head_commit->object.sha1, remoteheads->item->object.sha1))
 			return 1;
 
-		finish(o->sha1, msg.buf);
+		finish(head_commit, o->sha1, msg.buf);
 		drop_save();
 		return 0;
 	} else if (!remoteheads->next && common->next)
@@ -1274,8 +1283,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			git_committer_info(IDENT_ERROR_ON_NO_NAME);
 			printf(_("Trying really trivial in-index merge...\n"));
 			if (!read_tree_trivial(common->item->object.sha1,
-					head, remoteheads->item->object.sha1))
-				return merge_trivial();
+					head_commit->object.sha1, remoteheads->item->object.sha1))
+				return merge_trivial(head_commit);
 			printf(_("Nope.\n"));
 		}
 	} else {
@@ -1294,8 +1303,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			 * merge_bases again, otherwise "git merge HEAD^
 			 * HEAD^^" would be missed.
 			 */
-			common_one = get_merge_bases(lookup_commit(head),
-				j->item, 1);
+			common_one = get_merge_bases(head_commit, j->item, 1);
 			if (hashcmp(common_one->item->object.sha1,
 				j->item->object.sha1)) {
 				up_to_date = 0;
@@ -1333,7 +1341,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		int ret;
 		if (i) {
 			printf(_("Rewinding the tree to pristine...\n"));
-			restore_state(stash);
+			restore_state(head_commit->object.sha1, stash);
 		}
 		if (use_strategies_nr != 1)
 			printf(_("Trying merge strategy %s...\n"),
@@ -1345,7 +1353,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		wt_strategy = use_strategies[i]->name;
 
 		ret = try_merge_strategy(use_strategies[i]->name,
-			common, head_arg);
+					 common, head_commit, head_arg);
 		if (!option_commit && !ret) {
 			merge_was_ok = 1;
 			/*
@@ -1387,14 +1395,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * auto resolved the merge cleanly.
 	 */
 	if (automerge_was_ok)
-		return finish_automerge(common, result_tree, wt_strategy);
+		return finish_automerge(head_commit, common, result_tree,
+					wt_strategy);
 
 	/*
 	 * Pick the result from the best strategy and have the user fix
 	 * it up.
 	 */
 	if (!best_strategy) {
-		restore_state(stash);
+		restore_state(head_commit->object.sha1, stash);
 		if (use_strategies_nr > 1)
 			fprintf(stderr,
 				_("No merge strategy handled the merge.\n"));
@@ -1406,14 +1415,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		; /* We already have its result in the working tree. */
 	else {
 		printf(_("Rewinding the tree to pristine...\n"));
-		restore_state(stash);
+		restore_state(head_commit->object.sha1, stash);
 		printf(_("Using the %s to prepare resolving by hand.\n"),
 			best_strategy);
-		try_merge_strategy(best_strategy, common, head_arg);
+		try_merge_strategy(best_strategy, common, head_commit, head_arg);
 	}
 
 	if (squash)
-		finish(NULL, NULL);
+		finish(head_commit, NULL, NULL);
 	else {
 		int fd;
 		struct commit_list *j;
-- 
1.7.3.1.256.g2539c.dirty

[PATCH v5 4/4] Accept tags in HEAD or MERGE_HEAD

[Nguyễn Thái Ngọc Duy]

HEAD and MERGE_HEAD (among other branch tips) should never hold a
tag. That can only be caused by broken tools and is cumbersome to fix
by an end user with:

  $ git update-ref HEAD $(git rev-parse HEAD^{commit})

which may look like a magic to a new person.

Be easy, warn users (so broken tools can be fixed if they bother to
report) and move on.

Be robust, if the given SHA-1 cannot be resolved to a commit object,
die (therefore return value is always valid).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/commit.c        |    6 ++++--
 builtin/fmt-merge-msg.c |    2 +-
 builtin/merge.c         |    7 ++-----
 commit.c                |   12 ++++++++++++
 commit.h                |    7 +++++++
 http-push.c             |    8 ++++----
 revision.c              |    6 ++++--
 7 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1a65319..402eb5a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1396,7 +1396,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	if (get_sha1("HEAD", sha1))
 		current_head = NULL;
 	else {
-		current_head = lookup_commit(sha1);
+		current_head = lookup_commit_or_die(sha1, "HEAD");
 		if (!current_head || parse_commit(current_head))
 			die(_("could not parse HEAD commit"));
 	}
@@ -1431,6 +1431,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			pptr = &commit_list_insert(c->item, pptr)->next;
 	} else if (whence == FROM_MERGE) {
 		struct strbuf m = STRBUF_INIT;
+		struct commit *commit;
 		FILE *fp;
 
 		if (!reflog_msg)
@@ -1444,7 +1445,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			unsigned char sha1[20];
 			if (get_sha1_hex(m.buf, sha1) < 0)
 				die(_("Corrupt MERGE_HEAD file (%s)"), m.buf);
-			pptr = &commit_list_insert(lookup_commit(sha1), pptr)->next;
+			commit = lookup_commit_or_die(sha1, "MERGE_HEAD");
+			pptr = &commit_list_insert(commit, pptr)->next;
 		}
 		fclose(fp);
 		strbuf_release(&m);
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 7581632..7e2f225 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -293,7 +293,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
 		struct commit *head;
 		struct rev_info rev;
 
-		head = lookup_commit(head_sha1);
+		head = lookup_commit_or_die(head_sha1, "HEAD");
 		init_revisions(&rev, NULL);
 		rev.commit_format = CMIT_FMT_ONELINE;
 		rev.ignore_merges = 1;
diff --git a/builtin/merge.c b/builtin/merge.c
index f5eb3f5..9567d60 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1036,11 +1036,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		branch += 11;
 	if (!branch || is_null_sha1(head_sha1))
 		head_commit = NULL;
-	else {
-		head_commit = lookup_commit(head_sha1);
-		if (!head_commit)
-			die(_("could not parse HEAD"));
-	}
+	else
+		head_commit = lookup_commit_or_die(head_sha1, "HEAD");
 
 	git_config(git_merge_config, NULL);
 
diff --git a/commit.c b/commit.c
index ac337c7..50fcf96 100644
--- a/commit.c
+++ b/commit.c
@@ -39,6 +39,18 @@ struct commit *lookup_commit_reference(const unsigned char *sha1)
 	return lookup_commit_reference_gently(sha1, 0);
 }
 
+struct commit *lookup_commit_or_die(const unsigned char *sha1, const char *ref_name)
+{
+	struct commit *c = lookup_commit_reference(sha1);
+	if (!c)
+		die(_("could not parse %s"), ref_name);
+	if (hashcmp(sha1, c->object.sha1)) {
+		warning(_("%s %s is not a commit!"),
+			ref_name, sha1_to_hex(sha1));
+	}
+	return c;
+}
+
 struct commit *lookup_commit(const unsigned char *sha1)
 {
 	struct object *obj = lookup_object(sha1);
diff --git a/commit.h b/commit.h
index a2d571b..044134a 100644
--- a/commit.h
+++ b/commit.h
@@ -37,6 +37,13 @@ struct commit *lookup_commit_reference(const unsigned char *sha1);
 struct commit *lookup_commit_reference_gently(const unsigned char *sha1,
 					      int quiet);
 struct commit *lookup_commit_reference_by_name(const char *name);
+/*
+ * Look sha1 up for a commit, defer if needed. If dereference occurs,
+ * update "sha1" for consistency with retval->object.sha1. Also warn
+ * users this case because it is expected that sha1 points directly to
+ * a commit.
+ */
+struct commit *lookup_commit_or_die(const unsigned char *sha1, const char *ref_name);
 
 int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size);
 int parse_commit(struct commit *item);
diff --git a/http-push.c b/http-push.c
index 28bfe76..d432b30 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1606,10 +1606,10 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)
 	strbuf_release(&buffer);
 }
 
-static int verify_merge_base(unsigned char *head_sha1, unsigned char *branch_sha1)
+static int verify_merge_base(unsigned char *head_sha1, struct ref *remote)
 {
-	struct commit *head = lookup_commit(head_sha1);
-	struct commit *branch = lookup_commit(branch_sha1);
+	struct commit *head = lookup_commit_or_die(head_sha1, "HEAD");
+	struct commit *branch = lookup_commit_or_die(remote->old_sha1, remote->name);
 	struct commit_list *merge_bases = get_merge_bases(head, branch, 1);
 
 	return (merge_bases && !merge_bases->next && merge_bases->item == branch);
@@ -1680,7 +1680,7 @@ static int delete_remote_branch(const char *pattern, int force)
 			return error("Remote branch %s resolves to object %s\nwhich does not exist locally, perhaps you need to fetch?", remote_ref->name, sha1_to_hex(remote_ref->old_sha1));
 
 		/* Remote branch must be an ancestor of remote HEAD */
-		if (!verify_merge_base(head_sha1, remote_ref->old_sha1)) {
+		if (!verify_merge_base(head_sha1, remote_ref)) {
 			return error("The branch '%s' is not an ancestor "
 				     "of your current HEAD.\n"
 				     "If you are sure you want to delete it,"
diff --git a/revision.c b/revision.c
index c46cfaa..5e057a0 100644
--- a/revision.c
+++ b/revision.c
@@ -986,10 +986,12 @@ static void prepare_show_merge(struct rev_info *revs)
 	const char **prune = NULL;
 	int i, prune_num = 1; /* counting terminating NULL */
 
-	if (get_sha1("HEAD", sha1) || !(head = lookup_commit(sha1)))
+	if (get_sha1("HEAD", sha1))
 		die("--merge without HEAD?");
-	if (get_sha1("MERGE_HEAD", sha1) || !(other = lookup_commit(sha1)))
+	head = lookup_commit_or_die(sha1, "HEAD");
+	if (get_sha1("MERGE_HEAD", sha1))
 		die("--merge without MERGE_HEAD?");
+	other = lookup_commit_or_die(sha1, "MERGE_HEAD");
 	add_pending_object(revs, &head->object, "HEAD");
 	add_pending_object(revs, &other->object, "MERGE_HEAD");
 	bases = get_merge_bases(head, other, 1);
-- 
1.7.3.1.256.g2539c.dirty

Re: [PATCH v5 4/4] Accept tags in HEAD or MERGE_HEAD

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> HEAD and MERGE_HEAD (among other branch tips) should never hold a
> tag. That can only be caused by broken tools and is cumbersome to fix
> by an end user with:
>
>   $ git update-ref HEAD $(git rev-parse HEAD^{commit})
>
> which may look like a magic to a new person.
> ...
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Nicely done.

Just a few micronits.

> diff --git a/commit.h b/commit.h
> ...
> +/*
> + * Look sha1 up for a commit, defer if needed. If dereference occurs,

That's s/defer/deref/;

> + * update "sha1" for consistency with retval->object.sha1. Also warn
> + * users this case because it is expected that sha1 points directly to
> + * a commit.
> + */
> +struct commit *lookup_commit_or_die(const unsigned char *sha1, const char *ref_name);

This no longer updates sha1 now it is a pointer to a const arena.
I'll update this part as follows, and queue the result.

/*
 * Look up object named by "sha1", dereference tag as necessary, 
 * get a commit and return it. If "sha1" does not dereference to
 * a commit, use ref_name to report an error and die.
 */

Thanks.