Re: [PATCH] rev-parse: clarify documentation for the --verify option
On 04/01/2013 06:56 PM, Junio C Hamano wrote:
Junio C Hamano gits...@pobox.com writes:
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
...
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.
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
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse: clarify documentation for the --verify option
Michael Haggerty mhag...@alum.mit.edu writes:
On 04/01/2013 06:56 PM, Junio C Hamano wrote:
Junio C Hamano gits...@pobox.com writes:
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
...
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.
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
That is probably better (I was hoping the single would mean the
same to the reader, though). Thanks.
+ 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,...
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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse: clarify documentation for the --verify option
On 04/02/2013 04:57 PM, Junio C Hamano wrote:
Michael Haggerty mhag...@alum.mit.edu writes:
On 04/01/2013 06:56 PM, Junio C Hamano wrote:
Junio C Hamano gits...@pobox.com writes:
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
...
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.
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
That is probably better (I was hoping the single would mean the
same to the reader, though). Thanks.
+ 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,...
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.
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
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse: clarify documentation for the --verify option
Junio C Hamano gits...@pobox.com writes:
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
...
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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse: clarify documentation for the --verify option
Michael Haggerty mhag...@alum.mit.edu writes:
... Though honestly, I don't see the point of using
--default as opposed to
$ git rev-parse --verify ${REV:-master}^{commit}
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.
--verify::
+ If the parameter can be used as a single object name, output
+ that name; otherwise, emit an error message and exit with a
+ non-zero status. Please note that the existence and validity
+ of the named object itself are not checked.
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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] rev-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 mhag...@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(-)
diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 10a116f..6095227 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -60,8 +60,10 @@ OPTIONS
instead.
--verify::
- The parameter given must be usable as a single, valid
- object name. Otherwise barf and abort.
+ If the parameter can be used as a single object name, output
+ that name; otherwise, emit an error message and exit with a
+ non-zero status. Please note that the existence and validity
+ of the named object itself are not checked.
-q::
--quiet::
@@ -308,12 +310,12 @@ $ git rev-parse --verify HEAD
* Print the commit object name from the revision in the $REV shell variable:
+
-$ git rev-parse --verify $REV
+$ git rev-parse --verify $REV^{commit}
+
This will error out if $REV is empty or not a valid revision.
-* Same as above:
+* Similar to above:
+
$ git rev-parse --default master --verify $REV
--
1.8.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
