On Tue, Jul 03, 2012 at 11:41:42AM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html