{"thread":{"id":"29166","subject":"[BUG] in rev-parse","startedAt":"2011-12-14T18:49:27Z","lastAt":"2011-12-17T12:02:10Z","messageCount":7,"participants":["nathan.panike@gmail.com","Jeff King","Junio C Hamano","Michael Haggerty"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"181182","messageId":"20111214184926.GB18335@llunet.cs.wisc.edu","threadId":"29166","inReplyTo":null,"subject":"[BUG] in rev-parse","fromName":"","fromEmail":"nathan.panike@gmail.com","sentAt":"2011-12-14T18:49:27Z","receivedAt":"2011-12-14T18:49:27Z","isPatch":false,"sender":{"key":"nathan.panike@gmail.com","avatar":"https://avatars.githubusercontent.com/u/389447?v=4"},"body":"In my local git.git:\n\n$ git rev-parse 73c6b3575bc638b7096ec913bd91193707e2265d^@\n57526fde5df201a99afa6d122c3266b3a1c5673a\n942e6baa92846e5628752c65a22bc4957d8de4d0\n\n$ git rev-parse --short 73c6b3575bc638b7096ec913bd91193707e2265d^@\n57526fd\n942e6ba\nfatal: Needed a single revision\n\n^^^ I don't believe this \"fatal\" message should be here\n\n$ git version\ngit version 1.7.8\n\nNathan Panike\n"},{"id":"181183","messageId":"20111214210157.GA8990@sigill.intra.peff.net","threadId":"29166","inReplyTo":"20111214184926.GB18335@llunet.cs.wisc.edu","subject":"Re: [BUG] in rev-parse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-14T21:01:57Z","receivedAt":"2011-12-14T21:01:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 14, 2011 at 12:49:27PM -0600, nathan.panike@gmail.com wrote:\n\n> In my local git.git:\n> \n> $ git rev-parse 73c6b3575bc638b7096ec913bd91193707e2265d^@\n> 57526fde5df201a99afa6d122c3266b3a1c5673a\n> 942e6baa92846e5628752c65a22bc4957d8de4d0\n> \n> $ git rev-parse --short 73c6b3575bc638b7096ec913bd91193707e2265d^@\n> 57526fd\n> 942e6ba\n> fatal: Needed a single revision\n> \n> ^^^ I don't believe this \"fatal\" message should be here\n\nIt looks like \"--short\" implies \"--verify\", and you cannot \"--verify\"\nmultiple sha1s.\n\nBut the documentation for \"--short\" says only:\n\n  --short::\n  --short=number::\n          Instead of outputting the full SHA1 values of object names try to\n          abbreviate them to a shorter unique name. When no length is specified\n          7 is used. The minimum length is 4.\n\nwhich would imply to me that not only should your example work, but this\nshould, too:\n\n  $ git rev-parse HEAD HEAD^\n  803b1a83b0ec5f04dfb770e83e11211e0015630f\n  28c2058b6048d32abc0a23b827ab0b26a0332b9b\n\n  $ git rev-parse --short HEAD HEAD^\n  fatal: Needed a single revision\n\nOn the other hand, it has been like this since it was introduced in\n2006, and I wonder if scripts rely on the --verify side effect.\n\nAs a work-around, you can get what you want with:\n\n  git rev-list --no-walk --abbrev-commit $sha1^@\n\n-Peff\n"},{"id":"181204","messageId":"7vk45yplkm.fsf@alter.siamese.dyndns.org","threadId":"29166","inReplyTo":"20111214210157.GA8990@sigill.intra.peff.net","subject":"Re: [BUG] in rev-parse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-15T03:20:41Z","receivedAt":"2011-12-15T03:20:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On the other hand, it has been like this since it was introduced in\n> 2006, and I wonder if scripts rely on the --verify side effect.\n\nIt would have been nicer if it did not to imply --verify at all; a long\nhexdigit that do not name an existing object at all will be shortened to\nits prefix that still do not collide with an abbreviated object name of an\nexisting object, and even in such a case, the command should not error out\nonly because it was fed a non-existing object (of course, if \"--verify\" is\ngiven at the same time, its \"one input that names existing object only\"\nrule should also kick in).\n"},{"id":"181210","messageId":"20111215070521.GB1327@sigill.intra.peff.net","threadId":"29166","inReplyTo":"7vk45yplkm.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] in rev-parse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-15T07:05:21Z","receivedAt":"2011-12-15T07:05:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 14, 2011 at 07:20:41PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On the other hand, it has been like this since it was introduced in\n> > 2006, and I wonder if scripts rely on the --verify side effect.\n> \n> It would have been nicer if it did not to imply --verify at all; a long\n> hexdigit that do not name an existing object at all will be shortened to\n> its prefix that still do not collide with an abbreviated object name of an\n> existing object, and even in such a case, the command should not error out\n> only because it was fed a non-existing object (of course, if \"--verify\" is\n> given at the same time, its \"one input that names existing object only\"\n> rule should also kick in).\n\nDropping the implied verify is easy (see below). But handling\nnon-existant sha1s is a much more complicated change, as the regular\nabbreviation machinery assumes that they exist. E.g., with the patch\nbelow:\n\n  $ good=73c6b3575bc638b7096ec913bd91193707e2265d\n  $ bad=${good#d}e\n  $ git rev-parse --short $good\n  73c6b35\n  $ git rev-parse --short $bad\n  [no output]\n\nAnyway, I'm not sure it's worth changing at this point. It's part of the\nplumbing API that has been that way forever, it's kind of a rare thing\nto ask for, and I've already shown a workaround using rev-list.\n\n-Peff\n\n---\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 98d1cbe..b365ca0 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -545,7 +545,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\tif (!strcmp(arg, \"--short\") ||\n \t\t\t    !prefixcmp(arg, \"--short=\")) {\n \t\t\t\tfilter &= ~(DO_FLAGS|DO_NOREV);\n-\t\t\t\tverify = 1;\n \t\t\t\tabbrev = DEFAULT_ABBREV;\n \t\t\t\tif (arg[7] == '=')\n \t\t\t\t\tabbrev = strtoul(arg + 8, NULL, 10);\n"},{"id":"181233","messageId":"7vvcphohn1.fsf@alter.siamese.dyndns.org","threadId":"29166","inReplyTo":"20111215070521.GB1327@sigill.intra.peff.net","subject":"Re: [BUG] in rev-parse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-15T17:43:14Z","receivedAt":"2011-12-15T17:43:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Dropping the implied verify is easy (see below). But handling\n> non-existant sha1s is a much more complicated change, as the regular\n> abbreviation machinery assumes that they exist.\n\nHmm, I was very sure that I wrote a logic to do the \"no such object\" case\nat least once, so I looked and found one in find_unique_abbrev(). But that\nrequires the caller to feed the full 160-bit sha1[] and perhaps it is not\neasily used in the context of rev-parse (I didn't look further).\n\n> Anyway, I'm not sure it's worth changing at this point. It's part of the\n> plumbing API that has been that way forever,...\n\nOk.\n"},{"id":"181270","messageId":"4EEA7A7E.4070109@alum.mit.edu","threadId":"29166","inReplyTo":"20111215070521.GB1327@sigill.intra.peff.net","subject":"Re: [BUG] in rev-parse","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-12-15T22:53:50Z","receivedAt":"2011-12-15T22:53:50Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/15/2011 08:05 AM, Jeff King wrote:\n> On Wed, Dec 14, 2011 at 07:20:41PM -0800, Junio C Hamano wrote:\n> \n>> Jeff King <peff@peff.net> writes:\n>>\n>>> On the other hand, it has been like this since it was introduced in\n>>> 2006, and I wonder if scripts rely on the --verify side effect.\n>>\n>> It would have been nicer if it did not to imply --verify at all; a long\n>> hexdigit that do not name an existing object at all will be shortened to\n>> its prefix that still do not collide with an abbreviated object name of an\n>> existing object, and even in such a case, the command should not error out\n>> only because it was fed a non-existing object (of course, if \"--verify\" is\n>> given at the same time, its \"one input that names existing object only\"\n>> rule should also kick in).\n> \n> Dropping the implied verify is easy (see below). But handling\n> non-existant sha1s is a much more complicated change, as the regular\n> abbreviation machinery assumes that they exist. E.g., with the patch\n> below:\n> \n>   $ good=73c6b3575bc638b7096ec913bd91193707e2265d\n>   $ bad=${good#d}e\n>   $ git rev-parse --short $good\n>   73c6b35\n>   $ git rev-parse --short $bad\n>   [no output]\n> \n> Anyway, I'm not sure it's worth changing at this point. It's part of the\n> plumbing API that has been that way forever, it's kind of a rare thing\n> to ask for, and I've already shown a workaround using rev-list.\n\nI believe that the OP was more inconvenienced that \"git rev-parse\n--short\" chokes on multiple objects than by the fact that it insists\nthat the objects exist.  (And shortening the SHA1s of non-existent\nobjects doesn't sound very useful anyway.)  So I think that a useful\ncompromise would be for \"git rev-parse --short\" to accept multiple args\nbut continue to insist that each of the args is a valid object.\n\nIf that is considered too big a break with backwards compatibility, one\ncould add a --no-verify option that turns off the verification behavior\nof --short.  But IMHO this problem is not important enough to justify\nadding an extra option.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"181397","messageId":"20111217120210.GB31152@sigill.intra.peff.net","threadId":"29166","inReplyTo":"4EEA7A7E.4070109@alum.mit.edu","subject":"Re: [BUG] in rev-parse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-17T12:02:10Z","receivedAt":"2011-12-17T12:02:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 15, 2011 at 11:53:50PM +0100, Michael Haggerty wrote:\n\n> I believe that the OP was more inconvenienced that \"git rev-parse\n> --short\" chokes on multiple objects than by the fact that it insists\n> that the objects exist.  (And shortening the SHA1s of non-existent\n> objects doesn't sound very useful anyway.)  So I think that a useful\n> compromise would be for \"git rev-parse --short\" to accept multiple args\n> but continue to insist that each of the args is a valid object.\n\nPart of the guarantee of \"--verify\" is that it returns a single object.\nI don't know how many callers rely on \"--short\" implying \"--verify\"\nimplying a single object.\n\nI agree in practice it would probably be an OK change, and it's very\neasy to do. I just don't think it's an important enough problem to worry\nabout, given the available workaround. But if you want to write the\npatch, be my guest. :)\n\n-Peff\n"}]}