[PATCH] commit: check return value of lookup_commit()

[PATCH] commit: check return value of lookup_commit()

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

If lookup_commit() returns NULL, there's usually serious error and the
command aborts anyway. However it's still nicer to have a message
telling us where it aborts, rather than segmentation fault.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I suppose die() also cleans up $GIT_DIR/index.lock while sigsegv does not.

 builtin/commit.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 2088b6b..bfb7a5a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1387,6 +1387,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	unsigned char commit_sha1[20];
 	struct ref_lock *ref_lock;
 	struct commit_list *parents = NULL, **pptr = &parents;
+	struct commit *commit;
 	struct stat statbuf;
 	int allow_fast_forward = 1;
 	struct wt_status s;
@@ -1423,7 +1424,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			reflog_msg = "commit (initial)";
 	} else if (amend) {
 		struct commit_list *c;
-		struct commit *commit;
 
 		if (!reflog_msg)
 			reflog_msg = "commit (amend)";
@@ -1439,7 +1439,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 		if (!reflog_msg)
 			reflog_msg = "commit (merge)";
-		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
+		commit = lookup_commit(head_sha1);
+		if (!commit)
+			die(_("could not parse HEAD commit"));
+		pptr = &commit_list_insert(commit, pptr)->next;
 		fp = fopen(git_path("MERGE_HEAD"), "r");
 		if (fp == NULL)
 			die_errno(_("could not open '%s' for reading"),
@@ -1448,7 +1451,10 @@ 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(sha1);
+			if (!commit)
+				die(_("could not parse commit %s"), sha1_to_hex(sha1));
+			pptr = &commit_list_insert(commit, pptr)->next;
 		}
 		fclose(fp);
 		strbuf_release(&m);
@@ -1465,7 +1471,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			reflog_msg = (whence == FROM_CHERRY_PICK)
 					? "commit (cherry-pick)"
 					: "commit";
-		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
+		commit = lookup_commit(head_sha1);
+		if (!commit)
+			die(_("could not parse HEAD commit"));
+		pptr = &commit_list_insert(commit, pptr)->next;
 	}
 
 	/* Finally, get the commit message */
-- 
1.7.4.74.g639db

Re: [PATCH] commit: check return value of lookup_commit()

[Junio C Hamano]

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

> If lookup_commit() returns NULL, there's usually serious error and the
> command aborts anyway. However it's still nicer to have a message
> telling us where it aborts, rather than segmentation fault.

The change itself looks good to me but a point and a half to think about:

 - In this if/elseif/.../else cascade, everybody except for the
   "initial_commit" case needs to make sure that head_sha1 points at a
   valid commit and get an commit object. Hoisting the scope of the
   variable "commit" one level in your patch is good, but it would make it
   easier to read and the future code modification much less error prone
   if (1) you called lookup_commit() and checked for errors before
   entering this if/elseif/... cascade, and (2) you renamed this variable
   to "head_commit".

 - Whether we like it or not, many people have a broken reimplementations
   of git that can put a non-commit in HEAD, and they won't be fixed
   overnight. Instead of erroring out, would it be nicer of us if we just
   warned, unwrapped the tag and used the tagged commit instead?

Re: [PATCH] commit: check return value of lookup_commit()

[Nguyen Thai Ngoc Duy]

2011/8/16 Junio C Hamano <gitster@pobox.com>:
> The change itself looks good to me but a point and a half to think about:
>
>  - In this if/elseif/.../else cascade, everybody except for the
>   "initial_commit" case needs to make sure that head_sha1 points at a
>   valid commit and get an commit object. Hoisting the scope of the
>   variable "commit" one level in your patch is good, but it would make it
>   easier to read and the future code modification much less error prone
>   if (1) you called lookup_commit() and checked for errors before
>   entering this if/elseif/... cascade, and (2) you renamed this variable
>   to "head_commit".

But then I would need to avoid die()ing in "initial_commit" case. So
it becomes two related condition blocks (head_commit check and the
if/elseif...), more error prone to me.

>  - Whether we like it or not, many people have a broken reimplementations
>   of git that can put a non-commit in HEAD, and they won't be fixed
>   overnight. Instead of erroring out, would it be nicer of us if we just
>   warned, unwrapped the tag and used the tagged commit instead?

How about replacing those lookup_commit() with this? It would tolerate
tag-in-branch case, but also warn users that something's gone wrong.

struct commit *lookup_expect_commit(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;
}
-- 
Duy

Re: [PATCH] commit: check return value of lookup_commit()

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> 2011/8/16 Junio C Hamano <gitster@pobox.com>:
>> The change itself looks good to me but a point and a half to think about:
>>
>>  - In this if/elseif/.../else cascade, everybody except for the
>>   "initial_commit" case needs to make sure that head_sha1 points at a
>>   valid commit and get an commit object. Hoisting the scope of the
>>   variable "commit" one level in your patch is good, but it would make it
>>   easier to read and the future code modification much less error prone
>>   if (1) you called lookup_commit() and checked for errors before
>>   entering this if/elseif/... cascade, and (2) you renamed this variable
>>   to "head_commit".
>
> But then I would need to avoid die()ing in "initial_commit" case.

That's exactly what I said.

	if (!initial)
	        /* we need to know the head_commit */
                head_commit = lookup_and_check(HEAD);

	/* depending on what kind of commit, we need different stuff */
        if (initial)
        	... going to create a parentless commit
	else if (amending)
		... use the head_commit to learn parent, reuse the message
                ... from there
	else if ...

These two are independent if/else cascades in the sense that the first is
about learning the details of head_commit, and the latter is about
learning how the commit is done, and in a subset of the latter head_commit
is used.

>>  - Whether we like it or not, many people have a broken reimplementations
>>   of git that can put a non-commit in HEAD, and they won't be fixed
>>   overnight. Instead of erroring out, would it be nicer of us if we just
>>   warned, unwrapped the tag and used the tagged commit instead?
>
> How about replacing those lookup_commit() with this? It would tolerate
> tag-in-branch case, but also warn users that something's gone wrong.

Yes, that is exactly what I meant.

Thakns.

Re: [PATCH] commit: check return value of lookup_commit()

[Nguyen Thai Ngoc Duy]

On Wed, Aug 17, 2011 at 1:02 AM, Junio C Hamano >> But then I would
need to avoid die()ing in "initial_commit" case.
>
> That's exactly what I said.
>
>        if (!initial)
>                /* we need to know the head_commit */
>                head_commit = lookup_and_check(HEAD);
>
>        /* depending on what kind of commit, we need different stuff */
>        if (initial)
>                ... going to create a parentless commit
>        else if (amending)
>                ... use the head_commit to learn parent, reuse the message
>                ... from there
>        else if ...
>
> These two are independent if/else cascades in the sense that the first is
> about learning the details of head_commit, and the latter is about
> learning how the commit is done, and in a subset of the latter head_commit
> is used.

And some months from now, someone may add more code in between the two
"if" blocks. Some more months past, someone tweaks the "if (initial)"
block but forgets to also update the "if (!initial)" above. This is
what I want to avoid.
-- 
Duy

[PATCH v2] commit: accept tag objects in HEAD/MERGE_HEAD

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

HEAD, MERGE_HEAD (or any other branches) should only have SHA-1 of a
commit object. However broken tools can put a tag object there. While
it's wrong, it'd be better to tolerate the situation and move on
("commit" is an often used operation, unable to commit could be bad).

In worse cases, where we cannot even resolve HEAD to a commit, die()
instead of segfault.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/commit.c |   15 +++++++++------
 commit.c         |   12 ++++++++++++
 commit.h         |    1 +
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 2088b6b..f327595 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1387,6 +1387,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	unsigned char commit_sha1[20];
 	struct ref_lock *ref_lock;
 	struct commit_list *parents = NULL, **pptr = &parents;
+	struct commit *commit;
 	struct stat statbuf;
 	int allow_fast_forward = 1;
 	struct wt_status s;
@@ -1423,12 +1424,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			reflog_msg = "commit (initial)";
 	} else if (amend) {
 		struct commit_list *c;
-		struct commit *commit;
 
 		if (!reflog_msg)
 			reflog_msg = "commit (amend)";
-		commit = lookup_commit(head_sha1);
-		if (!commit || parse_commit(commit))
+		commit = lookup_expect_commit(head_sha1, "HEAD");
+		if (parse_commit(commit))
 			die(_("could not parse HEAD commit"));
 
 		for (c = commit->parents; c; c = c->next)
@@ -1439,7 +1439,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 		if (!reflog_msg)
 			reflog_msg = "commit (merge)";
-		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
+		commit = lookup_expect_commit(head_sha1, "HEAD");
+		pptr = &commit_list_insert(commit, pptr)->next;
 		fp = fopen(git_path("MERGE_HEAD"), "r");
 		if (fp == NULL)
 			die_errno(_("could not open '%s' for reading"),
@@ -1448,7 +1449,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_expect_commit(sha1, "MERGE_HEAD");
+			pptr = &commit_list_insert(commit, pptr)->next;
 		}
 		fclose(fp);
 		strbuf_release(&m);
@@ -1465,7 +1467,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			reflog_msg = (whence == FROM_CHERRY_PICK)
 					? "commit (cherry-pick)"
 					: "commit";
-		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
+		commit = lookup_expect_commit(head_sha1, "HEAD");
+		pptr = &commit_list_insert(commit, pptr)->next;
 	}
 
 	/* Finally, get the commit message */
diff --git a/commit.c b/commit.c
index ac337c7..dc22695 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_expect_commit(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 feb809f..0e36fd0 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_expect_commit(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);
-- 
1.7.4.74.g639db

Re: [PATCH v2] commit: accept tag objects in HEAD/MERGE_HEAD

[Junio C Hamano]

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

> HEAD, MERGE_HEAD (or any other branches) should only have SHA-1 of a
> commit object. However broken tools can put a tag object there. While
> it's wrong, it'd be better to tolerate the situation and move on

The best part in your patch is that you made it _warn_ when it happens; I
would suggest rewording this with s/situation/&, warn/.

> ("commit" is an often used operation, unable to commit could be bad).

Neither "often used" nor "unable to commit" is a good reason for this
added leniency. The real reason is that such a condition left by broken
tools 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.

By the way, what happens when you try to merge when HEAD points at a tag
that points at a commit? Would we end up creating a commit that points at
a bogus parent?

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 2088b6b..f327595 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1387,6 +1387,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	unsigned char commit_sha1[20];
>  	struct ref_lock *ref_lock;
>  	struct commit_list *parents = NULL, **pptr = &parents;
> +	struct commit *commit;
>  	struct stat statbuf;
>  	int allow_fast_forward = 1;
>  	struct wt_status s;

Here, you are being inconsistent with your own argument you made in your
previous message "later somebody may forget to update the former while
updating the latter" when I suggested to separate the two logically
separate operations (grab the head_commit object when necessary, and
decide how the commit is made). By hoisting of the scope of "commit", you
made the variable undefined when dealing with the initial_commit, exposing
the code to the same risk that somebody may try to use "commit" variable
after the if/elseif/... cascade, where it may or may not be defined.

Not that I buy your previous argument in this case---it's not like we have
deeply nested callchain that sometimes sets a variable and sometimes
doesn't. It's all there for the updater to see in a single function.

> @@ -1423,12 +1424,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  			reflog_msg = "commit (initial)";
>  	} else if (amend) {
>  		struct commit_list *c;
> -		struct commit *commit;
>  
>  		if (!reflog_msg)
>  			reflog_msg = "commit (amend)";
> -		commit = lookup_commit(head_sha1);
> -		if (!commit || parse_commit(commit))
> +		commit = lookup_expect_commit(head_sha1, "HEAD");
> +		if (parse_commit(commit))
>  			die(_("could not parse HEAD commit"));

Is this still necessary? I think your lookup_expect_commit() already
checks this condition and barfs.

Re: [PATCH v2] commit: accept tag objects in HEAD/MERGE_HEAD

[Nguyen Thai Ngoc Duy]

2011/8/18 Junio C Hamano <gitster@pobox.com>:
> By the way, what happens when you try to merge when HEAD points at a tag
> that points at a commit? Would we end up creating a commit that points at
> a bogus parent?

I guess I'd segfault the same way commit does. It does
lookup_commit(head) without checking the result.

>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 2088b6b..f327595 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1387,6 +1387,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>       unsigned char commit_sha1[20];
>>       struct ref_lock *ref_lock;
>>       struct commit_list *parents = NULL, **pptr = &parents;
>> +     struct commit *commit;
>>       struct stat statbuf;
>>       int allow_fast_forward = 1;
>>       struct wt_status s;
>
> Here, you are being inconsistent with your own argument you made in your
> previous message "later somebody may forget to update the former while
> updating the latter" when I suggested to separate the two logically
> separate operations (grab the head_commit object when necessary, and
> decide how the commit is made). By hoisting of the scope of "commit", you
> made the variable undefined when dealing with the initial_commit, exposing
> the code to the same risk that somebody may try to use "commit" variable
> after the if/elseif/... cascade, where it may or may not be defined.

Fair enough.

>> @@ -1423,12 +1424,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>                       reflog_msg = "commit (initial)";
>>       } else if (amend) {
>>               struct commit_list *c;
>> -             struct commit *commit;
>>
>>               if (!reflog_msg)
>>                       reflog_msg = "commit (amend)";
>> -             commit = lookup_commit(head_sha1);
>> -             if (!commit || parse_commit(commit))
>> +             commit = lookup_expect_commit(head_sha1, "HEAD");
>> +             if (parse_commit(commit))
>>                       die(_("could not parse HEAD commit"));
>
> Is this still necessary? I think your lookup_expect_commit() already
> checks this condition and barfs.

It is. lookup_expect_commit() does not parse commit content, only
makes sure it's a commit object.
-- 
Duy

[PATCH v3] 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) 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        |   25 ++++++++++++-------------
 builtin/fmt-merge-msg.c |    2 +-
 builtin/merge.c         |   19 +++++++++++--------
 commit.c                |   12 ++++++++++++
 commit.h                |    1 +
 http-push.c             |    8 ++++----
 revision.c              |    6 ++++--
 7 files changed, 45 insertions(+), 28 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index cb73857..f78b449 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -63,6 +63,7 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\
 "Otherwise, please use 'git reset'\n");
 
 static unsigned char head_sha1[20];
+static struct commit *head_commit;
 
 static const char *use_message_buffer;
 static const char commit_editmsg[] = "COMMIT_EDITMSG";
@@ -518,11 +519,8 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 	return s->commitable;
 }
 
-static int is_a_merge(const unsigned char *sha1)
+static int is_a_merge(struct commit *commit)
 {
-	struct commit *commit = lookup_commit(sha1);
-	if (!commit || parse_commit(commit))
-		die(_("could not parse HEAD commit"));
 	return !!(commit->parents && commit->parents->next);
 }
 
@@ -848,7 +846,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 * empty due to conflict resolution, which the user should okay.
 	 */
 	if (!commitable && whence != FROM_MERGE && !allow_empty &&
-	    !(amend && is_a_merge(head_sha1))) {
+	    !(amend && is_a_merge(head_commit))) {
 		run_status(stdout, index_file, prefix, 0, s);
 		if (amend)
 			fputs(_(empty_amend_advice), stderr);
@@ -1028,6 +1026,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
 
 	if (get_sha1("HEAD", head_sha1))
 		initial_commit = 1;
+	else
+		head_commit = lookup_expect_commit(head_sha1, "HEAD");
+
 
 	/* Sanity check options */
 	if (amend && initial_commit)
@@ -1421,23 +1422,20 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			reflog_msg = "commit (initial)";
 	} else if (amend) {
 		struct commit_list *c;
-		struct commit *commit;
 
 		if (!reflog_msg)
 			reflog_msg = "commit (amend)";
-		commit = lookup_commit(head_sha1);
-		if (!commit || parse_commit(commit))
-			die(_("could not parse HEAD commit"));
 
-		for (c = commit->parents; c; c = c->next)
+		for (c = head_commit->parents; c; c = c->next)
 			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)
 			reflog_msg = "commit (merge)";
-		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
+		pptr = &commit_list_insert(head_commit, pptr)->next;
 		fp = fopen(git_path("MERGE_HEAD"), "r");
 		if (fp == NULL)
 			die_errno(_("could not open '%s' for reading"),
@@ -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_expect_commit(sha1, "MERGE_HEAD");
+			pptr = &commit_list_insert(commit, pptr)->next;
 		}
 		fclose(fp);
 		strbuf_release(&m);
@@ -1463,7 +1462,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			reflog_msg = (whence == FROM_CHERRY_PICK)
 					? "commit (cherry-pick)"
 					: "commit";
-		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
+		pptr = &commit_list_insert(head_commit, pptr)->next;
 	}
 
 	/* Finally, get the commit message */
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 7581632..1a31b64 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_expect_commit(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 325891e..22e98e8 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -51,6 +51,7 @@ 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 struct commit* head_commit;
 static struct strategy **use_strategies;
 static size_t use_strategies_nr, use_strategies_alloc;
 static const char **xopts;
@@ -326,7 +327,7 @@ static void squash_message(void)
 	rev.ignore_merges = 1;
 	rev.commit_format = CMIT_FMT_MEDIUM;
 
-	commit = lookup_commit(head);
+	commit = head_commit;
 	commit->object.flags |= UNINTERESTING;
 	add_pending_object(&rev, &commit->object, NULL);
 
@@ -709,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_commit,
 				remoteheads->item, reversed, &result);
 		if (active_cache_changed &&
 				(write_cache(index_fd, active_cache, active_nr) ||
@@ -867,7 +868,7 @@ static int merge_trivial(void)
 
 	write_tree_trivial(result_tree);
 	printf(_("Wonderful.\n"));
-	parent->item = lookup_commit(head);
+	parent->item = head_commit;
 	parent->next = xmalloc(sizeof(*parent->next));
 	parent->next->item = remoteheads->item;
 	parent->next->next = NULL;
@@ -889,12 +890,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_commit, &parents);
 		parents = reduce_heads(parents);
 	} else {
 		struct commit_list **pptr = &parents;
 
-		pptr = &commit_list_insert(lookup_commit(head),
+		pptr = &commit_list_insert(head_commit,
 				pptr)->next;
 		for (j = remoteheads; j; j = j->next)
 			pptr = &commit_list_insert(j->item, pptr)->next;
@@ -1030,6 +1031,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		branch += 11;
 	if (is_null_sha1(head))
 		head_invalid = 1;
+	else
+		head_commit = lookup_expect_commit(head, "HEAD");
 
 	git_config(git_merge_config, NULL);
 
@@ -1203,11 +1206,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	}
 
 	if (!remoteheads->next)
-		common = get_merge_bases(lookup_commit(head),
+		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);
 	}
@@ -1292,7 +1295,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),
+			common_one = get_merge_bases(head_commit,
 				j->item, 1);
 			if (hashcmp(common_one->item->object.sha1,
 				j->item->object.sha1)) {
diff --git a/commit.c b/commit.c
index ac337c7..dc22695 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_expect_commit(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..f36c913 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_expect_commit(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 6e8f6d0..31297af 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_expect_commit(head_sha1, "HEAD");
+	struct commit *branch = lookup_expect_commit(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..f926412 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_expect_commit(sha1, "HEAD");
+	if (get_sha1("MERGE_HEAD", sha1))
 		die("--merge without MERGE_HEAD?");
+	other = lookup_expect_commit(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

Re: [PATCH v3] 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.
>
> Be easy, warn users (so broken tools can be fixed) and move on.

We do not really fix broken tools; we just fix breakages caused by them.

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

Makes sense.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index cb73857..f78b449 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -63,6 +63,7 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\
>  "Otherwise, please use 'git reset'\n");
>  
>  static unsigned char head_sha1[20];
> +static struct commit *head_commit;

I was not happy with the file-scope global head_sha1[] already, and this
makes me even less happy. Was it too much trouble to keep them local to
cmd_commit() and pass them around as arguments where necessary?  If you
pass around head_commit, is_null_sha1(head_sha1) can be replaced with a
check !head_commit so we may even be able to lose the head_sha1[] global.

And actually removing head_sha1[] is a necessary step from the correctness
point of view. The repository may have given an object name for a tag in
head_sha1[] and lookup_expect_commit() may have peeled it to a commit.
The code may want to add the "HEAD" as one of the parents of a new commit,
and head_sha1[] is no longer the place to pick that information up after
your fix. The code needs to look at head_commit->object.sha1[] instead.
Losing the use of head_sha1[] variable and forcing the code to go to
head_commit->object.sha1[] reduces the risk of such confusion.

> @@ -1028,6 +1026,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  
>  	if (get_sha1("HEAD", head_sha1))
>  		initial_commit = 1;
> +	else
> +		head_commit = lookup_expect_commit(head_sha1, "HEAD");

It may be just me, but the name feels a bit funny. The original name was
"look up" (verb) + "commit" (direct object of the verb), and what you are
doing is lookup_and_expect_commit(), but it is too long.

Perhaps lookup_commit_or_die()?

To a naïve reader of the caller, the function looks as if it would do

	struct commit *lookup_expect_commit(unsigned char *head_sha1, char *name)
        {
        	struct commit *c;
		if (get_sha1("HEAD", head_sha1) ||
	            !(c = lookup_commit(head_sha1))) die(...);
		return c;
	}

but in fact it does not use "HEAD" for anything other than error reporting
and it is the caller's responsibility to supply head_sha1[].

It is documented in the function declaration by making the memory
head_sha1 points at as a "const" (i.e. the function can't be calling
get_sha() on the refname), but it may also need a comment or two there.

> diff --git a/builtin/merge.c b/builtin/merge.c
> index 325891e..22e98e8 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -51,6 +51,7 @@ 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 struct commit* head_commit;

This is C, not C++; asterisk sticks to the variable, not type.

> @@ -1030,6 +1031,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		branch += 11;
>  	if (is_null_sha1(head))
>  		head_invalid = 1;
> +	else
> +		head_commit = lookup_expect_commit(head, "HEAD");
>  
>  	git_config(git_merge_config, NULL);
>  

The same comment as head[] vs head_commit->object.sha1[] redundancy
applies from builtin/commit.c here.

Overall, the changes to buitlin/merge.c look very nice, getting rid of the
repeated lookup_commit(head). In a separate patch after this fix gets
ready, we may want to further clean it up so that head_commit being NULL
means head_invalid, losing a redundant variable.

Re: [PATCH v3] Accept tags in HEAD or MERGE_HEAD

[Nguyen Thai Ngoc Duy]

2011/8/19 Junio C Hamano <gitster@pobox.com>:
>>  static unsigned char head_sha1[20];
>> +static struct commit *head_commit;
>
> I was not happy with the file-scope global head_sha1[] already, and this
> makes me even less happy. Was it too much trouble to keep them local to
> cmd_commit() and pass them around as arguments where necessary?  If you
> pass around head_commit, is_null_sha1(head_sha1) can be replaced with a
> check !head_commit so we may even be able to lose the head_sha1[] global.
>
> And actually removing head_sha1[] is a necessary step from the correctness
> point of view. The repository may have given an object name for a tag in
> head_sha1[] and lookup_expect_commit() may have peeled it to a commit.
> The code may want to add the "HEAD" as one of the parents of a new commit,
> and head_sha1[] is no longer the place to pick that information up after
> your fix. The code needs to look at head_commit->object.sha1[] instead.
> Losing the use of head_sha1[] variable and forcing the code to go to
> head_commit->object.sha1[] reduces the risk of such confusion.

I actually wanted to get rid of head_sha1 but it made changes bigger.
But the patch's become big already. Let me remove head_sha1[] (and
head[] in merge.c) globally, then put this fix on top.
-- 
Duy

[PATCH v4 1/4] commit: remove global variable head_sha1[]

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

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

diff --git a/builtin/commit.c b/builtin/commit.c
index cb73857..c9c4ea5 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -62,8 +62,6 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\
 "\n"
 "Otherwise, please use 'git reset'\n");
 
-static unsigned char head_sha1[20];
-
 static const char *use_message_buffer;
 static const char commit_editmsg[] = "COMMIT_EDITMSG";
 static struct lock_file index_lock; /* real index */
@@ -296,7 +294,7 @@ static void add_remove_files(struct string_list *list)
 	}
 }
 
-static void create_base_index(void)
+static void create_base_index(const unsigned char *head_sha1)
 {
 	struct tree *tree;
 	struct unpack_trees_options opts;
@@ -334,7 +332,8 @@ static void refresh_cache_or_die(int refresh_flags)
 		die_resolve_conflict("commit");
 }
 
-static char *prepare_index(int argc, const char **argv, const char *prefix, int is_status)
+static char *prepare_index(int argc, const char **argv, const char *prefix,
+			   const unsigned char *head_sha1, int is_status)
 {
 	int fd;
 	struct string_list partial;
@@ -469,7 +468,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 						(uintmax_t) getpid()),
 				       LOCK_DIE_ON_ERROR);
 
-	create_base_index();
+	create_base_index(head_sha1);
 	add_remove_files(&partial);
 	refresh_cache(REFRESH_QUIET);
 
@@ -518,11 +517,8 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 	return s->commitable;
 }
 
-static int is_a_merge(const unsigned char *sha1)
+static int is_a_merge(struct commit *commit)
 {
-	struct commit *commit = lookup_commit(sha1);
-	if (!commit || parse_commit(commit))
-		die(_("could not parse HEAD commit"));
 	return !!(commit->parents && commit->parents->next);
 }
 
@@ -627,7 +623,7 @@ static char *cut_ident_timestamp_part(char *string)
 }
 
 static int prepare_to_commit(const char *index_file, const char *prefix,
-			     struct wt_status *s,
+			     struct commit *head_commit, struct wt_status *s,
 			     struct strbuf *author_ident)
 {
 	struct stat statbuf;
@@ -848,7 +844,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 * empty due to conflict resolution, which the user should okay.
 	 */
 	if (!commitable && whence != FROM_MERGE && !allow_empty &&
-	    !(amend && is_a_merge(head_sha1))) {
+	    !(amend && is_a_merge(head_commit))) {
 		run_status(stdout, index_file, prefix, 0, s);
 		if (amend)
 			fputs(_(empty_amend_advice), stderr);
@@ -1026,9 +1022,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (!use_editor)
 		setenv("GIT_EDITOR", ":", 1);
 
-	if (get_sha1("HEAD", head_sha1))
-		initial_commit = 1;
-
 	/* Sanity check options */
 	if (amend && initial_commit)
 		die(_("You have nothing to amend."));
@@ -1102,12 +1095,12 @@ static int parse_and_validate_options(int argc, const char *argv[],
 }
 
 static int dry_run_commit(int argc, const char **argv, const char *prefix,
-			  struct wt_status *s)
+			  const unsigned char *head_sha1, struct wt_status *s)
 {
 	int commitable;
 	const char *index_file;
 
-	index_file = prepare_index(argc, argv, prefix, 1);
+	index_file = prepare_index(argc, argv, prefix, head_sha1, 1);
 	commitable = run_status(stdout, index_file, prefix, 0, s);
 	rollback_index_files();
 
@@ -1383,11 +1376,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	const char *index_file, *reflog_msg;
 	char *nl, *p;
 	unsigned char commit_sha1[20];
+	unsigned char head_sha1[20];
 	struct ref_lock *ref_lock;
 	struct commit_list *parents = NULL, **pptr = &parents;
 	struct stat statbuf;
 	int allow_fast_forward = 1;
 	struct wt_status s;
+	struct commit *head_commit;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_commit_usage, builtin_commit_options);
@@ -1396,6 +1391,14 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	git_config(git_commit_config, &s);
 	determine_whence(&s);
 
+	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"));
+	}
+
 	if (s.use_color == -1)
 		s.use_color = git_use_color_default;
 	argc = parse_and_validate_options(argc, argv, builtin_commit_usage,
@@ -1403,13 +1406,14 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	if (dry_run) {
 		if (diff_use_color_default == -1)
 			diff_use_color_default = git_use_color_default;
-		return dry_run_commit(argc, argv, prefix, &s);
+		return dry_run_commit(argc, argv, prefix, head_sha1, &s);
 	}
-	index_file = prepare_index(argc, argv, prefix, 0);
+	index_file = prepare_index(argc, argv, prefix, head_sha1, 0);
 
 	/* Set up everything for writing the commit object.  This includes
 	   running hooks, writing the trees, and interacting with the user.  */
-	if (!prepare_to_commit(index_file, prefix, &s, &author_ident)) {
+	if (!prepare_to_commit(index_file, prefix, head_commit,
+			       &s, &author_ident)) {
 		rollback_index_files();
 		return 1;
 	}
@@ -1421,15 +1425,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			reflog_msg = "commit (initial)";
 	} else if (amend) {
 		struct commit_list *c;
-		struct commit *commit;
 
 		if (!reflog_msg)
 			reflog_msg = "commit (amend)";
-		commit = lookup_commit(head_sha1);
-		if (!commit || parse_commit(commit))
-			die(_("could not parse HEAD commit"));
 
-		for (c = commit->parents; c; c = c->next)
+		for (c = head_commit->parents; c; c = c->next)
 			pptr = &commit_list_insert(c->item, pptr)->next;
 	} else if (whence == FROM_MERGE) {
 		struct strbuf m = STRBUF_INIT;
@@ -1437,7 +1437,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 		if (!reflog_msg)
 			reflog_msg = "commit (merge)";
-		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
+		pptr = &commit_list_insert(head_commit, pptr)->next;
 		fp = fopen(git_path("MERGE_HEAD"), "r");
 		if (fp == NULL)
 			die_errno(_("could not open '%s' for reading"),
@@ -1463,7 +1463,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			reflog_msg = (whence == FROM_CHERRY_PICK)
 					? "commit (cherry-pick)"
 					: "commit";
-		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
+		pptr = &commit_list_insert(head_commit, pptr)->next;
 	}
 
 	/* Finally, get the commit message */
-- 
1.7.4.74.g639db

[PATCH v4 2/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>
---
 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.4.74.g639db

Re: [PATCH v4 2/4] merge: keep stash[] a local variable

[Junio C Hamano]

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

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

Makes tons of sense; thanks.

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

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

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

diff --git a/builtin/merge.c b/builtin/merge.c
index a068660..b7260f5 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[20];
+	struct commit *head_commit = NULL;
 	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;
@@ -1030,8 +1034,11 @@ 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_invalid = 1;
+	if (!is_null_sha1(head)) {
+		head_commit = lookup_commit(head);
+		if (!head_commit)
+			die(_("could not parse HEAD"));
+	}
 
 	git_config(git_merge_config, NULL);
 
@@ -1112,12 +1119,12 @@ 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 && is_old_style_invocation(argc, argv, head)) {
 		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 +1171,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,11 +1212,10 @@ 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);
 	}
@@ -1254,7 +1260,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		if (checkout_fast_forward(head, 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)
@@ -1275,7 +1281,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			printf(_("Trying really trivial in-index merge...\n"));
 			if (!read_tree_trivial(common->item->object.sha1,
 					head, remoteheads->item->object.sha1))
-				return merge_trivial();
+				return merge_trivial(head_commit);
 			printf(_("Nope.\n"));
 		}
 	} else {
@@ -1294,8 +1300,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 +1338,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, stash);
 		}
 		if (use_strategies_nr != 1)
 			printf(_("Trying merge strategy %s...\n"),
@@ -1345,7 +1350,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 +1392,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, stash);
 		if (use_strategies_nr > 1)
 			fprintf(stderr,
 				_("No merge strategy handled the merge.\n"));
@@ -1406,14 +1412,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, 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.4.74.g639db

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

[Junio C Hamano]

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

> @@ -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[20];
> +	struct commit *head_commit = NULL;
>  	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;
> @@ -1030,8 +1034,11 @@ 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_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[]?

Also, would it be too much trouble and code churn to employ the same
strategy as my rewrite of your [1/4] and pass only head_commit around in
the call chain?

Because this is the way to set up head_commit in the first place, this
particular resolve_ref() -> is_null_sha1() chain is unavoidable and needs
to be written carefully, but after the

    !head_commit === is_null_sha1(head) === is_initial_commit

invariant is established, I suspect that it would reduce the chance of
similar mistakes in later parts of the code if it can check and use only
one argument.

[PATCH v4 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>
---
 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

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

[Junio C Hamano]

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

>  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.

That is why I suggested _removing_ head_sha1[] altogether, so that there
is only one source of information. is_initial becomes !current_head and 
head_sha1 becomes (current_head ? current_head->object.sha1 : null_sha1).

> 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.
> + */

That's de-reference, not deference ;-). You may want to be more explicit
about what kind of de-reference you are talking about.

/*
 * Get a commit object for the given sha1, unwrapping a tag object that
 * point at a commit while at it. ref_name is only used when the result 
 * is not a commit in the error message to report where we got the sha1
 * from.
 */

I actually was hoping that you would have this comment in commit.h to help
people who want to add callers of this function, not next to the
implementation.

As I said earlier, I do not think updating sha1[] here is necessary. The
caller should be updated to use c->object.sha1 instead.

> +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;
> +}

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

[Nguyen Thai Ngoc Duy]

2011/8/20 Junio C Hamano <gitster@pobox.com>:
> That's de-reference, not deference ;-). You may want to be more explicit
> about what kind of de-reference you are talking about.
>
> /*
>  * Get a commit object for the given sha1, unwrapping a tag object that
>  * point at a commit while at it. ref_name is only used when the result
>  * is not a commit in the error message to report where we got the sha1
>  * from.
>  */
>
> I actually was hoping that you would have this comment in commit.h to help
> people who want to add callers of this function, not next to the
> implementation.

OK. It's because I tend to go straight to implementation instead of
the declaration when I want to know how to use it.

> As I said earlier, I do not think updating sha1[] here is necessary. The
> caller should be updated to use c->object.sha1 instead.

The usual pattern is

get_sha1(ref, sha1);
commit = lookup_commit_or_die(sha1, ref);

From a quick look, it's very easy to assume "sha1" is safe to use
afterwards while it may be different from commit->object.sha1. I'm
tempted to make lookup_commit_or_die() resolve ref to sha1 internally,
no temporary sha1 variable will be hanging around, the pattern becomes

commit = lookup_commit_or_die(ref);

The only problem is MERGE_HEAD is not usual ref and cannot be treated this way.
-- 
Duy

Re: [PATCH v4 1/4] commit: remove global variable head_sha1[]

[Junio C Hamano]

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

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Thanks for a re-roll.

> @@ -1383,11 +1376,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	const char *index_file, *reflog_msg;
>  	char *nl, *p;
>  	unsigned char commit_sha1[20];
> +	unsigned char head_sha1[20];
>  	struct ref_lock *ref_lock;
>  	struct commit_list *parents = NULL, **pptr = &parents;
>  	struct stat statbuf;
>  	int allow_fast_forward = 1;
>  	struct wt_status s;
> +	struct commit *head_commit;

head_sha1[] is not initialized to NULs; neither is head_commit to NULL.

	cmd_commit()
        -> if it is an initial_commit, neither head_sha1[] nor head_commit
           is defined;
        -> prepare_to_commit()
           -> is_a_merge(head_commit) gets called to see if this is an
	      empty non-merge commit if --amend was passed.

Attempting to --amend an initial commit should be an error and
parse_and_validate_options() checks that condition so the above is not
possible, but it still feels wrong.

Also wouldn't these three be equivalents?

	head_commit == NULL
        is_null_sha1(head_sha1)
        initial_commit

Perhaps like this instead?

-- >8 --
Subject: commit: reduce use of redundant global variables

The file-scope global variable head_sha1[] was used to hold the object
name of the current HEAD commit (unless we are about to make an initial
commit). Also there is an independent "static int initial_commit".

Fix all the functions on the call-chain that use these two variables to
take a new "(const) struct commit *current_head" argument instead, and
replace their uses, e.g. "if (initial_commit)" becomes "if (!current_head)"
and a reference to "head_sha1" becomes "current_head->object.sha1".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/commit.c |   87 ++++++++++++++++++++++++++++--------------------------
 1 files changed, 45 insertions(+), 42 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index e1af9b1..1a65319 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -62,8 +62,6 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\
 "\n"
 "Otherwise, please use 'git reset'\n");
 
-static unsigned char head_sha1[20];
-
 static const char *use_message_buffer;
 static const char commit_editmsg[] = "COMMIT_EDITMSG";
 static struct lock_file index_lock; /* real index */
@@ -102,7 +100,7 @@ static enum {
 static char *cleanup_arg;
 
 static enum commit_whence whence;
-static int use_editor = 1, initial_commit, include_status = 1;
+static int use_editor = 1, include_status = 1;
 static int show_ignored_in_status;
 static const char *only_include_assumed;
 static struct strbuf message;
@@ -294,13 +292,13 @@ static void add_remove_files(struct string_list *list)
 	}
 }
 
-static void create_base_index(void)
+static void create_base_index(const struct commit *current_head)
 {
 	struct tree *tree;
 	struct unpack_trees_options opts;
 	struct tree_desc t;
 
-	if (initial_commit) {
+	if (!current_head) {
 		discard_cache();
 		return;
 	}
@@ -313,7 +311,7 @@ static void create_base_index(void)
 	opts.dst_index = &the_index;
 
 	opts.fn = oneway_merge;
-	tree = parse_tree_indirect(head_sha1);
+	tree = parse_tree_indirect(current_head->object.sha1);
 	if (!tree)
 		die(_("failed to unpack HEAD tree object"));
 	parse_tree(tree);
@@ -332,7 +330,8 @@ static void refresh_cache_or_die(int refresh_flags)
 		die_resolve_conflict("commit");
 }
 
-static char *prepare_index(int argc, const char **argv, const char *prefix, int is_status)
+static char *prepare_index(int argc, const char **argv, const char *prefix,
+			   const struct commit *current_head, int is_status)
 {
 	int fd;
 	struct string_list partial;
@@ -448,7 +447,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 
 	memset(&partial, 0, sizeof(partial));
 	partial.strdup_strings = 1;
-	if (list_paths(&partial, initial_commit ? NULL : "HEAD", prefix, pathspec))
+	if (list_paths(&partial, !current_head ? NULL : "HEAD", prefix, pathspec))
 		exit(1);
 
 	discard_cache();
@@ -467,7 +466,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 						(uintmax_t) getpid()),
 				       LOCK_DIE_ON_ERROR);
 
-	create_base_index();
+	create_base_index(current_head);
 	add_remove_files(&partial);
 	refresh_cache(REFRESH_QUIET);
 
@@ -516,12 +515,9 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 	return s->commitable;
 }
 
-static int is_a_merge(const unsigned char *sha1)
+static int is_a_merge(const struct commit *current_head)
 {
-	struct commit *commit = lookup_commit(sha1);
-	if (!commit || parse_commit(commit))
-		die(_("could not parse HEAD commit"));
-	return !!(commit->parents && commit->parents->next);
+	return !!(current_head->parents && current_head->parents->next);
 }
 
 static const char sign_off_header[] = "Signed-off-by: ";
@@ -625,6 +621,7 @@ static char *cut_ident_timestamp_part(char *string)
 }
 
 static int prepare_to_commit(const char *index_file, const char *prefix,
+			     struct commit *current_head,
 			     struct wt_status *s,
 			     struct strbuf *author_ident)
 {
@@ -846,7 +843,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 * empty due to conflict resolution, which the user should okay.
 	 */
 	if (!commitable && whence != FROM_MERGE && !allow_empty &&
-	    !(amend && is_a_merge(head_sha1))) {
+	    !(amend && is_a_merge(current_head))) {
 		run_status(stdout, index_file, prefix, 0, s);
 		if (amend)
 			fputs(_(empty_amend_advice), stderr);
@@ -1004,6 +1001,7 @@ static const char *read_commit_message(const char *name)
 static int parse_and_validate_options(int argc, const char *argv[],
 				      const char * const usage[],
 				      const char *prefix,
+				      struct commit *current_head,
 				      struct wt_status *s)
 {
 	int f = 0;
@@ -1024,11 +1022,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (!use_editor)
 		setenv("GIT_EDITOR", ":", 1);
 
-	if (get_sha1("HEAD", head_sha1))
-		initial_commit = 1;
-
 	/* Sanity check options */
-	if (amend && initial_commit)
+	if (amend && !current_head)
 		die(_("You have nothing to amend."));
 	if (amend && whence != FROM_COMMIT)
 		die(_("You are in the middle of a %s -- cannot amend."), whence_s());
@@ -1100,12 +1095,12 @@ static int parse_and_validate_options(int argc, const char *argv[],
 }
 
 static int dry_run_commit(int argc, const char **argv, const char *prefix,
-			  struct wt_status *s)
+			  const struct commit *current_head, struct wt_status *s)
 {
 	int commitable;
 	const char *index_file;
 
-	index_file = prepare_index(argc, argv, prefix, 1);
+	index_file = prepare_index(argc, argv, prefix, current_head, 1);
 	commitable = run_status(stdout, index_file, prefix, 0, s);
 	rollback_index_files();
 
@@ -1258,7 +1253,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
-static void print_summary(const char *prefix, const unsigned char *sha1)
+static void print_summary(const char *prefix, const unsigned char *sha1,
+			  int initial_commit)
 {
 	struct rev_info rev;
 	struct commit *commit;
@@ -1380,12 +1376,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	struct strbuf author_ident = STRBUF_INIT;
 	const char *index_file, *reflog_msg;
 	char *nl, *p;
-	unsigned char commit_sha1[20];
+	unsigned char sha1[20];
 	struct ref_lock *ref_lock;
 	struct commit_list *parents = NULL, **pptr = &parents;
 	struct stat statbuf;
 	int allow_fast_forward = 1;
 	struct wt_status s;
+	struct commit *current_head = NULL;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_commit_usage, builtin_commit_options);
@@ -1396,38 +1393,41 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	if (s.use_color == -1)
 		s.use_color = git_use_color_default;
+	if (get_sha1("HEAD", sha1))
+		current_head = NULL;
+	else {
+		current_head = lookup_commit(sha1);
+		if (!current_head || parse_commit(current_head))
+			die(_("could not parse HEAD commit"));
+	}
 	argc = parse_and_validate_options(argc, argv, builtin_commit_usage,
-					  prefix, &s);
+					  prefix, current_head, &s);
 	if (dry_run) {
 		if (diff_use_color_default == -1)
 			diff_use_color_default = git_use_color_default;
-		return dry_run_commit(argc, argv, prefix, &s);
+		return dry_run_commit(argc, argv, prefix, current_head, &s);
 	}
-	index_file = prepare_index(argc, argv, prefix, 0);
+	index_file = prepare_index(argc, argv, prefix, current_head, 0);
 
 	/* Set up everything for writing the commit object.  This includes
 	   running hooks, writing the trees, and interacting with the user.  */
-	if (!prepare_to_commit(index_file, prefix, &s, &author_ident)) {
+	if (!prepare_to_commit(index_file, prefix,
+			       current_head, &s, &author_ident)) {
 		rollback_index_files();
 		return 1;
 	}
 
 	/* Determine parents */
 	reflog_msg = getenv("GIT_REFLOG_ACTION");
-	if (initial_commit) {
+	if (!current_head) {
 		if (!reflog_msg)
 			reflog_msg = "commit (initial)";
 	} else if (amend) {
 		struct commit_list *c;
-		struct commit *commit;
 
 		if (!reflog_msg)
 			reflog_msg = "commit (amend)";
-		commit = lookup_commit(head_sha1);
-		if (!commit || parse_commit(commit))
-			die(_("could not parse HEAD commit"));
-
-		for (c = commit->parents; c; c = c->next)
+		for (c = current_head->parents; c; c = c->next)
 			pptr = &commit_list_insert(c->item, pptr)->next;
 	} else if (whence == FROM_MERGE) {
 		struct strbuf m = STRBUF_INIT;
@@ -1435,7 +1435,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 		if (!reflog_msg)
 			reflog_msg = "commit (merge)";
-		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
+		pptr = &commit_list_insert(current_head, pptr)->next;
 		fp = fopen(git_path("MERGE_HEAD"), "r");
 		if (fp == NULL)
 			die_errno(_("could not open '%s' for reading"),
@@ -1461,7 +1461,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			reflog_msg = (whence == FROM_CHERRY_PICK)
 					? "commit (cherry-pick)"
 					: "commit";
-		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
+		pptr = &commit_list_insert(current_head, pptr)->next;
 	}
 
 	/* Finally, get the commit message */
@@ -1487,7 +1487,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		exit(1);
 	}
 
-	if (commit_tree(sb.buf, active_cache_tree->sha1, parents, commit_sha1,
+	if (commit_tree(sb.buf, active_cache_tree->sha1, parents, sha1,
 			author_ident.buf)) {
 		rollback_index_files();
 		die(_("failed to write commit object"));
@@ -1495,7 +1495,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	strbuf_release(&author_ident);
 
 	ref_lock = lock_any_ref_for_update("HEAD",
-					   initial_commit ? NULL : head_sha1,
+					   !current_head
+					   ? NULL
+					   : current_head->object.sha1,
 					   0);
 
 	nl = strchr(sb.buf, '\n');
@@ -1510,7 +1512,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		rollback_index_files();
 		die(_("cannot lock HEAD ref"));
 	}
-	if (write_ref_sha1(ref_lock, commit_sha1, sb.buf) < 0) {
+	if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
 		rollback_index_files();
 		die(_("cannot update HEAD ref"));
 	}
@@ -1532,13 +1534,14 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		struct notes_rewrite_cfg *cfg;
 		cfg = init_copy_notes_for_rewrite("amend");
 		if (cfg) {
-			copy_note_for_rewrite(cfg, head_sha1, commit_sha1);
+			/* we are amending, so current_head is not NULL */
+			copy_note_for_rewrite(cfg, current_head->object.sha1, sha1);
 			finish_copy_notes_for_rewrite(cfg);
 		}
-		run_rewrite_hook(head_sha1, commit_sha1);
+		run_rewrite_hook(current_head->object.sha1, sha1);
 	}
 	if (!quiet)
-		print_summary(prefix, commit_sha1);
+		print_summary(prefix, sha1, !current_head);
 
 	return 0;
 }

Re: [PATCH v4 1/4] commit: remove global variable head_sha1[]

[Nguyen Thai Ngoc Duy]

2011/8/20 Junio C Hamano <gitster@pobox.com>:
> Also wouldn't these three be equivalents?
>
>        head_commit == NULL
>        is_null_sha1(head_sha1)
>        initial_commit

Right.

> Perhaps like this instead?

Yup. Looks good.
-- 
Duy