[BUG] in rev-parse

[BUG] in rev-parse

[nathan.panike]

In my local git.git:

$ git rev-parse 73c6b3575bc638b7096ec913bd91193707e2265d^@
57526fde5df201a99afa6d122c3266b3a1c5673a
942e6baa92846e5628752c65a22bc4957d8de4d0

$ git rev-parse --short 73c6b3575bc638b7096ec913bd91193707e2265d^@
57526fd
942e6ba
fatal: Needed a single revision

^^^ I don't believe this "fatal" message should be here

$ git version
git version 1.7.8

Nathan Panike

Re: [BUG] in rev-parse

[Jeff King]

On Wed, Dec 14, 2011 at 12:49:27PM -0600, nathan.panike@gmail.com wrote:

> In my local git.git:
> 
> $ git rev-parse 73c6b3575bc638b7096ec913bd91193707e2265d^@
> 57526fde5df201a99afa6d122c3266b3a1c5673a
> 942e6baa92846e5628752c65a22bc4957d8de4d0
> 
> $ git rev-parse --short 73c6b3575bc638b7096ec913bd91193707e2265d^@
> 57526fd
> 942e6ba
> fatal: Needed a single revision
> 
> ^^^ I don't believe this "fatal" message should be here

It looks like "--short" implies "--verify", and you cannot "--verify"
multiple sha1s.

But the documentation for "--short" says only:

  --short::
  --short=number::
          Instead of outputting the full SHA1 values of object names try to
          abbreviate them to a shorter unique name. When no length is specified
          7 is used. The minimum length is 4.

which would imply to me that not only should your example work, but this
should, too:

  $ git rev-parse HEAD HEAD^
  803b1a83b0ec5f04dfb770e83e11211e0015630f
  28c2058b6048d32abc0a23b827ab0b26a0332b9b

  $ git rev-parse --short HEAD HEAD^
  fatal: Needed a single revision

On the other hand, it has been like this since it was introduced in
2006, and I wonder if scripts rely on the --verify side effect.

As a work-around, you can get what you want with:

  git rev-list --no-walk --abbrev-commit $sha1^@

-Peff

Re: [BUG] in rev-parse

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On the other hand, it has been like this since it was introduced in
> 2006, and I wonder if scripts rely on the --verify side effect.

It would have been nicer if it did not to imply --verify at all; a long
hexdigit that do not name an existing object at all will be shortened to
its prefix that still do not collide with an abbreviated object name of an
existing object, and even in such a case, the command should not error out
only because it was fed a non-existing object (of course, if "--verify" is
given at the same time, its "one input that names existing object only"
rule should also kick in).

Re: [BUG] in rev-parse

[Jeff King]

On Wed, Dec 14, 2011 at 07:20:41PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On the other hand, it has been like this since it was introduced in
> > 2006, and I wonder if scripts rely on the --verify side effect.
> 
> It would have been nicer if it did not to imply --verify at all; a long
> hexdigit that do not name an existing object at all will be shortened to
> its prefix that still do not collide with an abbreviated object name of an
> existing object, and even in such a case, the command should not error out
> only because it was fed a non-existing object (of course, if "--verify" is
> given at the same time, its "one input that names existing object only"
> rule should also kick in).

Dropping the implied verify is easy (see below). But handling
non-existant sha1s is a much more complicated change, as the regular
abbreviation machinery assumes that they exist. E.g., with the patch
below:

  $ good=73c6b3575bc638b7096ec913bd91193707e2265d
  $ bad=${good#d}e
  $ git rev-parse --short $good
  73c6b35
  $ git rev-parse --short $bad
  [no output]

Anyway, I'm not sure it's worth changing at this point. It's part of the
plumbing API that has been that way forever, it's kind of a rare thing
to ask for, and I've already shown a workaround using rev-list.

-Peff

---
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 98d1cbe..b365ca0 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -545,7 +545,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			if (!strcmp(arg, "--short") ||
 			    !prefixcmp(arg, "--short=")) {
 				filter &= ~(DO_FLAGS|DO_NOREV);
-				verify = 1;
 				abbrev = DEFAULT_ABBREV;
 				if (arg[7] == '=')
 					abbrev = strtoul(arg + 8, NULL, 10);

Re: [BUG] in rev-parse

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Dropping the implied verify is easy (see below). But handling
> non-existant sha1s is a much more complicated change, as the regular
> abbreviation machinery assumes that they exist.

Hmm, I was very sure that I wrote a logic to do the "no such object" case
at least once, so I looked and found one in find_unique_abbrev(). But that
requires the caller to feed the full 160-bit sha1[] and perhaps it is not
easily used in the context of rev-parse (I didn't look further).

> Anyway, I'm not sure it's worth changing at this point. It's part of the
> plumbing API that has been that way forever,...

Ok.

Re: [BUG] in rev-parse

[Michael Haggerty]

On 12/15/2011 08:05 AM, Jeff King wrote:
> On Wed, Dec 14, 2011 at 07:20:41PM -0800, Junio C Hamano wrote:
> 
>> Jeff King <peff@peff.net> writes:
>>
>>> On the other hand, it has been like this since it was introduced in
>>> 2006, and I wonder if scripts rely on the --verify side effect.
>>
>> It would have been nicer if it did not to imply --verify at all; a long
>> hexdigit that do not name an existing object at all will be shortened to
>> its prefix that still do not collide with an abbreviated object name of an
>> existing object, and even in such a case, the command should not error out
>> only because it was fed a non-existing object (of course, if "--verify" is
>> given at the same time, its "one input that names existing object only"
>> rule should also kick in).
> 
> Dropping the implied verify is easy (see below). But handling
> non-existant sha1s is a much more complicated change, as the regular
> abbreviation machinery assumes that they exist. E.g., with the patch
> below:
> 
>   $ good=73c6b3575bc638b7096ec913bd91193707e2265d
>   $ bad=${good#d}e
>   $ git rev-parse --short $good
>   73c6b35
>   $ git rev-parse --short $bad
>   [no output]
> 
> Anyway, I'm not sure it's worth changing at this point. It's part of the
> plumbing API that has been that way forever, it's kind of a rare thing
> to ask for, and I've already shown a workaround using rev-list.

I believe that the OP was more inconvenienced that "git rev-parse
--short" chokes on multiple objects than by the fact that it insists
that the objects exist.  (And shortening the SHA1s of non-existent
objects doesn't sound very useful anyway.)  So I think that a useful
compromise would be for "git rev-parse --short" to accept multiple args
but continue to insist that each of the args is a valid object.

If that is considered too big a break with backwards compatibility, one
could add a --no-verify option that turns off the verification behavior
of --short.  But IMHO this problem is not important enough to justify
adding an extra option.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [BUG] in rev-parse

[Jeff King]

On Thu, Dec 15, 2011 at 11:53:50PM +0100, Michael Haggerty wrote:

> I believe that the OP was more inconvenienced that "git rev-parse
> --short" chokes on multiple objects than by the fact that it insists
> that the objects exist.  (And shortening the SHA1s of non-existent
> objects doesn't sound very useful anyway.)  So I think that a useful
> compromise would be for "git rev-parse --short" to accept multiple args
> but continue to insist that each of the args is a valid object.

Part of the guarantee of "--verify" is that it returns a single object.
I don't know how many callers rely on "--short" implying "--verify"
implying a single object.

I agree in practice it would probably be an OK change, and it's very
easy to do. I just don't think it's an important enough problem to worry
about, given the available workaround. But if you want to write the
patch, be my guest. :)

-Peff