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

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Aug 20, 2011, 16:37 UTC
Message-ID: <CACsJy8CLb-PrMdt05HEV-sHWwWbsu9MaC7-khAoT43O8vfkSDg@mail.gmail.com>
In-Reply-To: <7vaab52m2r.fsf@alter.siamese.dyndns.org>

2011/8/20 Junio C Hamano <gitster@pobox.com>:

Show 13 quoted lines

> 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