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

From: Jeff King <peff@peff.net>
Date: May 18, 2014, 11:20 UTC
Message-ID: <20140518112020.GA2153@sigill.intra.peff.net>
In-Reply-To: <1400342542-11256-2-git-send-email-jmmahler@gmail.com>

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

Show 8 quoted lines

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

Show 7 quoted lines

> @@ -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?

Show 9 quoted lines

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

Show 6 quoted lines

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