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

[Jeff King <peff@peff.net>]

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

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

> 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