{"thread":{"id":"29689","subject":"Ambiguous reference weirdness","startedAt":"2012-02-22T01:46:24Z","lastAt":"2012-02-22T20:38:21Z","messageCount":4,"participants":["Phil Hord","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"185135","messageId":"CABURp0oAw7cvU7cwCZOtvqZ_oa0hDPsE_0Lm3kR1ctdNuxU3hg@mail.gmail.com","threadId":"29689","inReplyTo":null,"subject":"Ambiguous reference weirdness","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2012-02-22T01:46:24Z","receivedAt":"2012-02-22T01:46:24Z","isPatch":false,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"I accidentally ran into this today:\n    $ git cherry-pick 1147\n    fatal: BUG: expected exactly one commit from walk\n\ngit log shows no output:\n    $ git log 1147\n\nNeither of these are very helpful or reassuring.  I tried a few things\nbut I haven't looked the code yet.  I found lots of inconsistencies\nalong the way.\n\n$ git clone https://github.com/git/git\n$ cd git\n\n$git log 114\nfatal: ambiguous argument '114': unknown revision or path not in the\nworking tree.\nUse '--' to separate paths from revisions\n\n$ git checkout 114\nerror: pathspec '114' did not match any file(s) known to git.\n$ git merge 114\nfatal: '114' does not point to a commit\n$ git cherry-pick 114\nfatal: ambiguous argument '114': unknown revision or path not in the\nworking tree.\nUse '--' to separate paths from revisions\n\n$ git checkout 1147\nfatal: reference is not a tree: 1147\n$ git merge 1147\nerror: 1147: expected commit type, but the object dereferences to blob type\nfatal: '1147' does not point to a commit\n$ git cherry-pick 1147\nfatal: BUG: expected exactly one commit from walk\n$ git log 1147\n\n$ git checkout 1146\nerror: short SHA1 1146 is ambiguous.\nerror: pathspec '1146' did not match any file(s) known to git.\n$ git merge 1146\nerror: short SHA1 1146 is ambiguous.\nfatal: '1146' does not point to a commit\n$ git cherry-pick 1146\nerror: short SHA1 1146 is ambiguous.\nerror: short SHA1 1146 is ambiguous.\nfatal: ambiguous argument '1146': unknown revision or path not in the\nworking tree.\nUse '--' to separate paths from revisions\n$ git log 1146\nerror: short SHA1 1146 is ambiguous.\nerror: short SHA1 1146 is ambiguous.\nfatal: ambiguous argument '1146': unknown revision or path not in the\nworking tree.\nUse '--' to separate paths from revisions\n\nI can understand some of the inconsistent error reporting (checkout\nmay expect filenames, but cherry-pick and merge do not).  But this\nseems too varied to me.\n\nAnd the first two look like bugs.\n\nAny comments or suggestions?\n\nPhil\n"},{"id":"185150","messageId":"20120222070034.GA17015@sigill.intra.peff.net","threadId":"29689","inReplyTo":"CABURp0oAw7cvU7cwCZOtvqZ_oa0hDPsE_0Lm3kR1ctdNuxU3hg@mail.gmail.com","subject":"Re: Ambiguous reference weirdness","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-22T07:00:34Z","receivedAt":"2012-02-22T07:00:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 21, 2012 at 08:46:24PM -0500, Phil Hord wrote:\n\n> I accidentally ran into this today:\n>     $ git cherry-pick 1147\n>     fatal: BUG: expected exactly one commit from walk\n> \n> git log shows no output:\n>     $ git log 1147\n\nWhat is 1147? Is it supposed to be a partial sha1, or is it a ref you\nhave?\n\nHave you looked at the object that it resolves to? I suspect it is the\npartial sha1 of a non-commit object. E.g.:\n\n  $ git cat-file -t HEAD^{tree}\n  tree\n  $ git cherry-pick HEAD^{tree}\n  fatal: BUG: expected exactly one commit from walk\n  $ git log HEAD^{tree} | wc -l\n  0\n\nBoth cases have a similar source: they feed the arguments to the\nrevision walking machinery, which of course finds no actual revisions to\nwalk.\n\nIn the cherry-pick case, the code is checking the right thing, but the\nmessage is horrible. It is not a bug, but merely unexpected input, and\nit should provide a usage message.\n\nIn the log case, we totally ignore any pending non-revision arguments.\nSo it is correct to produce no output (there is nothing to show, which\nis not unusual in itself; many queries end up producing empty output).\nBut we should probably notice that there are pending objects left over\nand produce some kind of diagnostic.\n\nI've reordered some of your example commands below to fit the flow of my\nexplanation better.\n\n> $git log 114\n> fatal: ambiguous argument '114': unknown revision or path not in the\n> working tree.\n> Use '--' to separate paths from revisions\n\nRight. I think we require at least 4 characters in a partial sha1, so we\ndon't treat that as a possibility. So we are left guessing whether you\nmean to do:\n\n  git log 114 --\n\nor\n\n  git log HEAD -- 114\n\nsince it exists as neither a revision nor a path, and the error message\nreflects that (the first one is an error, as there is no such revision.\nThe second is a correct query, though one that does not produce any\nresults).\n\n> $ git checkout 114\n> error: pathspec '114' did not match any file(s) known to git.\n\nI think checkout has the same \"is this a path or a revision\" ambiguity\nto resolve. But rather than be explicit that you might have meant \"114\"\nas a tree, the error message assumes you meant a path. That might be\nworth improving, similar to the above example.\n\nAgain, you can disambiguate with:\n\n  $ git checkout -- 1147\n  error: pathspec '1147' did not match any file(s) known to git.\n\n  $ git checkout 1147 --\n  fatal: reference is not a tree: 1147\n\n> $ git checkout 1147\n> fatal: reference is not a tree: 1147\n\nIn this case the name does resolve to an object, so we try to use it as\nsuch (even though we later find that it is useless for the operation).\nWe _could_ realize that it is not a tree and disambiguate to:\n\n  $ git checkout -- 1147\n\nbut the current rule is at least consistent and simple.\n\n> $ git checkout 1146\n> error: short SHA1 1146 is ambiguous.\n> error: pathspec '1146' did not match any file(s) known to git.\n\nThe sha1 is ambiguous, and therefore it does not resolve to anything. So\nyou get the same case as \"git checkout 1147\", but with the extra\nambiguity warning.\n\n> $ git merge 114\n> fatal: '114' does not point to a commit\n\nIt might be nice for this error message to be split into two cases:\n\n  1. the name does not resolve _at all_ (i.e., you made a typo)\n\n  2. the name does resolve to something, but it is not a commit\n\nIn the latter case, we actually do get an extra error message from\nelsewhere in the code:\n\n> $ git merge 1147\n> error: 1147: expected commit type, but the object dereferences to blob type\n> fatal: '1147' does not point to a commit\n\nBut in case 1, it's not clear which is which (maybe even rewording it as\n\"114 cannot be resolved as a commit\" would be less confusing).\n\n> $ git cherry-pick 114\n> fatal: ambiguous argument '114': unknown revision or path not in the\n> working tree.\n> Use '--' to separate paths from revisions\n> [...]\n> $ git cherry-pick 1147\n> fatal: BUG: expected exactly one commit from walk\n\nThis is the \"does not resolve\" versus \"is not actually a commit\". In the\nfirst case, though, I wonder if the error message is accurate. I'm not\nsure if you can do \"git cherry-pick <rev> -- <paths>\", so the error\nmessage is misleading (if anything, I would expect it to limit the\nrevision walk, but trying \"git cherry-pick HEAD -- 114\" seems to still\ncomplain about the absence of 114).\n\n> [more examples]\n\nThese are all variants that hopefully make sense in light of the\nexplanations above.\n\n> I can understand some of the inconsistent error reporting (checkout\n> may expect filenames, but cherry-pick and merge do not).  But this\n> seems too varied to me.\n> \n> And the first two look like bugs.\n> \n> Any comments or suggestions?\n\nI think the outcomes are all working as intended, but the error messages\ncould stand to be improved.\n\n-Peff\n"},{"id":"185176","messageId":"CABURp0r+3gOQ3rq_ubv=uEoU=XmtYs=etfT4W2Lb9M0LBrikWg@mail.gmail.com","threadId":"29689","inReplyTo":"20120222070034.GA17015@sigill.intra.peff.net","subject":"Re: Ambiguous reference weirdness","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2012-02-22T14:48:23Z","receivedAt":"2012-02-22T14:48:23Z","isPatch":false,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Wed, Feb 22, 2012 at 2:00 AM, Jeff King <peff@peff.net> wrote:\n> On Tue, Feb 21, 2012 at 08:46:24PM -0500, Phil Hord wrote:\n>\n>> I accidentally ran into this today:\n>>     $ git cherry-pick 1147\n>>     fatal: BUG: expected exactly one commit from walk\n>>\n>> git log shows no output:\n>>     $ git log 1147\n>\n> What is 1147? Is it supposed to be a partial sha1, or is it a ref you\n> have?\n\n1147 was a typo.  It was a Gerritt changeset ID I forgot to expand.\nWhen I type \"git cherry-pick 1147<TAB>\", autocompletion expands this\nfor me to \"git cherry-pick origin/changes/47/1147/1\".  Except in this\ncase I misfired the TAB and got the weird \"BUG:\" report.  But I didn't\nsee the same problem with other invalid refs, so I went searching for\nthe variants and to see what caused it.\n\n> Have you looked at the object that it resolves to? I suspect it is the\n> partial sha1 of a non-commit object. E.g.:\n\nAll of these examples were run in current git.git, so you can try them\nyourself if needed.  But I did figure out that 1147 resolves to a\nblob, and that's apparently the difference between these three:\n\n$ git cherry-pick 1147\nfatal: BUG: expected exactly one commit from walk\n\n$ git cherry-pick 1146\nerror: short SHA1 1146 is ambiguous.\nerror: short SHA1 1146 is ambiguous.\nfatal: ambiguous argument '1146': unknown revision or path not in the\nworking tree.\nUse '--' to separate paths from revisions\n\n$ git cherry-pick 114333\nfatal: ambiguous argument '114333': unknown revision or path not in\nthe working tree.\nUse '--' to separate paths from revisions\n\nI consider the first two responses to be UI bugs.   The second one is\nminor (the twice-reported error message), and the first one is pretty\nrude.   I would expect all three to report the same conclusion,\n\"fatal: ambiguous argument 'XXXXX': unknown revision or path not in\nthe working tree.\"  But the first one doesn't.\n\n\n\n>  $ git cat-file -t HEAD^{tree}\n>  tree\n>  $ git cherry-pick HEAD^{tree}\n>  fatal: BUG: expected exactly one commit from walk\n>  $ git log HEAD^{tree} | wc -l\n>  0\n\nThanks.  I'm not familiar with the {tree} syntax -- in fact I'd like\nto find a dictionary for all the reference spelling variants -- but\nthis is elucidating.\n\n> In the cherry-pick case, the code is checking the right thing, but the\n> message is horrible. It is not a bug, but merely unexpected input, and\n> it should provide a usage message.\n\nBug is too strong a word in one sense, but from the user perspective I\nconsider this \"horrible message\" a bug.\n\n> In the log case, we totally ignore any pending non-revision arguments.\n> So it is correct to produce no output (there is nothing to show, which\n> is not unusual in itself; many queries end up producing empty output).\n> But we should probably notice that there are pending objects left over\n> and produce some kind of diagnostic.\n\nI think you're right about this.  I tried log as an afterthought and\nwas surprised by the varying results.\n\n> I've reordered some of your example commands below to fit the flow of my\n> explanation better.\n>\n>> $git log 114\n>> fatal: ambiguous argument '114': unknown revision or path not in the\n>> working tree.\n>> Use '--' to separate paths from revisions\n>\n> Right. I think we require at least 4 characters in a partial sha1, so we\n> don't treat that as a possibility. So we are left guessing whether you\n> mean to do:\n>\n>  git log 114 --\n>\n> or\n>\n>  git log HEAD -- 114\n>\n> since it exists as neither a revision nor a path, and the error message\n> reflects that (the first one is an error, as there is no such revision.\n> The second is a correct query, though one that does not produce any\n> results).\n>\n>> $ git checkout 114\n>> error: pathspec '114' did not match any file(s) known to git.\n>\n> I think checkout has the same \"is this a path or a revision\" ambiguity\n> to resolve. But rather than be explicit that you might have meant \"114\"\n> as a tree, the error message assumes you meant a path. That might be\n> worth improving, similar to the above example.\n>\n> Again, you can disambiguate with:\n>\n>  $ git checkout -- 1147\n>  error: pathspec '1147' did not match any file(s) known to git.\n>\n>  $ git checkout 1147 --\n>  fatal: reference is not a tree: 1147\n>\n>> $ git checkout 1147\n>> fatal: reference is not a tree: 1147\n\nYes, I understand this.  This was a typo and it was ambiguous.  But\nshouldn't we tell the user the same thing when encountering the same\nfailure?\n\n> In this case the name does resolve to an object, so we try to use it as\n> such (even though we later find that it is useless for the operation).\n> We _could_ realize that it is not a tree and disambiguate to:\n>\n>  $ git checkout -- 1147\n>\n> but the current rule is at least consistent and simple.\n>\n>> $ git checkout 1146\n>> error: short SHA1 1146 is ambiguous.\n>> error: pathspec '1146' did not match any file(s) known to git.\n>\n> The sha1 is ambiguous, and therefore it does not resolve to anything. So\n> you get the same case as \"git checkout 1147\", but with the extra\n> ambiguity warning.\n\nThis one I like.  I only included it to demonstrate the different\nerror messages git produces.\n\n>> $ git merge 114\n>> fatal: '114' does not point to a commit\n>\n> It might be nice for this error message to be split into two cases:\n>\n>  1. the name does not resolve _at all_ (i.e., you made a typo)\n>\n>  2. the name does resolve to something, but it is not a commit\n>\n> In the latter case, we actually do get an extra error message from\n> elsewhere in the code:\n>\n>> $ git merge 1147\n>> error: 1147: expected commit type, but the object dereferences to blob type\n>> fatal: '1147' does not point to a commit\n>\n> But in case 1, it's not clear which is which (maybe even rewording it as\n> \"114 cannot be resolved as a commit\" would be less confusing).\n>\n>> $ git cherry-pick 114\n>> fatal: ambiguous argument '114': unknown revision or path not in the\n>> working tree.\n>> Use '--' to separate paths from revisions\n>> [...]\n>> $ git cherry-pick 1147\n>> fatal: BUG: expected exactly one commit from walk\n>\n> This is the \"does not resolve\" versus \"is not actually a commit\". In the\n> first case, though, I wonder if the error message is accurate. I'm not\n> sure if you can do \"git cherry-pick <rev> -- <paths>\", so the error\n> message is misleading\n\nGood point.  I missed that one completely.\n\n\n> (if anything, I would expect it to limit the\n> revision walk, but trying \"git cherry-pick HEAD -- 114\" seems to still\n> complain about the absence of 114).\n>\n>> [more examples]\n>\n> These are all variants that hopefully make sense in light of the\n> explanations above.\n>\n>> I can understand some of the inconsistent error reporting (checkout\n>> may expect filenames, but cherry-pick and merge do not).  But this\n>> seems too varied to me.\n>>\n>> And the first two look like bugs.\n>>\n>> Any comments or suggestions?\n>\n> I think the outcomes are all working as intended, but the error messages\n> could stand to be improved.\n\nYes, I agree.  I only meant to complain about the error messages, not\nthe results.  Thanks for the discussion.  I'll try to look for where\nthese come from and see if they can be improved within reason.\n\nPhil\n"},{"id":"185196","messageId":"20120222203821.GA6781@sigill.intra.peff.net","threadId":"29689","inReplyTo":"CABURp0r+3gOQ3rq_ubv=uEoU=XmtYs=etfT4W2Lb9M0LBrikWg@mail.gmail.com","subject":"Re: Ambiguous reference weirdness","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-22T20:38:21Z","receivedAt":"2012-02-22T20:38:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 22, 2012 at 09:48:23AM -0500, Phil Hord wrote:\n\n> > What is 1147? Is it supposed to be a partial sha1, or is it a ref you\n> > have?\n> \n> 1147 was a typo.  It was a Gerritt changeset ID I forgot to expand.\n> When I type \"git cherry-pick 1147<TAB>\", autocompletion expands this\n> for me to \"git cherry-pick origin/changes/47/1147/1\".  Except in this\n> case I misfired the TAB and got the weird \"BUG:\" report.  But I didn't\n> see the same problem with other invalid refs, so I went searching for\n> the variants and to see what caused it.\n\nAh. It's a little annoying that Gerrit names refs that look kind of\nlike partial sha1s. But I guess most of the time it's not that big a\ndeal (it is only because you were using the partial Gerrit ref with tab\ncompletion). And I don't think we're about to change how Gerrit names\nthings. :)\n\n> > Have you looked at the object that it resolves to? I suspect it is the\n> > partial sha1 of a non-commit object. E.g.:\n> \n> All of these examples were run in current git.git, so you can try them\n> yourself if needed.  But I did figure out that 1147 resolves to a\n> blob, and that's apparently the difference between these three:\n\nYeah. I figured that after reading more, but didn't go back and revise\nthe first part of my message. I was able to easily get the same results\nas you (actually, in my git.git, 1147 is ambiguous because I happen to\nhave some extra refs, but it was easy to replicate with a fresh clone).\n\n> $ git cherry-pick 1147\n> fatal: BUG: expected exactly one commit from walk\n> \n> $ git cherry-pick 1146\n> error: short SHA1 1146 is ambiguous.\n> error: short SHA1 1146 is ambiguous.\n> fatal: ambiguous argument '1146': unknown revision or path not in the\n> working tree.\n> Use '--' to separate paths from revisions\n> \n> $ git cherry-pick 114333\n> fatal: ambiguous argument '114333': unknown revision or path not in\n> the working tree.\n> Use '--' to separate paths from revisions\n> \n> I consider the first two responses to be UI bugs.   The second one is\n> minor (the twice-reported error message), and the first one is pretty\n> rude.   I would expect all three to report the same conclusion,\n> \"fatal: ambiguous argument 'XXXXX': unknown revision or path not in\n> the working tree.\"  But the first one doesn't.\n\nRight. Because cherry-pick's logic is:\n\n  if the arguments do not resolve to any objects at all\n      complain of unknown revision\n  if the resolved object is not a single commit\n      die of BUG\n\nAnd I think you just want to collapse those two conditions into a single\nconditional, which seems reasonable (the error message does say \"unknown\nrevision\", not \"unknown object\". It's a little more complicated than\nthat, because you are crossing a boundary between reusable library code\nand cherry-pick specific code.\n\nAs for the doubled \"ambiguous\" warning, I'm not sure what the cause is\nfor that. I suspect it is because cherry-pick tries to parse one way (as\na revision walk specifier, like \"a..b\"), and then parses again (as a\nsingle commit) when that fails, due to historical reasons. Obviously\nit would be nice to see that improved, but I suspect it will be annoying\nto do so; the error message is emitted by a low-level, and cherry-pick\nhas no idea that it has happened (it only sees \"this didn't result in\nparsing anything\").\n\n> Thanks.  I'm not familiar with the {tree} syntax -- in fact I'd like\n> to find a dictionary for all the reference spelling variants -- but\n> this is elucidating.\n\nSee \"git help rev-parse\", section \"Specifying Revisions\".\n\n> > In the cherry-pick case, the code is checking the right thing, but the\n> > message is horrible. It is not a bug, but merely unexpected input, and\n> > it should provide a usage message.\n> \n> Bug is too strong a word in one sense, but from the user perspective I\n> consider this \"horrible message\" a bug.\n\nSorry, I meant git is wrong to use the word \"BUG\". For us, die(\"BUG:\n...\") is the equivalent of an assert. It means something totally\nunexpected happened that should never happen, and therefore there is a\nbug in git. But this is not a bug in git (well, it is, but not that\nkind). It is a totally reasonable and expected malformed input that git\nshould diagnose and report correctly.\n\nSo it is a bug that git says \"BUG\". It should say \"you gave me objects,\nbut they are not revisions\" or something similar.\n\n> > I think checkout has the same \"is this a path or a revision\" ambiguity\n> > to resolve. But rather than be explicit that you might have meant \"114\"\n> > as a tree, the error message assumes you meant a path. That might be\n> > worth improving, similar to the above example.\n> >\n> > Again, you can disambiguate with:\n> >\n> >  $ git checkout -- 1147\n> >  error: pathspec '1147' did not match any file(s) known to git.\n> >\n> >  $ git checkout 1147 --\n> >  fatal: reference is not a tree: 1147\n> >\n> >> $ git checkout 1147\n> >> fatal: reference is not a tree: 1147\n> \n> Yes, I understand this.  This was a typo and it was ambiguous.  But\n> shouldn't we tell the user the same thing when encountering the same\n> failure?\n\nNo, because there are really three cases here:\n\n  1. The user told us 1147 is a path.\n\n  2. The user told us 1147 is a tree.\n\n  3. The user did not tell us which, and we must guess.\n\nWe guess (1) if the name does not resolve at all, and (2) otherwise. And\nthe error message only indicates the particular guess we made.\n\nSo while I don't think they should all have the same error message,\nthere are two improvements I can see:\n\n  a. If we are guessing, and 1147 resolves but is _not_ a tree, we\n     could guess path instead.\n\n  b. When our guess results in an error, it would be better to be\n     explicit about the fact that we guessed (i.e., say \"this is neither\n     a tree nor a path that git knows about\", similar to git-log).\n\n> > I think the outcomes are all working as intended, but the error messages\n> > could stand to be improved.\n> \n> Yes, I agree.  I only meant to complain about the error messages, not\n> the results.  Thanks for the discussion.  I'll try to look for where\n> these come from and see if they can be improved within reason.\n\nGreat. I look forward to it.\n\n-Peff\n"}]}