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

["Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>]

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>
---
 Junio's point, that if HEAD holds a tag, then head_sha1 and head->object.sha1
 in statement [1] are different, is entirely correct. However, favoring
 head->object.sha1 over head_sha1 is not enough. The variable head_sha1 is
 still there. Somewhere, some time, people may misuse it.

  [1] head = lookup_commit_or_die(head_sha1, "HEAD");

 Better update head_sha1 to new value in this case, which is the new change
 in lookup_commit_or_die().

 Or maybe a better approach is

  int get_commit_sha1(const char *ref, unsigned char *sha1);
 
 where it only returns zero if it can resolve to SHA-1 of a commit.

 builtin/commit.c        |   11 +++++------
 builtin/fmt-merge-msg.c |    2 +-
 builtin/merge.c         |    7 ++-----
 commit.c                |   19 +++++++++++++++++++
 commit.h                |    1 +
 http-push.c             |    8 ++++----
 revision.c              |    6 ++++--
 7 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index c9c4ea5..72e2cc5 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1393,11 +1393,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	if (get_sha1("HEAD", head_sha1))
 		initial_commit = 1;
-	else {
-		head_commit = lookup_commit(head_sha1);
-		if (!head_commit || parse_commit(head_commit))
-			die(_("could not parse HEAD commit"));
-	}
+	else
+		head_commit = lookup_commit_or_die(head_sha1, "HEAD");
 
 	if (s.use_color == -1)
 		s.use_color = git_use_color_default;
@@ -1433,6 +1430,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)
@@ -1446,7 +1444,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 b7260f5..39d9ac8 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1034,11 +1034,8 @@ 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)) {
-		head_commit = lookup_commit(head);
-		if (!head_commit)
-			die(_("could not parse HEAD"));
-	}
+	if (!is_null_sha1(head))
+		head_commit = lookup_commit_or_die(head, "HEAD");
 
 	git_config(git_merge_config, NULL);
 
diff --git a/commit.c b/commit.c
index ac337c7..9e7f7ef 100644
--- a/commit.c
+++ b/commit.c
@@ -39,6 +39,25 @@ struct commit *lookup_commit_reference(const unsigned char *sha1)
 	return lookup_commit_reference_gently(sha1, 0);
 }
 
+/*
+ * Look sha1 up for a commit, defer if needed. If deference 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(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));
+		hashcpy(sha1, c->object.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..a098b4c 100644
--- a/commit.h
+++ b/commit.h
@@ -37,6 +37,7 @@ 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);
+struct commit *lookup_commit_or_die(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 6e8f6d0..7ccff8f 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.4.74.g639db