Re: Ambiguous reference weirdness

On Wed, Feb 22, 2012 at 2:00 AM, Jeff King [off-list ref] wrote:

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

quoted

I accidentally ran into this today:
    $ git cherry-pick 1147
    fatal: BUG: expected exactly one commit from walk

git log shows no output:
    $ git log 1147

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

1147 was a typo.  It was a Gerritt changeset ID I forgot to expand.
When I type "git cherry-pick 1147<TAB>", autocompletion expands this
for me to "git cherry-pick origin/changes/47/1147/1".  Except in this
case I misfired the TAB and got the weird "BUG:" report.  But I didn't
see the same problem with other invalid refs, so I went searching for
the variants and to see what caused it.

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

All of these examples were run in current git.git, so you can try them
yourself if needed.  But I did figure out that 1147 resolves to a
blob, and that's apparently the difference between these three:

$ git cherry-pick 1147
fatal: BUG: expected exactly one commit from walk

$ git cherry-pick 1146
error: short SHA1 1146 is ambiguous.
error: short SHA1 1146 is ambiguous.
fatal: ambiguous argument '1146': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions

$ git cherry-pick 114333
fatal: ambiguous argument '114333': unknown revision or path not in
the working tree.
Use '--' to separate paths from revisions

I consider the first two responses to be UI bugs.   The second one is
minor (the twice-reported error message), and the first one is pretty
rude.   I would expect all three to report the same conclusion,
"fatal: ambiguous argument 'XXXXX': unknown revision or path not in
the working tree."  But the first one doesn't.

 $ 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

Thanks.  I'm not familiar with the {tree} syntax -- in fact I'd like
to find a dictionary for all the reference spelling variants -- but
this is elucidating.

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.

Bug is too strong a word in one sense, but from the user perspective I
consider this "horrible message" a bug.

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 think you're right about this.  I tried log as an afterthought and
was surprised by the varying results.

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

quoted

$git log 114
fatal: ambiguous argument '114': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions

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).

quoted

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

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

quoted

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

Yes, I understand this.  This was a typo and it was ambiguous.  But
shouldn't we tell the user the same thing when encountering the same
failure?

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.

quoted

$ git checkout 1146
error: short SHA1 1146 is ambiguous.
error: pathspec '1146' did not match any file(s) known to git.

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.

This one I like.  I only included it to demonstrate the different
error messages git produces.

quoted

$ git merge 114
fatal: '114' does not point to a commit

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:

quoted

$ git merge 1147
error: 1147: expected commit type, but the object dereferences to blob type
fatal: '1147' does not point to a commit

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).

quoted

$ git cherry-pick 114
fatal: ambiguous argument '114': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions
[...]
$ git cherry-pick 1147
fatal: BUG: expected exactly one commit from walk

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

Good point.  I missed that one completely.

(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).

quoted

[more examples]

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

quoted

I can understand some of the inconsistent error reporting (checkout
may expect filenames, but cherry-pick and merge do not).  But this
seems too varied to me.

And the first two look like bugs.

Any comments or suggestions?

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

Yes, I agree.  I only meant to complain about the error messages, not
the results.  Thanks for the discussion.  I'll try to look for where
these come from and see if they can be improved within reason.

Phil