{"thread":{"id":"33331","subject":"[PATCH] rev-parse: clarify documentation for the --verify option","startedAt":"2013-03-30T06:44:25Z","lastAt":"2013-04-02T16:28:20Z","messageCount":6,"participants":["Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"212677","messageId":"1364625865-8459-1-git-send-email-mhagger@alum.mit.edu","threadId":"33331","inReplyTo":null,"subject":"[PATCH] rev-parse: clarify documentation for the --verify option","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-03-30T06:44:25Z","receivedAt":"2013-03-30T06:44:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The old version could be read to mean that the argument has to refer\nto a valid object, but that is incorrect:\n\n* the object is not necessarily read (e.g., to check for corruption)\n\n* if the argument is a 40-digit string of hex digits, then it is\n  accepted whether or not is is the name of an existing object.\n\nSo reword the explanation to be less ambiguous.\n\nAlso fix the examples involving --verify: to be sure that the argument\nrefers to a commit (rather than some other kind of object), the\nargument has to be suffixed with \"^{commit}\".  This trick is not\npossible in the example involving --default, so don't imply that it is\nexactly the same as the previous example.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n\nThis was discussed in the following thread:\n\n    http://thread.gmane.org/gmane.comp.version-control.git/219409\n\nPlease note that the second example already reveals a fly in the\nointment:\n\n    $ git rev-parse --default master --verify $REV\n\nFollowing Junio's advice, we would like to change this to\n\n    $ git rev-parse --default master --verify $REV^{commit}\n\nbut that defeats the purpose of the --default option.  Doing\n\n    $ git rev-parse --verify $(git rev-parse --default master $REV)^{commit}\n\nis just plain ugly.  Though honestly, I don't see the point of using\n--default as opposed to\n\n    $ git rev-parse --verify ${REV:-master}^{commit}\n\nSo maybe the second example should be removed entirely or converted to\nuse ${REV:-master} rather than --default.\n\n Documentation/git-rev-parse.txt | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt\nindex 10a116f..6095227 100644\n--- a/Documentation/git-rev-parse.txt\n+++ b/Documentation/git-rev-parse.txt\n@@ -60,8 +60,10 @@ OPTIONS\n \tinstead.\n \n --verify::\n-\tThe parameter given must be usable as a single, valid\n-\tobject name.  Otherwise barf and abort.\n+\tIf the parameter can be used as a single object name, output\n+\tthat name; otherwise, emit an error message and exit with a\n+\tnon-zero status.  Please note that the existence and validity\n+\tof the named object itself are not checked.\n \n -q::\n --quiet::\n@@ -308,12 +310,12 @@ $ git rev-parse --verify HEAD\n * Print the commit object name from the revision in the $REV shell variable:\n +\n ------------\n-$ git rev-parse --verify $REV\n+$ git rev-parse --verify $REV^{commit}\n ------------\n +\n This will error out if $REV is empty or not a valid revision.\n \n-* Same as above:\n+* Similar to above:\n +\n ------------\n $ git rev-parse --default master --verify $REV\n-- \n1.8.2\n"},{"id":"212759","messageId":"7vwqsnnpwj.fsf@alter.siamese.dyndns.org","threadId":"33331","inReplyTo":"1364625865-8459-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH] rev-parse: clarify documentation for the --verify option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-31T22:52:12Z","receivedAt":"2013-03-31T22:52:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> ...  Though honestly, I don't see the point of using\n> --default as opposed to\n>\n>     $ git rev-parse --verify ${REV:-master}^{commit}\n\nI would agree ${VAR:-default} is sufficient in that particular case.\n\nThe --default is more about the use of the pluming command not with\n--verify but as its original use of an \"argument sifter\" when\ncomposing \"git rev-list\" piped into \"git diff-tree --stdin\", i.e.\n\n\tgit rev-list $(git rev-parse --default HEAD --revs-only \"$@\") |\n        git diff-tree --stdin $(git rev-parse --no-revs \"$@\")\n\nwhich was the original way to write commands in the \"git log\" family\nusing the plumbing command as a scripted Porcelain.\n\n>  --verify::\n> +\tIf the parameter can be used as a single object name, output\n> +\tthat name; otherwise, emit an error message and exit with a\n> +\tnon-zero status.  Please note that the existence and validity\n> +\tof the named object itself are not checked.\n\nPerhaps s/used as a single object name/turned into a raw 20-byte SHA-1/;\n\nBecause the primary use case of this option is to implement end-user\ninput validation, I think it would be helpful to clarify use of the\npeeler here.  Perhaps\n\n    --verify::\n            Make sure the single given parameter can be turned into a\n            raw 20-byte SHA-1, something that can be used to access the\n            object database, and emit the SHA-1 in 40-hex format (unless\n            --symbolic and other formatting option is given); otherwise,\n            error out.\n    +\n    If you want to make sure that the output from this actually\n    names an object in your object database and/or can be used\n    as a specific type of object you require, it is a good idea\n    to add \"^{type}\" peeling operator to the parmeter.  For\n    example, `git rev-parse \"$VAR^{commit}\"` will make sure $VAR\n    names an existing object that is a commit-ish (i.e. a\n    commit, or an annotated tag that points at a commit).  To\n    make sure that $VAR names an existing object of any type,\n    you can say `git rev-parse \"$VAR^{object}\"`.\n\nor something.\n"},{"id":"212802","messageId":"7vli92jijz.fsf@alter.siamese.dyndns.org","threadId":"33331","inReplyTo":"7vwqsnnpwj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rev-parse: clarify documentation for the --verify option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-01T16:56:48Z","receivedAt":"2013-04-01T16:56:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Because the primary use case of this option is to implement end-user\n> input validation, I think it would be helpful to clarify use of the\n> peeler here.  Perhaps\n> ...\n\nA \"SQUASH???\" patch on top of your original is queued on 'pu',\ntogether with the earlier \"^{object}\" peeler patch.  Comments,\nimprovements, etc. would be nice.\n\nThanks.\n"},{"id":"212860","messageId":"515A8D79.6050808@alum.mit.edu","threadId":"33331","inReplyTo":"7vli92jijz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rev-parse: clarify documentation for the --verify option","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-02T07:49:13Z","receivedAt":"2013-04-02T07:49:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/01/2013 06:56 PM, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> Because the primary use case of this option is to implement end-user\n>> input validation, I think it would be helpful to clarify use of the\n>> peeler here.  Perhaps\n>> ...\n> \n> A \"SQUASH???\" patch on top of your original is queued on 'pu',\n> together with the earlier \"^{object}\" peeler patch.  Comments,\n> improvements, etc. would be nice.\n\nYes, your version is better.  I would make one change, though.  In your\n\n+\tMake sure the single given parameter can be turned into a\n+\traw 20-byte SHA-1 that can be used to access the object\n+\tdatabase, and emit it to the standard output. If it can't,\n+\terror out.\n\nit could be made clearer that exactly one parameter should be provided.\n Maybe\n\n+\tVerify that exactly one parameter is provided, and that it\n+       can be turned into a raw 20-byte SHA-1 that can be used to\n+\taccess the object database.  If so, emit the SHA-1 to the\n+\tstandard output; otherwise, error out.\n\nBut this makes it sound a little like the \"raw 20-byte SHA-1\" will be\noutput to stdout, whereas both the input and the output are in fact\n40-character hex-encoded SHA-1s.  Perhaps a further change\n\n    s/raw 20-byte SHA-1/full SHA-1/\n\nwould avoid the false implication?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"212886","messageId":"7vobdxgeud.fsf@alter.siamese.dyndns.org","threadId":"33331","inReplyTo":"515A8D79.6050808@alum.mit.edu","subject":"Re: [PATCH] rev-parse: clarify documentation for the --verify option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-02T14:57:30Z","receivedAt":"2013-04-02T14:57:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> On 04/01/2013 06:56 PM, Junio C Hamano wrote:\n>> Junio C Hamano <gitster@pobox.com> writes:\n>> \n>>> Because the primary use case of this option is to implement end-user\n>>> input validation, I think it would be helpful to clarify use of the\n>>> peeler here.  Perhaps\n>>> ...\n>> \n>> A \"SQUASH???\" patch on top of your original is queued on 'pu',\n>> together with the earlier \"^{object}\" peeler patch.  Comments,\n>> improvements, etc. would be nice.\n>\n> Yes, your version is better.  I would make one change, though.  In your\n>\n> +\tMake sure the single given parameter can be turned into a\n> +\traw 20-byte SHA-1 that can be used to access the object\n> +\tdatabase, and emit it to the standard output. If it can't,\n> +\terror out.\n>\n> it could be made clearer that exactly one parameter should be provided.\n> Maybe\n>\n> +\tVerify that exactly one parameter is provided, and that it\n\nThat is probably better (I was hoping \"the single\" would mean the\nsame to the reader, though).  Thanks.\n\n> +     can be turned into a raw 20-byte SHA-1 that can be used to\n> +\taccess the object database.  If so, emit the SHA-1 to the\n> +\tstandard output; otherwise, error out.\n>\n> But this makes it sound a little like the \"raw 20-byte SHA-1\" will be\n> output to stdout,...\n\nI did consider that point, wrote \"and outputs 40-hex\" in my earlier\ndraft, and then rejected it because it was even more misleading.\nThe output follows the usual rules for \"rev\" parameters, e.g.\n\n\tgit rev-parse --short --verify HEAD\n\tgit rev-parse --symbolic --verify v1.8.2^{tree}\n\nand \"--verify\" does not mean 40-hex output.  That is why I left it\nvague as \"emit it\".\n\nI agree that the wording incorrectly hints that you may be able to\nget 20-byte raw output.  I didn't find a satisfactory phrasing.\n"},{"id":"212897","messageId":"515B0724.3060702@alum.mit.edu","threadId":"33331","inReplyTo":"7vobdxgeud.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rev-parse: clarify documentation for the --verify option","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-02T16:28:20Z","receivedAt":"2013-04-02T16:28:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/02/2013 04:57 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> On 04/01/2013 06:56 PM, Junio C Hamano wrote:\n>>> Junio C Hamano <gitster@pobox.com> writes:\n>>>\n>>>> Because the primary use case of this option is to implement end-user\n>>>> input validation, I think it would be helpful to clarify use of the\n>>>> peeler here.  Perhaps\n>>>> ...\n>>>\n>>> A \"SQUASH???\" patch on top of your original is queued on 'pu',\n>>> together with the earlier \"^{object}\" peeler patch.  Comments,\n>>> improvements, etc. would be nice.\n>>\n>> Yes, your version is better.  I would make one change, though.  In your\n>>\n>> +\tMake sure the single given parameter can be turned into a\n>> +\traw 20-byte SHA-1 that can be used to access the object\n>> +\tdatabase, and emit it to the standard output. If it can't,\n>> +\terror out.\n>>\n>> it could be made clearer that exactly one parameter should be provided.\n>> Maybe\n>>\n>> +\tVerify that exactly one parameter is provided, and that it\n> \n> That is probably better (I was hoping \"the single\" would mean the\n> same to the reader, though).  Thanks.\n> \n>> +     can be turned into a raw 20-byte SHA-1 that can be used to\n>> +\taccess the object database.  If so, emit the SHA-1 to the\n>> +\tstandard output; otherwise, error out.\n>>\n>> But this makes it sound a little like the \"raw 20-byte SHA-1\" will be\n>> output to stdout,...\n> \n> I did consider that point, wrote \"and outputs 40-hex\" in my earlier\n> draft, and then rejected it because it was even more misleading.\n> The output follows the usual rules for \"rev\" parameters, e.g.\n> \n> \tgit rev-parse --short --verify HEAD\n> \tgit rev-parse --symbolic --verify v1.8.2^{tree}\n> \n> and \"--verify\" does not mean 40-hex output.  That is why I left it\n> vague as \"emit it\".\n> \n> I agree that the wording incorrectly hints that you may be able to\n> get 20-byte raw output.  I didn't find a satisfactory phrasing.\n\nIt's the explicit mention of \"raw 20-byte\" that puts the reader in mind\nof 20-byte binary data.  I think any version that omitted that phrase\nwould let the reader make the assumption that the SHA-1s are expressed\nas 40-byte hex numbers just they are everywhere else in the command-line\ninterface.\n\nBut I'm OK with any of the variations that we have discussed.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}