Re: rev-list and "ambiguous" IDs

From: Jeff King <peff@peff.net>
Date: Nov 18, 2019, 12:03 UTC
Message-ID: <20191118120315.GB12766@sigill.intra.peff.net>
In-Reply-To: <xmqqmucw4h4n.fsf@gitster-ct.c.googlers.com>

On Sat, Nov 16, 2019 at 12:47:20PM +0900, Junio C Hamano wrote:

Show 26 quoted lines

> > I would have expected that git log did just tell me that it could not
> > find something commitish, instead it told me that there are multiple
> > candidates, all of them being no commit.
> 
> With this, I 100% agree with.   The latter should instead say
> 
>     $ git log abcd2 [--]
>     error: bad revision 'abcd2'
> 
> just like the case where no object has abcd2 as prefix.
> 
> When we ask for commit-ish or any specific type in general, there
> are a few possible cases.
> 
>  - There is only one such object that has the prefix and is
>    compatible with the type.  We handle this correctly---yield that
>    object and do not complain about ambiguity.
> 
>  - There are two or more such objects, or there is no such object.
>    We show all objects that share the prefix, regardless of the
>    type, which is way suboptimal.
> 
> An improvement can be localized to sha1-name.c::get_short_oid(), I
> would think.  We know what type we want (e.g. GET_OID_COMMITTISH)
> in the function, so we should be able to teach collect_ambiguous() 
> to discard an object with the given prefix but of a wrong type.

I think that changes the meaning of GET_OID_COMMITTISH, though. Right now it means "if disambiguating, prefer a committish", but not "I can only accept a commit". So we would still happily return an unambiguous object that does not match that type. And that is why "git -c core.disambiguate=committish show $short_blob" works, for example.

If you adjust your first case above to "only one such object...and the type does not matter" then I think it is OK. I.e., the logic in get_short_oid() becomes:

  - if there is only one, return it

  - if there is more than one, and only one matches the disambiguator,
    return it

  - otherwise, _do not_ print the ambiguous list, and return an error
    (and no object at all), letting the caller complain

where the third part is the new behavior. I think that helps in some ways (you do not get a list of non-commits for a context that only takes commits). But it might also hurt, because it gives the user less information. E.g., imagine the user feeds a short sha1 that they know to be a blob to a command expecting a committish and is told "no, that short sha1 does not exist", even though the actual problem is that there are two such blobs.

I think it's a bit simpler for a command which doesn't expect non-commits at all, like "git log". But it would need to communicate that to get_short_oid() with more than just GET_OID_COMMITTISH, so that the latter can tell it apart from contexts which merely prefer a committish.

I'm also not entirely sure that even that case doesn't suffer from telling the user less information. If I say "git log 1234" knowing that "1234" is a blob, that's a mistake. But Git may guide me in correcting that mistake by saying "yes, we know about 1234, but it's ambiguous" rather than "1234 is not something we know about".

Perhaps a simple fix would just be for get_short_oid()'s error message to mention the disambiguation rule. E.g., something like:

   $ git show abcd2
   error: short SHA1 abcd2 is ambiguous
   hint: We would have preferred a commit or tag pointing to a commit,
   hint: but none were found. The candidates are:
   hint:   abcd22f55e tree
   hint:   abcd238df0 tree
   hint:   abcd2b1cc8 blob

or

  $ git show abcd2
  error: short SHA1 abcd2 is ambiguous
  hint: We preferred a commit or tag pointing to a commit to other
  hint: object types, but two candidates were found:
  hint:   abcd22f55e commit
  hint:   abcd238df0 commit
  hint:   abcd2b1cc8 blob

(optionally the second one could even not mention the blob, though I think with the lead-in sentence it's OK).

The verbiage there isn't great (I was trying to avoid the jargon "committish"), but hopefully you get the point.

-Peff