Re: [PATCH v3] format-patch --signature-file <file>

[Jeff King <peff@peff.net>]

On Sat, May 17, 2014 at 09:02:22AM -0700, Jeremiah Mahler wrote:

> Added feature that allows a signature file to be used with format-patch.
> 
>   $ git format-patch --signature-file ~/.signature -1
> 
> Now signatures with newlines and other special characters can be
> easily included.
> 
> Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>

This is looking much better. I have a few comments still, inline below.

By the way, did you want to add a format.signaturefile config, too? I do
not care myself, but I would imagine most workflows would end up
specifying it every time.

> @@ -1447,6 +1450,16 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  			cover_letter = (config_cover_letter == COVER_ON);
>  	}
>  
> +	if (signature_file) {
> +		if (signature && signature != git_version_string)
> +			die(_("--signature and --signature-file are mutually exclusive"));

Technically "signature" might have come from config, not "--signature"
on the command-line.  But I don't know if that's even worth worrying
about; presumably the user can figure it out if they set the config.

> +		struct strbuf buf = STRBUF_INIT;
> +
> +		strbuf_read_file(&buf, signature_file, 128);
> +		signature = strbuf_detach(&buf, NULL);

Your cover letter mentioned generating an error here. Did you want:

  if (strbuf_read_file(&buf, signature_file, 128) < 0)
	die_errno("unable to read signature file '%s'", signature_file);

or similar?

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 9c80633..fb3dc1b 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -762,6 +762,34 @@ test_expect_success 'format-patch --signature="" suppresses signatures' '
>  	! grep "^-- \$" output
>  '
>  
> +cat > expect << EOF

Minor style nits, but we usually omit the whitespace between redirection
operations, and we always quote our here-doc endings unless they
explicitly want to interpolate. So:

  cat >expect <<\EOF

(we also tend to use "<<-\EOF" to drop leading tabs, and then include
them inside the test_expect_success properly indented, but as this
expectation is used in multiple places, it's not unreasonable to keep it
separate).

> +test_expect_success 'format-patch --signature-file file' '
> +	git format-patch --stdout --signature-file expect -1 >output &&
> +	check_patch output &&
> +	fgrep -x -f output expect >output2 &&

Both of these fgrep options are in POSIX, but it looks like this will be
the first use for either of them. I'm not sure if they will give us any
portability problems.

We could probably do something like:

  sed -n '/^-- $/,$p'

if we have to.

> +	diff expect output2

Please use test_cmp here, which adjusts automatically for less-abled
systems where diff is not available.

> +test_expect_success 'format-patch --signature-file=file' '
> +	git format-patch --stdout --signature-file=expect -1 >output &&
> +	check_patch output &&
> +	fgrep -x -f output expect >output2 &&
> +	diff expect output2
> +'

Same comments as above; I note that this is just checking "--foo=bar"
rather than "--foo bar". I don't mind being thorough, but for the most
part we just assume this is tested as part of the parse-options tests,
and don't check it explicitly for each option.

> +test_expect_success 'format-patch --signature and --signature-file die' '
> +	! git format-patch --stdout --signature="foo" --signature-file=expect -1 >output
> +'

Please use test_must_fail instead of "!" here; it is more thorough in
checking that we exited with a non-zero error code (and didn't die from
signal death, for example).

-Peff