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

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

[Jeremiah Mahler]

v3 patch to add format-patch --signature-file <file> option.

This revision includes changes suggested by Jeff King:

  - Instead of a custom OPTION_CALLBACK, use OPT_FILENAME which
	correctly resolves file names which are not evaluated by the
	shell such as "--signature-file=<file>"

  - Use strbuf_read_file() which eliminates the arbitrary size limit
	and simplifies the code.

  - Generate an error if the file cannot be read instead of quietly
	continuing.  This is accomplished with strbuf_read_file().

  - Die if both --signature and --signature-file are used.

Jeremiah Mahler (1):
  format-patch --signature-file <file>

 Documentation/git-format-patch.txt |  4 ++++
 builtin/log.c                      | 13 +++++++++++++
 t/t4014-format-patch.sh            | 28 ++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+)

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

[Jeremiah Mahler]

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>
---
 Documentation/git-format-patch.txt |  4 ++++
 builtin/log.c                      | 13 +++++++++++++
 t/t4014-format-patch.sh            | 28 ++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 5c0a4ab..c0fd470 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -14,6 +14,7 @@ SYNOPSIS
 		   [(--attach|--inline)[=<boundary>] | --no-attach]
 		   [-s | --signoff]
 		   [--signature=<signature> | --no-signature]
+		   [--signature-file=<file>]
 		   [-n | --numbered | -N | --no-numbered]
 		   [--start-number <n>] [--numbered-files]
 		   [--in-reply-to=Message-Id] [--suffix=.<sfx>]
@@ -233,6 +234,9 @@ configuration options in linkgit:git-notes[1] to use this workflow).
 	signature option is omitted the signature defaults to the Git version
 	number.
 
+--signature-file=<file>::
+	Works just like --signature except the signature is read from a file.
+
 --suffix=.<sfx>::
 	Instead of using `.patch` as the suffix for generated
 	filenames, use specified suffix.  A common alternative is
diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..af7d610 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -673,6 +673,7 @@ static void add_header(const char *value)
 static int thread;
 static int do_signoff;
 static const char *signature = git_version_string;
+static const char *signature_file;
 static int config_cover_letter;
 
 enum {
@@ -1230,6 +1231,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    PARSE_OPT_OPTARG, thread_callback },
 		OPT_STRING(0, "signature", &signature, N_("signature"),
 			    N_("add a signature")),
+		OPT_FILENAME(0, "signature-file", &signature_file,
+				N_("add a signature from a file")),
 		OPT__QUIET(&quiet, N_("don't print the patch filenames")),
 		OPT_END()
 	};
@@ -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"));
+
+		struct strbuf buf = STRBUF_INIT;
+
+		strbuf_read_file(&buf, signature_file, 128);
+		signature = strbuf_detach(&buf, NULL);
+	}
+
 	if (in_reply_to || thread || cover_letter)
 		rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
 	if (in_reply_to) {
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
+Test User <test.email@kernel.org>
+http://git.kernel.org/cgit/git/git.git
+git.kernel.org/?p=git/git.git;a=summary
+EOF
+
+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
+'
+
+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
+'
+
+test_expect_success 'format-patch --signature and --signature-file die' '
+	! git format-patch --stdout --signature="foo" --signature-file=expect -1 >output
+'
+
+test_expect_success 'format-patch --no-signature and --signature-file OK' '
+	git format-patch --stdout --no-signature --signature-file=expect -1 >output
+'
+
 test_expect_success TTY 'format-patch --stdout paginates' '
 	rm -f pager_used &&
 	test_terminal env GIT_PAGER="wc >pager_used" git format-patch --stdout --all &&

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

[Jeff King]

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

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

[Jeremiah Mahler]

Peff,

Lots of good suggestions.  I had to expand upon the signature
pattern match to get to work.

On Sun, May 18, 2014 at 07:20:20AM -0400, Jeff King wrote:
> On Sat, May 17, 2014 at 09:02:22AM -0700, Jeremiah Mahler wrote:
> 
...
> > +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'
> 

This gets the signature out but it will have '--' and
some trailing blank lines which were not in the original signature.
So then test_cmp won't work directly.

What I came up with was to use head and tail to remove the first line
and the last two lines.  Then test_cmp can be used normally.

test_expect_success 'format-patch --signature-file=file' '
	git format-patch --stdout --signature-file=expect -1 >output &&
	check_patch_output output &&
	sed -n "/^-- $/,\$p" <output | head --lines=-2 | tail --lines=+2 >output2 &&
	test_cmp expect output2
'

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler