Re: Ambiguous reference weirdness

On Tue, Feb 21, 2012 at 08:46:24PM -0500, Phil Hord wrote:

What is 1147? Is it supposed to be a partial sha1, or is it a ref you have?

Have you looked at the object that it resolves to? I suspect it is the partial sha1 of a non-commit object. E.g.:

  $ git cat-file -t HEAD^{tree}
  tree
  $ git cherry-pick HEAD^{tree}
  fatal: BUG: expected exactly one commit from walk
  $ git log HEAD^{tree} | wc -l
  0

Both cases have a similar source: they feed the arguments to the revision walking machinery, which of course finds no actual revisions to walk.

In the cherry-pick case, the code is checking the right thing, but the message is horrible. It is not a bug, but merely unexpected input, and it should provide a usage message.

In the log case, we totally ignore any pending non-revision arguments. So it is correct to produce no output (there is nothing to show, which is not unusual in itself; many queries end up producing empty output). But we should probably notice that there are pending objects left over and produce some kind of diagnostic.

I've reordered some of your example commands below to fit the flow of my explanation better.

Right. I think we require at least 4 characters in a partial sha1, so we don't treat that as a possibility. So we are left guessing whether you mean to do:

  git log 114 --

or

  git log HEAD -- 114

since it exists as neither a revision nor a path, and the error message reflects that (the first one is an error, as there is no such revision. The second is a correct query, though one that does not produce any results).

I think checkout has the same "is this a path or a revision" ambiguity to resolve. But rather than be explicit that you might have meant "114" as a tree, the error message assumes you meant a path. That might be worth improving, similar to the above example.

Again, you can disambiguate with:

  $ git checkout -- 1147
  error: pathspec '1147' did not match any file(s) known to git.

  $ git checkout 1147 --
  fatal: reference is not a tree: 1147

In this case the name does resolve to an object, so we try to use it as such (even though we later find that it is useless for the operation). We _could_ realize that it is not a tree and disambiguate to:

  $ git checkout -- 1147

but the current rule is at least consistent and simple.

The sha1 is ambiguous, and therefore it does not resolve to anything. So you get the same case as "git checkout 1147", but with the extra ambiguity warning.

It might be nice for this error message to be split into two cases:

  1. the name does not resolve _at all_ (i.e., you made a typo)

  2. the name does resolve to something, but it is not a commit

In the latter case, we actually do get an extra error message from elsewhere in the code:

But in case 1, it's not clear which is which (maybe even rewording it as "114 cannot be resolved as a commit" would be less confusing).

This is the "does not resolve" versus "is not actually a commit". In the first case, though, I wonder if the error message is accurate. I'm not sure if you can do "git cherry-pick <rev> -- <paths>", so the error message is misleading (if anything, I would expect it to limit the revision walk, but trying "git cherry-pick HEAD -- 114" seems to still complain about the absence of 114).

These are all variants that hopefully make sense in light of the explanations above.

I think the outcomes are all working as intended, but the error messages could stand to be improved.

-Peff