Re: [PATCH v4 00/18] Extending the shelf-life of "git describe" output

From: Jeff King <peff@peff.net>
Date: Jul 3, 2012, 19:34 UTC
Message-ID: <20120703193410.GA20503@sigill.intra.peff.net>
In-Reply-To: <7vy5n0vg9l.fsf@alter.siamese.dyndns.org>

On Tue, Jul 03, 2012 at 11:41:42AM -0700, Junio C Hamano wrote:

Show 5 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > It looks right to me (and certainly fixes the behavior I mentioned).
> 
> OK, I have further updates on the topic; pushed out to 'pu'.

Looks reasonable to me. One thing I wondered about is in:

Show 15 quoted lines

> static int get_short_sha1(const char *name, int len, unsigned char *sha1,
>			  unsigned flags)
> {
> [...]
>	memset(&ds, 0, sizeof(ds));
>	if (flags & GET_SHA1_COMMIT)
>		ds.fn = disambiguate_commit_only;
>	else if (flags & GET_SHA1_COMMITTISH)
>		ds.fn = disambiguate_committish_only;
>	else if (flags & GET_SHA1_TREE)
>		ds.fn = disambiguate_tree_only;
>	else if (flags & GET_SHA1_TREEISH)
>		ds.fn = disambiguate_treeish_only;
>	else if (flags & GET_SHA1_BLOB)
>		ds.fn = disambiguate_blob_only;

What happens if I set multiple flags? One or more of them will be ignored (you _could_ try to establish a hierarchy, for example that TREEISH is a superset of COMMITISH, but that could not handle the *_only cases, which really are mutually exclusive). IOW, I think these are not really flags but rather enum elements.

It is probably an OK trade-off since we are also stuffing true flags like GET_SHA1_QUIETLY in the same field, and we don't want to make the parameter list too unwieldy by splitting out the enum. But it might be worth throwing a comment into cache.h that a certain set of the flags are mutually exclusive. Or I guess you could mask off that part and make sure only one bit was set, which would catch the error at run-time. But I think a comment is probably sufficient.

-Peff