Bug in "git rev-parse --verify"

On Junio's master, "git rev-parse --verify" accepts *any* 40-digit hexadecimal number. For example, pass it 40 "1" characters, and it accepts the argument:

$ git rev-parse --verify 1111111111111111111111111111111111111111 1111111111111111111111111111111111111111 $ echo $? 0

Obviously, my repo doesn't have an object with this hash :-) so I think this argument should be rejected.

If you add or remove a digit (to make the length different than 40), it is correctly rejected:

$ git rev-parse --verify 111111111111111111111111111111111111111 fatal: Needed a single revision $ echo $? 128

I believe that "git rev-parse --verify" is meant to verify that the
argument is an actual object, and that it should reject fictional SHA1s.
 (If not then the documentation should be clarified.)  The same problem
also exists in 1.8.2 but I haven't checked how much older it is.

The behavior presumably comes from the following clause in get_sha1_basic():

	if (len == 40 && !get_sha1_hex(str, sha1))
		return 0;

I won't have time to pursue this.

Michael

On Thu, Mar 28, 2013 at 02:04:27PM +0100, Michael Haggerty wrote:

I think it is only about verifying the name of the object. I.e., that you can resolve the argument to an object. It has always behaved this way; I even just tested with git v0.99.

I can't think of any reason it would not be helpful to have it _also_ verify that we have the object in question. Most of the time that would be a no-op, as any ref we resolve should point to a valid object, but it would mean we could catch repository corruption (i.e., missing objects) early.

Doing a has_sha1_file() check would be relatively quick. Doing a full parse_object and checking the sha1 would be more robust, but would mean that:

  blob=`git rev-parse --verify HEAD:some-large-file`

would become much more expensive (and presumably you are about to access the object _anyway_, at which point you would notice problems, so it is redundantly expensive).

-Peff

Jeff King <peff@peff.net> writes:

Correct. It is about "is it well formed and something we can turn into 20-byte object name?" and nothing more. It certainly does not mean "do we have it?", as the function needs to be able to validate something we do not yet have.

On 03/28/2013 02:48 PM, Junio C Hamano wrote:

That's surprising.  The man page says

    --verify
        The parameter given must be usable as a single, valid object name.
        Otherwise barf and abort.

"Valid", to me, implies that the parameter should be the name of an actual object, and this also seems a more useful concept to me and more consistent with the command's behavior when passed other arguments.

Is there a simple way to verify an object name more strictly and convert it to an SHA1? I can only think of solutions that require two commands, like

    git cat-file -e $ARG && git rev-parse --verify $ARG

I suppose in most contexts where one wants to know whether an object name is valid, one should also verify that the object has the type that you expect:

    test X$(git cat-file -t $ARG) = Xcommit &&
        git rev-parse --verify $ARG

or (allowing tag dereferencing)

    git cat-file -e $ARG^{commit} &&
        git rev-parse --verify $ARG^{commit}

Michael

On Thu, Mar 28, 2013 at 04:34:19PM +0100, Michael Haggerty wrote:

Is the rev-parse line doing anything there? If $ARG does not resolve to a sha1, then wouldn't cat-file have failed?

-Peff

On 03/28/2013 04:38 PM, Jeff King wrote:

It's outputting the SHA1, which cat-file seems incapable of providing in a useful way.

Michael

On Thu, Mar 28, 2013 at 04:52:15PM +0100, Michael Haggerty wrote:

Ah, I see; I was looking too much at your example, and not thinking about how you would want to use it in a script.

-Peff

Jeff King <peff@peff.net> writes:

You could force rev-parse to resolve the input to an existing object, with something like this:

	git rev-parse --verify "$ARG^{}"

It will unwrap a tag, so the output may end up pointing at a object that is different from $ARG in such a case.

But what is the purpose of turning a random string to a random 40-hex in the first place?

On 03/28/2013 05:52 PM, Junio C Hamano wrote:

Yes, if unwrapping tags is OK then this would work.

In non-trivial scripts, it makes sense to convert user input into a known and verified quantity (SHA1) once, while processing external inputs, and not have to think about it afterwards. Verifying and converting to pure-SHA1s as soon as possible has several advantages:

1. An SHA1 is a canonical representation of the argument, useful for
example as the key in a hash map for for looking for the presence of a
commit in a rev-list output.

2. An SHA1 is persistent.  For example, I use them when caching
benchmark results across versions.  Moreover, they are safe for use in
filenames.  The persistence also makes scripts more robust against
simultaneous changes to the repo by other processes, whereas if I use a
string like "branch^" multiple times, there is no guarantee that it
always refers to the same commit.

3. Verifying arguments at one spot centralizes error-checking at the
start of a script and eliminates one reason for random git commands to
fail later (when error recovery is perhaps more difficult).

4. Converting once avoids the overhead of repeated conversion from a
free-form committish into an object name if the argument needs to be
passed to multiple git commands (though presumably the overhead is
insignificant in most cases).

Undoubtedly in many cases this practice of early-verification-and-conversion is unnecessary, or the same benefit could be had from either verifying or converting without doing both. But verifying-and-converting is easy, cheap, and means not having to decide on a case-by-case basis whether it could have been avoided.

Michael

Michael Haggerty <mhagger@alum.mit.edu> writes:

These two are half-irrelevant; they only call for use of the current "rev-parse --verify" that always gives you 40-hex. The more important one is the next one.

Not necessarily, unless your script is a very narrow corner case that is satisfied with "any kind of object goes". When a parameter has to be commit for some purpose and another parameter can be any tree-ish, you would want to validate them _with_ type requirement.

If you are using the "object name persistency" to create a cache of "how many times the word 'gogopuff' appears in the entire tree?", you want to cache the result keyed with the object name of the tree, whether your user gives you v1.8.0 (tag), v1.8.0^0 (commit), or v1.8.0^{tree} (tree), and you want to unwrap the user input down to tree object name to look up the pre-computed result for the cache to be any useful.

True.

So why not verify arguments while making sure of their type early with 'rev-parse --verify "$userinput^{desiredtype}"?

Junio C Hamano <gitster@pobox.com> writes:

Clarification. I meant "A single central point to check is a useful concept, but centrally checking 'does this exist? I do not care what it is' is not necessarily useful and I suspect it is not useful more often than it is". When you care if something exists, you often want to know what it is and other aspects of that thing at the same time.

On 03/30/2013 05:09 AM, Junio C Hamano wrote:

Yes, that's a better solution in almost all cases.  Thanks for the tip.

(It doesn't change my opinion that the documentation for "rev-parse --verify" is misleading, but given that you don't appear to want to change its behavior I will submit a documentation patch.)

Michael

Michael Haggerty <mhagger@alum.mit.edu> writes:

It does not matter what I want. What matters is that changing the definition is a _wrong_ thing to do, as --verify is designed to be usable for objects you may not yet have.

What we may want is another type peeling operator, ^{object}. makes sure it is an object, that lets you say:

    rev-parse --verify 572a535454612a046e7dd7404dcca94d6243c788^{object}

It asks "I have this 40-hex; I want an object out of it", just like frotz^{tree} is "I have 'frotz'; I want a tree-ish" for any value of 'frotz'.

With that, a use case that it wants to see _any_ object can safely use 'rev-parse --verify "$userinput^{object}' without an annotated tag getting in the way.

How does that sound?

Junio C Hamano <gitster@pobox.com> writes:

Perhaps something like this. Note that the last hunk is unrelated thinko-fix I noticed while browsing the code.

-- >8 --
Subject: sha1_name.c: ^{object} peeler

A string that names an object can be suffixed with ^{type} peeler to say "I have this object name; peel it until you get this type. If you cannot do so, it is an error". v1.8.2^{commit} asks for a commit that is pointed at an annotated tag v1.8.2; v1.8.2^{tree} unwraps it further to the top-level tree object. A special suffix ^{} (i.e. no type specified) means "I do not care what it unwraps to; just peel annotated tag until you get something that is not a tag".

When you have a random user-supplied string, you can turn it to a bare 40-hex object name, and cause it to error out if such an object does not exist, with:

	git rev-parse --verify "$userstring^{}"

for most objects, but this does not yield the tag object name when $userstring refers to an annotated tag.

Introduce a new suffix, ^{object}, that only makes sure the given name refers to an existing object. Then

	git rev-parse --verify "$userstring^{object}"

becomes a way to make sure $userstring refers to an existing object.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_name.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

On 03/30/2013 08:05 AM, Junio C Hamano wrote:

Sounds reasonable to me. I'm not familiar with this code, but your change looks simple enough. Plus documentation change in Documentation/revisions.txt, of course.

Thanks, Michael

Fwiw, look very a sound idea for me.

Best

2013/3/30, Junio C Hamano <gitster@pobox.com>:

The function already knows when interpreting $foo^{commit} to tell the underlying get_sha1_1() to expect a commit-ish while evaluating $foo. Teach it to do the same when asked for $foo^{tree}; we are expecting a tree-ish and $foo should be disambiguated in favor of a tree-ish, discarding a possible ambiguous match with a blob object.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

Junio C Hamano <gitster@pobox.com> writes:

 sha1_name.c | 2 ++
 1 file changed, 2 insertions(+)

A string that names an object can be suffixed with ^{type} peeler to say "I have this object name; peel it until you get this type. If you cannot do so, it is an error". v1.8.2^{commit} asks for a commit that is pointed at an annotated tag v1.8.2; v1.8.2^{tree} unwraps it further to the top-level tree object. A special suffix ^{} (i.e. no type specified) means "I do not care what it unwraps to; just peel annotated tag until you get something that is not a tag".

When you have a random user-supplied string, you can turn it to a bare 40-hex object name, and cause it to error out if such an object does not exist, with:

	git rev-parse --verify "$userstring^{}"

for most objects, but this does not yield the tag object name when $userstring refers to an annotated tag.

Introduce a new suffix, ^{object}, that only makes sure the given name refers to an existing object. Then

	git rev-parse --verify "$userstring^{object}"

becomes a way to make sure $userstring refers to an existing object.

This is necessary because the plumbing "rev-parse --verify" is only about "make sure the argument is something we can feed to get_sha1() and turn it into a raw 20-byte object name SHA-1" and is not about "make sure that 20-byte object name SHA-1 refers to an object that exists in our object store". When the given $userstring is already a 40-hex, by definition "rev-parse --verify $userstring" can turn it into a raw 20-byte object name. With "$userstring^{object}", we can make sure that the 40-hex string names an object that exists in our object store before "--verify" kicks in.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_name.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

While I was in the middle of suggesting documentation for this new syntax, I discovered that you already added documentation to your repo but didn't mention the new version on the mailing list (or maybe I overlooked it). It would be helpful if you would submit your own changes to the mailing list to make it harder for the rest of us to overlook them--and easier to look over them :-)

The documentation looks fine to me.

Off topic: Your patch reminds me of something else that surprised me: there is no "$userstring^{tag}". I suppose it would be a bit ambiguous, given that tags can point at tags, and it would also be less useful than the other suffixes. But its absence irked the completionist in me :-)

Michael

On 04/01/2013 12:38 AM, Junio C Hamano wrote:

Michael Haggerty <mhagger@alum.mit.edu> writes:

Yes, unfortunately, foo^{type} means "start from foo, and until what you are looking at it is of type, repeatedly peel to see if you can get to an object of that type, or stop and report an error". If a tag A points at another tag B, which in turn points at an object C, you will never see B by applying usual peeling operator.

Also, v1.8.2^{tag} would be give the tag itself, while master^{tag} would not report the commit "master" but would error out, which would be useless. You are better off doing `git cat-file -t foo` and seeing if it is a tag object at that point.

On 04/02/2013 05:45 PM, Junio C Hamano wrote:

All correct, of course. But the user would never use "master^{tag}" unless he wants a tag and nothing else, so erroring out would be exactly the thing he wants in that case. This is no different than the "^{commit}" part of "master^{tree}^{commit}", which correctly errors out because a commit cannot be inferred from a tree.

Michael

Michael Haggerty <mhagger@alum.mit.edu> writes:

Correct; I was only saying adding it is not something that solves a problem that cannot be solved with the current system (i.e. no added value from feature point-of-view). I would not object to a patch to allow "git rev-parse v1.8.2^{tag}" for completeness.

We may want to rethink if we can lose the hardcoded lengths like 6, 3, 4, 4 from here, but I didn't bother ;-).

 sha1_name.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)