# [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options 8 messages from 2016-03-27 to 2016-04-08. Participants: Eric Sunshine, Junio C Hamano, Jacob Keller. Thread: https://gitlist.dev/t/41841 ## Eric Sunshine, 2016-03-27 21:26 Subject: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options Message-ID: <1459113967-13864-1-git-send-email-sunshine@sunshineco.com> URL: https://gitlist.dev/e/1459113967-13864-1-git-send-email-sunshine%40sunshineco.com ``` git-format-patch recognizes -s as shorthand only for --signoff, however, its documentation shows -s as shorthand for both --signoff and --no-patch. Resolve this confusion by suppressing the bogus -s shorthand for --no-patch. While here, also avoid showing the --no-patch option in git-format-patch documentation since it doesn't make sense to ask to suppress the patch while at the same time explicitly asking to format the patch (which, after all, is the purpose of git-format-patch). Reported-by: Kevin Brodsky Signed-off-by: Eric Sunshine --- I haven't quite managed to trace the code yet, but git-format-patch oddly does recognize --no-patch, and it appears to act as an alias of --no-stat. At any rate, --no-patch seems rather senseless with git-format-patch, hence this patch suppresses it in documentation altogether. Documentation/diff-options.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt index 306b7e3..6eb591f 100644 --- a/Documentation/diff-options.txt +++ b/Documentation/diff-options.txt @@ -28,10 +28,12 @@ ifdef::git-diff[] endif::git-diff[] endif::git-format-patch[] +ifndef::git-format-patch[] -s:: --no-patch:: Suppress diff output. Useful for commands like `git show` that show the patch by default, or to cancel the effect of `--patch`. +endif::git-format-patch[] -U:: --unified=:: -- 2.8.0.rc4.285.gc3ac548 ``` ## Eric Sunshine, 2016-04-04 17:34 Subject: Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSBNHu%3DV1awsv0Hq%3DVSg%2B_WJ%3D5iFo99du699vdh1J8hEw%40mail.gmail.com In-Reply-To: <1459113967-13864-1-git-send-email-sunshine@sunshineco.com> ``` Ping? On Sun, Mar 27, 2016 at 5:26 PM, Eric Sunshine wrote: > git-format-patch recognizes -s as shorthand only for --signoff, however, > its documentation shows -s as shorthand for both --signoff and > --no-patch. Resolve this confusion by suppressing the bogus -s shorthand > for --no-patch. > > While here, also avoid showing the --no-patch option in git-format-patch > documentation since it doesn't make sense to ask to suppress the patch > while at the same time explicitly asking to format the patch (which, > after all, is the purpose of git-format-patch). > > Reported-by: Kevin Brodsky > Signed-off-by: Eric Sunshine > --- > > I haven't quite managed to trace the code yet, but git-format-patch > oddly does recognize --no-patch, and it appears to act as an alias of > --no-stat. At any rate, --no-patch seems rather senseless with > git-format-patch, hence this patch suppresses it in documentation > altogether. > > Documentation/diff-options.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt > index 306b7e3..6eb591f 100644 > --- a/Documentation/diff-options.txt > +++ b/Documentation/diff-options.txt > @@ -28,10 +28,12 @@ ifdef::git-diff[] > endif::git-diff[] > endif::git-format-patch[] > > +ifndef::git-format-patch[] > -s:: > --no-patch:: > Suppress diff output. Useful for commands like `git show` that > show the patch by default, or to cancel the effect of `--patch`. > +endif::git-format-patch[] > > -U:: > --unified=:: > -- > 2.8.0.rc4.285.gc3ac548 ``` ## Junio C Hamano, 2016-04-04 19:32 Subject: Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options Message-ID: URL: https://gitlist.dev/e/xmqqvb3x41bo.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1459113967-13864-1-git-send-email-sunshine@sunshineco.com> ``` Eric Sunshine writes: > Documentation/diff-options.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt > index 306b7e3..6eb591f 100644 > --- a/Documentation/diff-options.txt > +++ b/Documentation/diff-options.txt > @@ -28,10 +28,12 @@ ifdef::git-diff[] > endif::git-diff[] > endif::git-format-patch[] > > +ifndef::git-format-patch[] > -s:: > --no-patch:: > Suppress diff output. Useful for commands like `git show` that > show the patch by default, or to cancel the effect of `--patch`. > +endif::git-format-patch[] Given that the ifndef/endif block immediately before this part is also about excluding -p/-u/--patch when formatting the documentation for format-patch, perhaps the attached may be a smaller equivalent? Documentation/diff-options.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt index 306b7e3..42e6620 100644 --- a/Documentation/diff-options.txt +++ b/Documentation/diff-options.txt @@ -26,12 +26,12 @@ ifndef::git-format-patch[] ifdef::git-diff[] This is the default. endif::git-diff[] -endif::git-format-patch[] -s:: --no-patch:: Suppress diff output. Useful for commands like `git show` that show the patch by default, or to cancel the effect of `--patch`. +endif::git-format-patch[] -U:: --unified=:: ``` ## Eric Sunshine, 2016-04-04 19:48 Subject: Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRMti0ayBMS2TTUc-Og%3DX-Pu2yeHxHOS74fw%3DX8BTsjhQ%40mail.gmail.com In-Reply-To: ``` On Mon, Apr 4, 2016 at 3:32 PM, Junio C Hamano wrote: > Eric Sunshine writes: >> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt >> @@ -28,10 +28,12 @@ ifdef::git-diff[] >> endif::git-diff[] >> endif::git-format-patch[] >> >> +ifndef::git-format-patch[] >> -s:: >> --no-patch:: >> Suppress diff output. Useful for commands like `git show` that >> show the patch by default, or to cancel the effect of `--patch`. >> +endif::git-format-patch[] > > Given that the ifndef/endif block immediately before this part is > also about excluding -p/-u/--patch when formatting the documentation > for format-patch, perhaps the attached may be a smaller equivalent? Perhaps. I kept self-contained to make it easier to add new options between the two if need be, but I don't feel strongly about it. > Documentation/diff-options.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt > index 306b7e3..42e6620 100644 > --- a/Documentation/diff-options.txt > +++ b/Documentation/diff-options.txt > @@ -26,12 +26,12 @@ ifndef::git-format-patch[] > ifdef::git-diff[] > This is the default. > endif::git-diff[] > -endif::git-format-patch[] > > -s:: > --no-patch:: > Suppress diff output. Useful for commands like `git show` that > show the patch by default, or to cancel the effect of `--patch`. > +endif::git-format-patch[] > > -U:: > --unified=:: ``` ## Junio C Hamano, 2016-04-04 20:07 Subject: Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options Message-ID: URL: https://gitlist.dev/e/xmqqr3el3zox.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Eric Sunshine writes: >> Given that the ifndef/endif block immediately before this part is >> also about excluding -p/-u/--patch when formatting the documentation >> for format-patch, perhaps the attached may be a smaller equivalent? > > Perhaps. I kept self-contained to make it easier to add new options > between the two if need be, but I don't feel strongly about it. I don't either, but the reason why I thought it would make sense to have them in the same block is because hiding --no-patch and --patch are about the same theme: format-patch is about presenting the diff, and neither disabling diff output nor explicitly asking for diff output makes sense. So... ``` ## Eric Sunshine, 2016-04-04 22:38 Subject: Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options Message-ID: URL: https://gitlist.dev/e/CAPig%2BcR1mmx%2B2D7U8ejx8HTWbFtxbmi-85D0hgm9J48bkai6CA%40mail.gmail.com In-Reply-To: ``` On Mon, Apr 4, 2016 at 4:07 PM, Junio C Hamano wrote: > Eric Sunshine writes: >>> Given that the ifndef/endif block immediately before this part is >>> also about excluding -p/-u/--patch when formatting the documentation >>> for format-patch, perhaps the attached may be a smaller equivalent? >> >> Perhaps. I kept self-contained to make it easier to add new options >> between the two if need be, but I don't feel strongly about it. > > I don't either, but the reason why I thought it would make sense to > have them in the same block is because hiding --no-patch and --patch > are about the same theme: format-patch is about presenting the diff, > and neither disabling diff output nor explicitly asking for diff > output makes sense. That's reasonable. Should I re-roll, or would you like to amend it locally? ``` ## Eric Sunshine, 2016-04-04 22:44 Subject: Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options Message-ID: URL: https://gitlist.dev/e/CAPig%2BcT2w_t68w5qr-bYxrgOdXcO%3DRhuazCnoHuF4J7n%2BAQi5A%40mail.gmail.com In-Reply-To: ``` On Mon, Apr 4, 2016 at 6:38 PM, Eric Sunshine wrote: > On Mon, Apr 4, 2016 at 4:07 PM, Junio C Hamano wrote: >> Eric Sunshine writes: >>>> Given that the ifndef/endif block immediately before this part is >>>> also about excluding -p/-u/--patch when formatting the documentation >>>> for format-patch, perhaps the attached may be a smaller equivalent? >>> >>> Perhaps. I kept self-contained to make it easier to add new options >>> between the two if need be, but I don't feel strongly about it. >> >> I don't either, but the reason why I thought it would make sense to >> have them in the same block is because hiding --no-patch and --patch >> are about the same theme: format-patch is about presenting the diff, >> and neither disabling diff output nor explicitly asking for diff >> output makes sense. > > That's reasonable. Should I re-roll, or would you like to amend it locally? Okay, I just pulled upon seeing "What's Cooking" and see that you amended it locally. Thanks. ``` ## Jacob Keller, 2016-04-08 18:22 Subject: Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options Message-ID: URL: https://gitlist.dev/e/CA%2BP7%2BxqzXt-uKc%3DK%3D5UPmaziApNQFL06SQbgYUdOVgZP%3DMy5kA%40mail.gmail.com In-Reply-To: ``` On Mon, Apr 4, 2016 at 12:32 PM, Junio C Hamano wrote: > Eric Sunshine writes: > >> Documentation/diff-options.txt | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt >> index 306b7e3..6eb591f 100644 >> --- a/Documentation/diff-options.txt >> +++ b/Documentation/diff-options.txt >> @@ -28,10 +28,12 @@ ifdef::git-diff[] >> endif::git-diff[] >> endif::git-format-patch[] >> >> +ifndef::git-format-patch[] >> -s:: >> --no-patch:: >> Suppress diff output. Useful for commands like `git show` that >> show the patch by default, or to cancel the effect of `--patch`. >> +endif::git-format-patch[] > > Given that the ifndef/endif block immediately before this part is > also about excluding -p/-u/--patch when formatting the documentation > for format-patch, perhaps the attached may be a smaller equivalent? > > Documentation/diff-options.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt > index 306b7e3..42e6620 100644 > --- a/Documentation/diff-options.txt > +++ b/Documentation/diff-options.txt > @@ -26,12 +26,12 @@ ifndef::git-format-patch[] > ifdef::git-diff[] > This is the default. > endif::git-diff[] > -endif::git-format-patch[] > > -s:: > --no-patch:: > Suppress diff output. Useful for commands like `git show` that > show the patch by default, or to cancel the effect of `--patch`. > +endif::git-format-patch[] > > -U:: > --unified=:: > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html This patch or another patch should also include a check to ensure that --no-patch isn't silently accepted, similar to a few other diff option arguments: (pardon if this copy below gets malformed by GMail webclient) --- diff --git i/builtin/log.c w/builtin/log.c index dff3fbbb437c..a12db191acb1 100644 --- i/builtin/log.c +++ w/builtin/log.c @@ -1384,6 +1384,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) die(_("--name-status does not make sense")); if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF) die(_("--check does not make sense")); + if (rev.diffopt.output_format & DIFF_FORMAT_NO_OUTPUT) + die(_("--no-patch does not make sense")); if (!use_patch_format && (!rev.diffopt.output_format || diff --git i/t/t4014-format-patch.sh w/t/t4014-format-patch.sh index eed2981b96df..fdcd7de7a0f0 100755 --- i/t/t4014-format-patch.sh +++ w/t/t4014-format-patch.sh @@ -691,6 +691,7 @@ test_expect_success 'format-patch --notes --signoff' ' echo "fatal: --name-only does not make sense" > expect.name-only echo "fatal: --name-status does not make sense" > expect.name-status +echo "fatal: --no-patch does not make sense" > expect.no-patch echo "fatal: --check does not make sense" > expect.check test_expect_success 'options no longer allowed for format-patch' ' @@ -698,6 +699,8 @@ test_expect_success 'options no longer allowed for format-patch' ' test_i18ncmp expect.name-only output && test_must_fail git format-patch --name-status 2> output && test_i18ncmp expect.name-status output && + test_must_fail git format-patch --no-patch 2> output && + test_i18ncmp expect.no-patch output && test_must_fail git format-patch --check 2> output && test_i18ncmp expect.check output' ```