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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 17, 2011, 17:59 UTC
Message-ID: <7vy5yrex70.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1313545369-7096-1-git-send-email-pclouds@gmail.com>

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?

Show 12 quoted lines

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

Show 13 quoted lines

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