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

From: Jacob Keller <jacob.keller@gmail.com>
Date: Apr 8, 2016, 18:22 UTC
Message-ID: <CA+P7+xqzXt-uKc=K=5UPmaziApNQFL06SQbgYUdOVgZP=My5kA@mail.gmail.com>
In-Reply-To: <xmqqvb3x41bo.fsf@gitster.mtv.corp.google.com>

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)

---

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'