Re: [PATCH v8 2/2] format-patch --signature-file <file>

From: Junio C Hamano <gitster@pobox.com>
Date: May 22, 2014, 20:52 UTC
Message-ID: <xmqqtx8hmt7m.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1400723589-8975-3-git-send-email-jmmahler@gmail.com>

Jeremiah Mahler <jmmahler@gmail.com> writes:

> Added option that allows a signature file to be used with format-patch
> so that signatures with newlines and other special characters can be
> easily included.

s/Added option/Add an option/. I do not think "with newlines and other special characters" is the primary issue---isn't it more about "I have chosen to use this mail-signature; do not force me to retype the same all the time"?

>   $ git format-patch --signature-file ~/.signature -1

The recommended command-line convention (see gitcli(7)) is to use "--option=value", so an example would be better to follow it, i.e.

    $ git format-patch -1 --signature-file=$HOME/.signature

Show 5 quoted lines

> The config variable format.signaturefile is also provided so that it
> can be added by default.
>
>   $ git config format.signaturefile ~/.signature
>   $ git format-patch -1

Something like:

    To countermand the configuration variable for a specific run:

	$ git format-patch -1 --signature="This time only"
        $ git format-patch -1 --signature    ;# to use the default
        $ git format-patch -1 --signature="" ;# to add nothing

is also needed here, I think. Similarly, these two needs to be tested in the test scripts you are modifying. Specifically:

> +test_expect_success 'format-patch --no-signature and --signature-file OK' '
> +	git format-patch --stdout --no-signature --signature-file=mail-signature -1
> +'

should not just make sure "format-patch" does _something_, but needs to make sure it does not contain the contents of the configured mail signagture file.

I didn't see offhand if the tests make sure that a configured mail signature can be overriden from the command line. I think you would want to test, with format-patch.signature-file pointing at the mail-signature file, at least these three cases:

 - Run "format-patch --no-signature" and make sure that stops the
   contents from mail-signature file from being shown, and instead
   no mail-signature is given.

 - Run "format-patch --signature='this time only'" and make sure
   that stops the contents from mail-signature file from being shown
   and "this time only" is used instead.

 - Run "format-patch --signature-file=another-mail-signature" and
   make sure that stops the contents from mail-signature file from
   being shown and the contents from the other file is used instead.

Test for these "negative cases" is often what we forget, when we are thrilled to show off that the shiny new feature works as expected. We need to ensure that the ways to stop the shiny new feature from kicking in will not be broken as well.

Thanks.