Re: [PATCH] rev-parse: clarify documentation for the --verify option
On 04/02/2013 04:57 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >> On 04/01/2013 06:56 PM, Junio C Hamano wrote: >>> Junio C Hamano 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
Michael Haggerty writes: > On 04/01/2013 06:56 PM, Junio C Hamano wrote: >> Junio C Hamano 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/01/2013 06:56 PM, Junio C Hamano wrote: > Junio C Hamano 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
Junio C Hamano 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 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 --- 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