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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 18, 2011, 18:54 UTC
Message-ID: <7vd3g2a431.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1313674994-22902-1-git-send-email-pclouds@gmail.com>

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

Show 9 quoted lines

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

Show 9 quoted lines

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

Show 6 quoted lines

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

Show 9 quoted lines

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

Show 9 quoted lines

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