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

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

[Eric Sunshine]

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

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

[Eric Sunshine]

Ping?

On Sun, Mar 27, 2016 at 5:26 PM, Eric Sunshine <sunshine@sunshineco.com> 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 <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

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

[Junio C Hamano]

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>::

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

[Eric Sunshine]

On Mon, Apr 4, 2016 at 3:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

>  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>::

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

[Junio C Hamano]

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.

So...

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

[Eric Sunshine]

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?

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

[Eric Sunshine]

On Mon, Apr 4, 2016 at 6:38 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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.

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

[Jacob Keller]

On Mon, Apr 4, 2016 at 12:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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)

---

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'