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

[Junio C Hamano <gitster@pobox.com>]

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