threads / patch / 41841

patchgit-format-patch.txt: don't show -s as shorthand for multiple options

Subject: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options

## tl;dr

8 messages between Mar 27, 2016 and Apr 8, 2016. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Eric Sunshine· Mar 27, 2016, 21:26 UTC · lore

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 <corax26@gmail.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

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(+)

Show changes to Documentation/diff-options.txt +2 −0

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<n>::
 --unified=<n>::

-- 
2.8.0.rc4.285.gc3ac548

Eric Sunshine· Apr 4, 2016, 17:34 UTC · re: Eric Sunshine · lore

Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options

Ping?

On Sun, Mar 27, 2016 at 5:26 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 42 quoted lines

> 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 <corax26@gmail.com>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>
> 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<n>::
>  --unified=<n>::
> --
> 2.8.0.rc4.285.gc3ac548

Junio C Hamano· Apr 4, 2016, 19:32 UTC · re: Eric Sunshine · lore

Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 17 quoted lines

> 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(-)

Show changes to Documentation/diff-options.txt +1 −1

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<n>::
 --unified=<n>::

Eric Sunshine· Apr 4, 2016, 19:48 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options

On Mon, Apr 4, 2016 at 3:32 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> 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.

Show 21 quoted lines

>  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<n>::
>  --unified=<n>::

Junio C Hamano· Apr 4, 2016, 20:07 UTC · re: Eric Sunshine · lore

Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 6 quoted lines

>> 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· Apr 4, 2016, 22:38 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options

On Mon, Apr 4, 2016 at 4:07 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> 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· Apr 4, 2016, 22:44 UTC · re: Eric Sunshine · lore

Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options

On Mon, Apr 4, 2016 at 6:38 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 16 quoted lines

> On Mon, Apr 4, 2016 at 4:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> 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· Apr 8, 2016, 18:22 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options

On Mon, Apr 4, 2016 at 12:32 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 49 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> 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<n>::
>  --unified=<n>::
> --
> 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)

---

Show changes to diff +5 −0

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'