[PATCH] Documentation/rev-parse: --verify does not check existence

[PATCH] Documentation/rev-parse: --verify does not check existence

[Thomas Rast]

Rather counterintuitively,

  $ git rev-parse --verify aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

does *not* fail.  The check is based solely on whether we can come up
with something that "looks like" a SHA1, not whether the object
actually exists.  To wit:

  # this cannot be done with update-ref as that *does* check
  $ echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa > .git/refs/heads/nonexistent
  $ git rev-parse --verify nonexistent
  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

Note that the commit message of 79162bb makes clear that this is
exactly the intended behaviour:

  git-rev-parse: Allow a "zeroth" parent of a commit - the commit itself.

  This sounds nonsensical, but it's useful to make sure that the result is
  a commit.
[...]
  Also, since the "parent" code will actually parse the commit, this,
  together with the "--verify" flag, will verify not only that the result
  is a single SHA1, but will also have verified that it's a proper commit
  that we can see.

Document this clearly in the description for --verify.

Furthermore the second item in EXAMPLES

  * Print the commit object name from the revision in the $REV shell variable:

seems to imply that rev-parse should actually check that the object
exists *and* is a commit, when in reality it does neither.  We could
suggest the ^0 trick alluded to above, but instead document the more
verbose (and clear)

  $ git rev-parse --verify "$REV^{commit}"

Observe that if you ran

  $ git rev-parse --verify "nonexistent^{commit}"

after the above setup, the failure would come from the ^{} peeling
operator and not from --verify.

Noticed-by: Zachery Hostens <zacheryph@gmail.com>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-rev-parse.txt |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index ff23cb0..779fa87 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -59,8 +59,9 @@ OPTIONS
 	instead.
 
 --verify::
-	The parameter given must be usable as a single, valid
-	object name.  Otherwise barf and abort.
+	The parameter must either be formed like an object name, or
+	dereference to an object name.  This does 'not' verify that
+	the object actually exists!  See EXAMPLES below.
 
 -q::
 --quiet::
@@ -292,21 +293,31 @@ EXAMPLES
 $ git rev-parse --verify HEAD
 ------------
 
-* Print the commit object name from the revision in the $REV shell variable:
+* Print the object name from the revision in the $REV shell variable:
 +
 ------------
 $ git rev-parse --verify $REV
 ------------
 +
-This will error out if $REV is empty or not a valid revision.
+This will error out if $REV does not dereference to a well-formed
+object name (i.e., SHA1).
 
-* Same as above:
+* Same as above, but also verify that the object exists and is a commit:
++
+------------
+$ git rev-parse --verify "$REV^{commit}"
+------------
++
+This works because the `{caret}\{commit\}` peeling operator will fail
+unless the object exists and can be peeled into a commit.
+
+* Print the object name from $REV, but default to master:
 +
 ------------
 $ git rev-parse --default master --verify $REV
 ------------
 +
-but if $REV is empty, the commit object name from master will be printed.
+If $REV is empty, the commit object name from master will be printed.
 
 
 Author
-- 
1.7.4.rc1.309.g58aa0

Re: [PATCH] Documentation/rev-parse: --verify does not check existence

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> index ff23cb0..779fa87 100644
> --- a/Documentation/git-rev-parse.txt
> +++ b/Documentation/git-rev-parse.txt
> @@ -59,8 +59,9 @@ OPTIONS
>  	instead.
>  
>  --verify::
> -	The parameter given must be usable as a single, valid
> -	object name.  Otherwise barf and abort.
> +	The parameter must either be formed like an object name, or
> +	dereference to an object name.  This does 'not' verify that
> +	the object actually exists!  See EXAMPLES below.

To paraphrase what this text says, if you do

	echo >.git/refs/tags/next e17aa8a9dca972ca278dd91a097873101066e963

where that hexstring does not name any existing object [*1*], you can
successfully verify "tags/next" (i.e. "git rev-parse --verify tags/next"),
but not "tags/next^0", because "tags/next^0" does not "dereference" to a
40-hex string, while "tags/next" does.

That's all good, but as I said, the hexstring is _not_ an object name.  So
"dereference to an object name" is actively a wrong thing to say here.

You meant "40-hex string" with "formed _like_ an object name" in the
above, and the "like" is an attempt to say "could be usable to name an
object, but does not mean that the object has to exist".

But "dereferencing" a 40-hex string yields itself, so what you wrote can
be simplified to something like this, without using a wrong term "object
name":

        Makes sure that the parameter dereferences to a string of 40
        hexadecimal characters.  Otherwise barf and abort.

But then, if you read the original carefully, it already says "must be
usable as a single valid object name", and "be usable as" does not
necessarily mean "named object must exist".  So I agree you identified
something that needs to be clarified, but I do not think your rewrite
clarified things very much.

How about this?

	The parameter given must be usable as a string to name a single
        object (note that the named object does not have to exist).
        Otherwise barf and abort.

Unfortunately, this does not clarify if it satisfies the above condition
to say "refs/heads/nosuch" in a repository without "nosuch" branch---the
ref expression is usable as a string to name a single object, so it should
verify Ok, but the reason it doesn't name a single object is not because
it yields a 40-hex string that no object with that name exists, but
because it doesn't dereference to a 40-hex string to begin with.  So it
actually should _not_ verify well.

To fully clarify this, I think the first step we need is to add a better
definition of "to dereference" (or whatever word we would end up using to
call the act of passing a string through get_sha1()) to the glossary.
Then we can use the "string of 40 hexadecimal" rewrite I gave earlier.

[Footnote]

*1* These days, "git tag" and even "git update-ref" seems to verify that
the given name actually refers to an existing object, so this experiment
has to be done by manually futzing with the repository.  Joy of safety ;-)