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

From: Jeff King <peff@peff.net>
Date: Jul 3, 2012, 07:19 UTC
Message-ID: <20120703071940.GB16679@sigill.intra.peff.net>
In-Reply-To: <1341268449-27801-1-git-send-email-gitster@pobox.com>

On Mon, Jul 02, 2012 at 03:33:51PM -0700, Junio C Hamano wrote:

Show 15 quoted lines

> This is take 4.  The earlier rounds were $gmane/200165, 200387, and
> 200506.
> 
> Compared to the previous round, it has more patches in the clean-up
> phase.  Most notably, patch 03/18 gets rid of get_sha1_with_mode_1()
> and replaces the only external caller of it with a call to a more
> straightforward die_on_misspelt_object_name().  The test suite added
> by patch 12/18 has more patterns that we can potentially improve on.
> 
> The disambiguation logic can now be asked to pick only committish,
> which can be used in places like "git commit -C deadbeef".  It also
> knows that A and B in "git log A..B" can only be committishes.
> 
> Adding support for treeish, if anybody is tempted to do so, should
> now be pretty straightforward.

I finally took a moment to read over this series. I really like the cleanups in the early part (reducing the number of get_sha1_* functions especially), and the design of the disambiguate_state code is nice and clean. I didn't notice anything wrong while reading the patches themselves.

I do see one slight regression. If you feed an unambiguous partial sha1 and it does not match the hint, we still select it, which is good. So you still get:

  $ git show 000379^{commit}
  error: 000379^{commit}: expected commit type, but the object dereferences to blob type
  error: 000379^{commit}: expected commit type, but the object dereferences to blob type
  fatal: ambiguous argument '000379^{commit}': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions

So far so good (aside from the duplicated error message, but that is unrelated). However, if you feed a partial sha1 for which there are multiple matches, none of which satisfy the disambiguation hint, then we used to say "short SHA1 is ambiguous", and now we don't.

  [before]
  $ git show 0003
  error: short SHA1 0003 is ambiguous.
  error: short SHA1 0003 is ambiguous.
  fatal: ambiguous argument '0003': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions

  $ git show 0003^{commit}
  error: short SHA1 0003 is ambiguous.
  error: short SHA1 0003 is ambiguous.
  fatal: ambiguous argument '0003^{commit}': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions

  [after]
  $ git show 0003
  error: short SHA1 0003 is ambiguous.
  error: short SHA1 0003 is ambiguous.
  fatal: ambiguous argument '0003': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions

  $ git show 0003^{commit}
  fatal: ambiguous argument '0003^{commit}': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions

I think the code is right not to return an object at all (since it is ambiguous), but it might be helpful to say "your sha1 was ambiguous, but none of the possibilities matched our criteria" or similar. Otherwise the error message looks exactly like the "there is no such object at all" case. I don't think this is a huge problem, but I can see it being surprising if somebody is trying to debug an issue where they expect an object to be of a different type.

Other than that, the series looks good to me.

-Peff