{"thread":{"id":"41841","subject":"[PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options","startedAt":"2016-03-27T21:26:07Z","lastAt":"2016-04-08T18:22:23Z","messageCount":8,"participants":["Eric Sunshine","Junio C Hamano","Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"281946","messageId":"1459113967-13864-1-git-send-email-sunshine@sunshineco.com","threadId":"41841","inReplyTo":null,"subject":"[PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-27T21:26:07Z","receivedAt":"2016-03-27T21:26:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"git-format-patch recognizes -s as shorthand only for --signoff, however,\nits documentation shows -s as shorthand for both --signoff and\n--no-patch. Resolve this confusion by suppressing the bogus -s shorthand\nfor --no-patch.\n\nWhile here, also avoid showing the --no-patch option in git-format-patch\ndocumentation since it doesn't make sense to ask to suppress the patch\nwhile at the same time explicitly asking to format the patch (which,\nafter all, is the purpose of git-format-patch).\n\nReported-by: Kevin Brodsky \nSigned-off-by: Eric Sunshine \n---\n\nI haven't quite managed to trace the code yet, but git-format-patch\noddly does recognize --no-patch, and it appears to act as an alias of\n--no-stat. At any rate, --no-patch seems rather senseless with\ngit-format-patch, hence this patch suppresses it in documentation\naltogether.\n\nDocumentation/diff-options.txt | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 306b7e3..6eb591f 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -28,10 +28,12 @@ ifdef::git-diff[]\n endif::git-diff[]\n endif::git-format-patch[]\n \n+ifndef::git-format-patch[]\n -s::\n --no-patch::\n \tSuppress diff output. Useful for commands like `git show` that\n \tshow the patch by default, or to cancel the effect of `--patch`.\n+endif::git-format-patch[]\n \n -U::\n --unified=::\n-- \n2.8.0.rc4.285.gc3ac548\n"},{"id":"282628","messageId":"CAPig+cSBNHu=V1awsv0Hq=VSg+_WJ=5iFo99du699vdh1J8hEw@mail.gmail.com","threadId":"41841","inReplyTo":"1459113967-13864-1-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-04T17:34:21Z","receivedAt":"2016-04-04T17:34:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Ping?\n\nOn Sun, Mar 27, 2016 at 5:26 PM, Eric Sunshine wrote:\n> git-format-patch recognizes -s as shorthand only for --signoff, however,\n> its documentation shows -s as shorthand for both --signoff and\n> --no-patch. Resolve this confusion by suppressing the bogus -s shorthand\n> for --no-patch.\n>\n> While here, also avoid showing the --no-patch option in git-format-patch\n> documentation since it doesn't make sense to ask to suppress the patch\n> while at the same time explicitly asking to format the patch (which,\n> after all, is the purpose of git-format-patch).\n>\n> Reported-by: Kevin Brodsky \n> Signed-off-by: Eric Sunshine \n> ---\n>\n> I haven't quite managed to trace the code yet, but git-format-patch\n> oddly does recognize --no-patch, and it appears to act as an alias of\n> --no-stat. At any rate, --no-patch seems rather senseless with\n> git-format-patch, hence this patch suppresses it in documentation\n> altogether.\n>\n> Documentation/diff-options.txt | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 306b7e3..6eb591f 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -28,10 +28,12 @@ ifdef::git-diff[]\n> endif::git-diff[]\n> endif::git-format-patch[]\n>\n> +ifndef::git-format-patch[]\n> -s::\n> --no-patch::\n> Suppress diff output. Useful for commands like `git show` that\n> show the patch by default, or to cancel the effect of `--patch`.\n> +endif::git-format-patch[]\n>\n> -U::\n> --unified=::\n> --\n> 2.8.0.rc4.285.gc3ac548\n"},{"id":"282640","messageId":"xmqqvb3x41bo.fsf@gitster.mtv.corp.google.com","threadId":"41841","inReplyTo":"1459113967-13864-1-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-04T19:32:27Z","receivedAt":"2016-04-04T19:32:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Documentation/diff-options.txt | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 306b7e3..6eb591f 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -28,10 +28,12 @@ ifdef::git-diff[]\n> endif::git-diff[]\n> endif::git-format-patch[]\n> \n> +ifndef::git-format-patch[]\n> -s::\n> --no-patch::\n> \tSuppress diff output. Useful for commands like `git show` that\n> \tshow the patch by default, or to cancel the effect of `--patch`.\n> +endif::git-format-patch[]\n\nGiven that the ifndef/endif block immediately before this part is\nalso about excluding -p/-u/--patch when formatting the documentation\nfor format-patch, perhaps the attached may be a smaller equivalent?\n\n Documentation/diff-options.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 306b7e3..42e6620 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -26,12 +26,12 @@ ifndef::git-format-patch[]\n ifdef::git-diff[]\n \tThis is the default.\n endif::git-diff[]\n-endif::git-format-patch[]\n \n -s::\n --no-patch::\n \tSuppress diff output. Useful for commands like `git show` that\n \tshow the patch by default, or to cancel the effect of `--patch`.\n+endif::git-format-patch[]\n \n -U::\n --unified=::\n"},{"id":"282641","messageId":"CAPig+cRMti0ayBMS2TTUc-Og=X-Pu2yeHxHOS74fw=X8BTsjhQ@mail.gmail.com","threadId":"41841","inReplyTo":"xmqqvb3x41bo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-04T19:48:57Z","receivedAt":"2016-04-04T19:48:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 4, 2016 at 3:32 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n>> @@ -28,10 +28,12 @@ ifdef::git-diff[]\n>> endif::git-diff[]\n>> endif::git-format-patch[]\n>>\n>> +ifndef::git-format-patch[]\n>> -s::\n>> --no-patch::\n>> Suppress diff output. Useful for commands like `git show` that\n>> show the patch by default, or to cancel the effect of `--patch`.\n>> +endif::git-format-patch[]\n>\n> Given that the ifndef/endif block immediately before this part is\n> also about excluding -p/-u/--patch when formatting the documentation\n> for format-patch, perhaps the attached may be a smaller equivalent?\n\nPerhaps. I kept self-contained to make it easier to add new options\nbetween the two if need be, but I don't feel strongly about it.\n\n> Documentation/diff-options.txt | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 306b7e3..42e6620 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -26,12 +26,12 @@ ifndef::git-format-patch[]\n> ifdef::git-diff[]\n> This is the default.\n> endif::git-diff[]\n> -endif::git-format-patch[]\n>\n> -s::\n> --no-patch::\n> Suppress diff output. Useful for commands like `git show` that\n> show the patch by default, or to cancel the effect of `--patch`.\n> +endif::git-format-patch[]\n>\n> -U::\n> --unified=::\n"},{"id":"282642","messageId":"xmqqr3el3zox.fsf@gitster.mtv.corp.google.com","threadId":"41841","inReplyTo":"CAPig+cRMti0ayBMS2TTUc-Og=X-Pu2yeHxHOS74fw=X8BTsjhQ@mail.gmail.com","subject":"Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-04T20:07:42Z","receivedAt":"2016-04-04T20:07:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> Given that the ifndef/endif block immediately before this part is\n>> also about excluding -p/-u/--patch when formatting the documentation\n>> for format-patch, perhaps the attached may be a smaller equivalent?\n>\n> Perhaps. I kept self-contained to make it easier to add new options\n> between the two if need be, but I don't feel strongly about it.\n\nI don't either, but the reason why I thought it would make sense to\nhave them in the same block is because hiding --no-patch and --patch\nare about the same theme: format-patch is about presenting the diff,\nand neither disabling diff output nor explicitly asking for diff\noutput makes sense.\n\nSo...\n"},{"id":"282659","messageId":"CAPig+cR1mmx+2D7U8ejx8HTWbFtxbmi-85D0hgm9J48bkai6CA@mail.gmail.com","threadId":"41841","inReplyTo":"xmqqr3el3zox.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-04T22:38:47Z","receivedAt":"2016-04-04T22:38:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 4, 2016 at 4:07 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>>> Given that the ifndef/endif block immediately before this part is\n>>> also about excluding -p/-u/--patch when formatting the documentation\n>>> for format-patch, perhaps the attached may be a smaller equivalent?\n>>\n>> Perhaps. I kept self-contained to make it easier to add new options\n>> between the two if need be, but I don't feel strongly about it.\n>\n> I don't either, but the reason why I thought it would make sense to\n> have them in the same block is because hiding --no-patch and --patch\n> are about the same theme: format-patch is about presenting the diff,\n> and neither disabling diff output nor explicitly asking for diff\n> output makes sense.\n\nThat's reasonable. Should I re-roll, or would you like to amend it locally?\n"},{"id":"282660","messageId":"CAPig+cT2w_t68w5qr-bYxrgOdXcO=RhuazCnoHuF4J7n+AQi5A@mail.gmail.com","threadId":"41841","inReplyTo":"CAPig+cR1mmx+2D7U8ejx8HTWbFtxbmi-85D0hgm9J48bkai6CA@mail.gmail.com","subject":"Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-04T22:44:14Z","receivedAt":"2016-04-04T22:44:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 4, 2016 at 6:38 PM, Eric Sunshine wrote:\n> On Mon, Apr 4, 2016 at 4:07 PM, Junio C Hamano wrote:\n>> Eric Sunshine writes:\n>>>> Given that the ifndef/endif block immediately before this part is\n>>>> also about excluding -p/-u/--patch when formatting the documentation\n>>>> for format-patch, perhaps the attached may be a smaller equivalent?\n>>>\n>>> Perhaps. I kept self-contained to make it easier to add new options\n>>> between the two if need be, but I don't feel strongly about it.\n>>\n>> I don't either, but the reason why I thought it would make sense to\n>> have them in the same block is because hiding --no-patch and --patch\n>> are about the same theme: format-patch is about presenting the diff,\n>> and neither disabling diff output nor explicitly asking for diff\n>> output makes sense.\n>\n> That's reasonable. Should I re-roll, or would you like to amend it locally?\n\nOkay, I just pulled upon seeing \"What's Cooking\" and see that you\namended it locally. Thanks.\n"},{"id":"282972","messageId":"CA+P7+xqzXt-uKc=K=5UPmaziApNQFL06SQbgYUdOVgZP=My5kA@mail.gmail.com","threadId":"41841","inReplyTo":"xmqqvb3x41bo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-04-08T18:22:23Z","receivedAt":"2016-04-08T18:22:23Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Apr 4, 2016 at 12:32 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>> Documentation/diff-options.txt | 2 ++\n>> 1 file changed, 2 insertions(+)\n>>\n>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n>> index 306b7e3..6eb591f 100644\n>> --- a/Documentation/diff-options.txt\n>> +++ b/Documentation/diff-options.txt\n>> @@ -28,10 +28,12 @@ ifdef::git-diff[]\n>> endif::git-diff[]\n>> endif::git-format-patch[]\n>>\n>> +ifndef::git-format-patch[]\n>> -s::\n>> --no-patch::\n>> Suppress diff output. Useful for commands like `git show` that\n>> show the patch by default, or to cancel the effect of `--patch`.\n>> +endif::git-format-patch[]\n>\n> Given that the ifndef/endif block immediately before this part is\n> also about excluding -p/-u/--patch when formatting the documentation\n> for format-patch, perhaps the attached may be a smaller equivalent?\n>\n> Documentation/diff-options.txt | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 306b7e3..42e6620 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -26,12 +26,12 @@ ifndef::git-format-patch[]\n> ifdef::git-diff[]\n> This is the default.\n> endif::git-diff[]\n> -endif::git-format-patch[]\n>\n> -s::\n> --no-patch::\n> Suppress diff output. Useful for commands like `git show` that\n> show the patch by default, or to cancel the effect of `--patch`.\n> +endif::git-format-patch[]\n>\n> -U::\n> --unified=::\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n\nThis patch or another patch should also include a check to ensure that\n--no-patch isn't silently accepted, similar to a few other diff option\narguments: (pardon if this copy below gets malformed by GMail\nwebclient)\n\n---\n\ndiff --git i/builtin/log.c w/builtin/log.c\nindex dff3fbbb437c..a12db191acb1 100644\n--- i/builtin/log.c\n+++ w/builtin/log.c\n@@ -1384,6 +1384,8 @@ int cmd_format_patch(int argc, const char\n**argv, const char *prefix)\n die(_(\"--name-status does not make sense\"));\n if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)\n die(_(\"--check does not make sense\"));\n+ if (rev.diffopt.output_format & DIFF_FORMAT_NO_OUTPUT)\n+ die(_(\"--no-patch does not make sense\"));\n\n if (!use_patch_format &&\n (!rev.diffopt.output_format ||\ndiff --git i/t/t4014-format-patch.sh w/t/t4014-format-patch.sh\nindex eed2981b96df..fdcd7de7a0f0 100755\n--- i/t/t4014-format-patch.sh\n+++ w/t/t4014-format-patch.sh\n@@ -691,6 +691,7 @@ test_expect_success 'format-patch --notes --signoff' '\n\n echo \"fatal: --name-only does not make sense\" > expect.name-only\n echo \"fatal: --name-status does not make sense\" > expect.name-status\n+echo \"fatal: --no-patch does not make sense\" > expect.no-patch\n echo \"fatal: --check does not make sense\" > expect.check\n\n test_expect_success 'options no longer allowed for format-patch' '\n@@ -698,6 +699,8 @@ test_expect_success 'options no longer allowed for\nformat-patch' '\n test_i18ncmp expect.name-only output &&\n test_must_fail git format-patch --name-status 2> output &&\n test_i18ncmp expect.name-status output &&\n+ test_must_fail git format-patch --no-patch 2> output &&\n+ test_i18ncmp expect.no-patch output &&\n test_must_fail git format-patch --check 2> output &&\n test_i18ncmp expect.check output'\n"}]}