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