From: Nguyen Thai Ngoc Duy Date: Sat, 20 Aug 2011 16:37:11 GMT Subject: Re: [PATCH v4 4/4] Accept tags in HEAD or MERGE_HEAD Message-ID: In-Reply-To: <7vaab52m2r.fsf@alter.siamese.dyndns.org> 2011/8/20 Junio C Hamano : > 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