patchrev-parse: clarify documentation for the --verify option

The old version could be read to mean that the argument has to refer to a valid object, but that is incorrect:

* the object is not necessarily read (e.g., to check for corruption)

* if the argument is a 40-digit string of hex digits, then it is
  accepted whether or not is is the name of an existing object.

So reword the explanation to be less ambiguous.

Also fix the examples involving --verify: to be sure that the argument refers to a commit (rather than some other kind of object), the argument has to be suffixed with "^{commit}". This trick is not possible in the example involving --default, so don't imply that it is exactly the same as the previous example.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

This was discussed in the following thread:

    http://thread.gmane.org/gmane.comp.version-control.git/219409

Please note that the second example already reveals a fly in the ointment:

    $ git rev-parse --default master --verify $REV

Following Junio's advice, we would like to change this to

    $ git rev-parse --default master --verify $REV^{commit}

but that defeats the purpose of the --default option.  Doing

    $ git rev-parse --verify $(git rev-parse --default master $REV)^{commit}

is just plain ugly. Though honestly, I don't see the point of using --default as opposed to

    $ git rev-parse --verify ${REV:-master}^{commit}

So maybe the second example should be removed entirely or converted to use ${REV:-master} rather than --default.

 Documentation/git-rev-parse.txt | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

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

I would agree ${VAR:-default} is sufficient in that particular case.

The --default is more about the use of the pluming command not with --verify but as its original use of an "argument sifter" when composing "git rev-list" piped into "git diff-tree --stdin", i.e.

	git rev-list $(git rev-parse --default HEAD --revs-only "$@") |
        git diff-tree --stdin $(git rev-parse --no-revs "$@")

which was the original way to write commands in the "git log" family using the plumbing command as a scripted Porcelain.

Perhaps s/used as a single object name/turned into a raw 20-byte SHA-1/;

Because the primary use case of this option is to implement end-user input validation, I think it would be helpful to clarify use of the peeler here. Perhaps

    --verify::
            Make sure the single given parameter can be turned into a
            raw 20-byte SHA-1, something that can be used to access the
            object database, and emit the SHA-1 in 40-hex format (unless
            --symbolic and other formatting option is given); otherwise,
            error out.
    +
    If you want to make sure that the output from this actually
    names an object in your object database and/or can be used
    as a specific type of object you require, it is a good idea
    to add "^{type}" peeling operator to the parmeter.  For
    example, `git rev-parse "$VAR^{commit}"` will make sure $VAR
    names an existing object that is a commit-ish (i.e. a
    commit, or an annotated tag that points at a commit).  To
    make sure that $VAR names an existing object of any type,
    you can say `git rev-parse "$VAR^{object}"`.

or something.

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

A "SQUASH???" patch on top of your original is queued on 'pu', together with the earlier "^{object}" peeler patch. Comments, improvements, etc. would be nice.

Thanks.

On 04/01/2013 06:56 PM, Junio C Hamano wrote:

Yes, your version is better.  I would make one change, though.  In your

+ Make sure the single given parameter can be turned into a + raw 20-byte SHA-1 that can be used to access the object + database, and emit it to the standard output. If it can't, + error out.

it could be made clearer that exactly one parameter should be provided.
 Maybe

+	Verify that exactly one parameter is provided, and that it
+       can be turned into a raw 20-byte SHA-1 that can be used to
+	access the object database.  If so, emit the SHA-1 to the
+	standard output; otherwise, error out.

But this makes it sound a little like the "raw 20-byte SHA-1" will be output to stdout, whereas both the input and the output are in fact 40-character hex-encoded SHA-1s. Perhaps a further change

    s/raw 20-byte SHA-1/full SHA-1/

would avoid the false implication?

Michael

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

That is probably better (I was hoping "the single" would mean the same to the reader, though). Thanks.

I did consider that point, wrote "and outputs 40-hex" in my earlier draft, and then rejected it because it was even more misleading. The output follows the usual rules for "rev" parameters, e.g.

	git rev-parse --short --verify HEAD
	git rev-parse --symbolic --verify v1.8.2^{tree}

and "--verify" does not mean 40-hex output. That is why I left it vague as "emit it".

I agree that the wording incorrectly hints that you may be able to get 20-byte raw output. I didn't find a satisfactory phrasing.

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

It's the explicit mention of "raw 20-byte" that puts the reader in mind of 20-byte binary data. I think any version that omitted that phrase would let the reader make the assumption that the SHA-1s are expressed as 40-byte hex numbers just they are everywhere else in the command-line interface.

But I'm OK with any of the variations that we have discussed.

Michael