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

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

[Jeremiah Mahler]

v8 of patch to add format-patch --signature-file <file> option.

This revision includes more suggestions from Jeff King and Junio C Hamano.

  - Renamed variable which stores the signature to 'mail-signature'
	which better describes what is being stored.

  - Fixed a declaration after code (forbidden by ISO C90).

  - Simplified sed usage in test cases from a pipeline with
    three stages to just one.

  - Enclosed the creation of test data ('cat >mail-signature') inside
	a test_expect_* case.

  - Test cases now add one blank line to the actual signature to match
  	the signature output of Git.  Previously, the last of the output
	was deleted.

Thanks again for all the suggestions.

Jeff King (1):
  format-patch: make newline after signature conditional

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

 Documentation/config.txt           |  4 ++++
 Documentation/git-format-patch.txt |  4 ++++
 builtin/log.c                      | 25 +++++++++++++++++++++--
 t/t4014-format-patch.sh            | 41 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 2 deletions(-)

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

[PATCH v8 1/2] format-patch: make newline after signature conditional

[Jeremiah Mahler]

From: Jeff King <peff@peff.net>

When we print an email signature, we print the divider "--
\n", then the signature string, then two newlines.
Traditionally the signature is a one-liner (and the default
is just the git version), so the extra newline makes sense.

But one could easily specify a longer, multi-line signature,
like:

  git format-patch --signature='
  this is my long signature

  it has multiple lines
  ' ...

We should notice that it already has its own trailing
newline, and suppress one of ours.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 builtin/log.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..5acc048 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -844,8 +844,13 @@ static void gen_message_id(struct rev_info *info, char *base)
 
 static void print_signature(void)
 {
-	if (signature && *signature)
-		printf("-- \n%s\n\n", signature);
+	if (!signature || !*signature)
+		return;
+
+	printf("-- \n%s", signature);
+	if (signature[strlen(signature)-1] != '\n')
+		putchar('\n');
+	putchar('\n');
 }
 
 static void add_branch_description(struct strbuf *buf, const char *branch_name)
-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

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

[Jeremiah Mahler]

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.

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

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 Documentation/config.txt           |  4 ++++
 Documentation/git-format-patch.txt |  4 ++++
 builtin/log.c                      | 16 +++++++++++++++
 t/t4014-format-patch.sh            | 41 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..140ed77 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1114,6 +1114,10 @@ format.signature::
 	Set this variable to the empty string ("") to suppress
 	signature generation.
 
+format.signaturefile::
+	Works just like format.signature except the contents of the
+	file specified by this variable will be used as the signature.
+
 format.suffix::
 	The default for format-patch is to output files with the suffix
 	`.patch`. Use this variable to change that suffix (make sure to
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 5acc048..5e3cc29 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 {
@@ -742,6 +743,8 @@ static int git_format_config(const char *var, const char *value, void *cb)
 	}
 	if (!strcmp(var, "format.signature"))
 		return git_config_string(&signature, var, value);
+	if (!strcmp(var, "format.signaturefile"))
+		return git_config_pathname(&signature_file, var, value);
 	if (!strcmp(var, "format.coverletter")) {
 		if (value && !strcasecmp(value, "auto")) {
 			config_cover_letter = COVER_AUTO;
@@ -1235,6 +1238,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()
 	};
@@ -1452,6 +1457,17 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			cover_letter = (config_cover_letter == COVER_ON);
 	}
 
+	if (signature_file) {
+		struct strbuf buf = STRBUF_INIT;
+
+		if (signature && signature != git_version_string)
+			die(_("cannot specify both signature and signature-file"));
+
+		if (strbuf_read_file(&buf, signature_file, 128) < 0)
+			die_errno(_("unable to read signature file '%s'"), signature_file);
+		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..37d25c4 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -762,6 +762,47 @@ test_expect_success 'format-patch --signature="" suppresses signatures' '
 	! grep "^-- \$" output
 '
 
+test_expect_success 'prepare mail-signature input' '
+	cat >mail-signature <<-\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=mail-signature' '
+	git format-patch --stdout --signature-file=mail-signature -1 >output &&
+	check_patch output &&
+	sed -e "1,/^-- \$/d" <output >actual &&
+	{
+		cat mail-signature && echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'format-patch with format.signaturefile config' '
+	test_config format.signaturefile mail-signature &&
+	git format-patch --stdout -1 >output &&
+	check_patch output &&
+	sed -e "1,/^-- \$/d" <output >actual &&
+	{
+		cat mail-signature && echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'format-patch --signature and --signature-file die' '
+	test_must_fail git format-patch --stdout --signature="foo" \
+		--signature-file=mail-signature -1 >output
+'
+
+test_expect_success 'format-patch --no-signature and --signature-file OK' '
+	git format-patch --stdout --no-signature --signature-file=mail-signature -1
+'
+
 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 &&
-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

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

[Jeremiah Mahler]

Junio,

On Wed, May 21, 2014 at 06:53:07PM -0700, Jeremiah Mahler wrote:
> v8 of patch to add format-patch --signature-file <file> option.
> 
...
> 

I just notice that my patch is in 'pu'.
But it is version 7 instead of the improved version 8.

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

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

[Junio C Hamano]

Jeremiah Mahler <jmmahler@gmail.com> writes:

> I just notice that my patch is in 'pu'.
> But it is version 7 instead of the improved version 8.

Yeah, I know.  In a distributed environment, multiple people work
independently and a sequence of event can go like this:

 - I read v7, comment, and queue it only so that I won't forget;
 - I attend to other topics;
 - I start the day's integration cycle, merging topics to the
   integration branches, maint, master, next and pu;
 - You reroll v8 and post it;
 - I may not notice v8, or I may notice v8 but think it is not
   worth to discard the integration work so far only to replace
   v7 with v8.
 - The integration result is pushed out.

A reminder is very much appreciated, but on the other hand it is not
something unusual.  It happens all the time, and I usually am aware
of it ;-)

Thanks.

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

[Jeremiah Mahler]

Junio,

On Thu, May 22, 2014 at 12:00:39PM -0700, Junio C Hamano wrote:
> Jeremiah Mahler <jmmahler@gmail.com> writes:
> 
> > I just notice that my patch is in 'pu'.
> > But it is version 7 instead of the improved version 8.
> 
> Yeah, I know.  In a distributed environment, multiple people work
> independently and a sequence of event can go like this:
> 
>  - I read v7, comment, and queue it only so that I won't forget;
>  - I attend to other topics;
>  - I start the day's integration cycle, merging topics to the
>    integration branches, maint, master, next and pu;
>  - You reroll v8 and post it;
>  - I may not notice v8, or I may notice v8 but think it is not
>    worth to discard the integration work so far only to replace
>    v7 with v8.
>  - The integration result is pushed out.
> 
> A reminder is very much appreciated, but on the other hand it is not
> something unusual.  It happens all the time, and I usually am aware
> of it ;-)
> 
> Thanks.

Thanks for explaining how this process works.

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

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

[Junio C Hamano]

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

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

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

[Jeremiah Mahler]

Junio,

On Thu, May 22, 2014 at 01:52:45PM -0700, Junio C Hamano wrote:
> Jeremiah Mahler <jmmahler@gmail.com> writes:
> 
...
>
> 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
> 
...
>
> 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.
> 
--no-signature should inhibit all signatures.

>  - 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.
> 
Arguments on the command line should take precendence over
anything in the config.

Your comments make it clear that I have not accounted for all the possible
cases.  Below is a table of all the reasonable cases.  It should account
for cases you mentioned as well as others.

  Key:
  ---
  default: Git version number
  sig1: .signature from column 1
  file1: .signaturefile from column 1
  sig2: --signature from column 2
  file2: --signature-file in column 2

A preceeding 'format.' is assumed for config.  .signature -> format.signature
Command line arguments take precedence over config options.

|----------------------------+-------------------------------------+---------|
| config  (1)                | argv  (2)                           |         |
|----------------------------+-------------------------------------+---------|
|                            |                                     | default |
|                            | --signature                         | sig2    |
|                            | --signature-file                    | file2   |
|                            | --no-signature                      | none    |
|                            | --no-signature-file                 | none    |
|                            | --signature, --signature-file       | die     |
|                            | --signature, --no-signature-file    | sig2    |
|                            | --signature-file, --no-signature    | none    |
|                            | --no-signature, --no-signature-file | none    |
| .signature                 |                                     | sig1    |
| .signature                 | --signature                         | sig2    |
| .signature                 | --signature-file                    | file2   |
| .signature                 | --no-signature                      | none    |
| .signature                 | --no-signature-file                 | sig1    |
| .signature                 | --signature, --signature-file       | die     |
| .signature                 | --signature, --no-signature-file    | sig2    |
| .signature                 | --signature-file, --no-signature    | none    |
| .signature                 | --no-signature, --no-signature-file | none    |
| .signaturefile             |                                     | file1   |
| .signaturefile             | --signature                         | sig2    |
| .signaturefile             | --signature-file                    | file2   |
| .signaturefile             | --no-signature                      | none    |
| .signaturefile             | --no-signature-file                 | default |
| .signaturefile             | --signature, --signature-file       | die     |
| .signaturefile             | --signature, --no-signature-file    | sig2    |
| .signaturefile             | --signature-file, --no-signature    | none    |
| .signaturefile             | --no-signature, --no-signature-file | none    |
| .signature, .signaturefile |                                     | die     |
| .signature, .signaturefile | --signature                         | sig2    |
| .signature, .signaturefile | --signature-file                    | file2   |
| .signature, .signaturefile | --no-signature                      | none    |
| .signature, .signaturefile | --no-signature-file                 | sig1    |
| .signature, .signaturefile | --signature, --signature-file       | die     |
| .signature, .signaturefile | --signature, --no-signature-file    | sig2    |
| .signature, .signaturefile | --signature-file, --no-signature    | none    |
| .signature, .signaturefile | --no-signature, --no-signature-file | none    |
|----------------------------+-------------------------------------+---------|


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

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

[Junio C Hamano]

Jeremiah Mahler <jmmahler@gmail.com> writes:

> Your comments make it clear that I have not accounted for all the possible
> cases.  Below is a table of all the reasonable cases.  It should account
> for cases you mentioned as well as others.
>
>   Key:
>   ---
>   default: Git version number
>   sig1: .signature from column 1
>   file1: .signaturefile from column 1
>   sig2: --signature from column 2
>   file2: --signature-file in column 2
>
> A preceeding 'format.' is assumed for config.  .signature -> format.signature
> Command line arguments take precedence over config options.

;-)

I do not mind the full matrix if it is cleanly done.

It may be an overkill and a maintenance burden, especially if it can
be done only with a sequence of cut-and-paste repetition (i.e. not
as a loop in the test script that synthesises the permutations).

The suggestion for those three combinations were meant to be an
easy-to-implement-and-maintain compromise.

Thanks.

> |----------------------------+-------------------------------------+---------|
> | config  (1)                | argv  (2)                           |         |
> |----------------------------+-------------------------------------+---------|
> |                            |                                     | default |
> |                            | --signature                         | sig2    |
> |                            | --signature-file                    | file2   |
> |                            | --no-signature                      | none    |
> |                            | --no-signature-file                 | none    |
> |                            | --signature, --signature-file       | die     |
> |                            | --signature, --no-signature-file    | sig2    |
> |                            | --signature-file, --no-signature    | none    |
> |                            | --no-signature, --no-signature-file | none    |
> | .signature                 |                                     | sig1    |
> | .signature                 | --signature                         | sig2    |
> | .signature                 | --signature-file                    | file2   |
> | .signature                 | --no-signature                      | none    |
> | .signature                 | --no-signature-file                 | sig1    |
> | .signature                 | --signature, --signature-file       | die     |
> | .signature                 | --signature, --no-signature-file    | sig2    |
> | .signature                 | --signature-file, --no-signature    | none    |
> | .signature                 | --no-signature, --no-signature-file | none    |
> | .signaturefile             |                                     | file1   |
> | .signaturefile             | --signature                         | sig2    |
> | .signaturefile             | --signature-file                    | file2   |
> | .signaturefile             | --no-signature                      | none    |
> | .signaturefile             | --no-signature-file                 | default |
> | .signaturefile             | --signature, --signature-file       | die     |
> | .signaturefile             | --signature, --no-signature-file    | sig2    |
> | .signaturefile             | --signature-file, --no-signature    | none    |
> | .signaturefile             | --no-signature, --no-signature-file | none    |
> | .signature, .signaturefile |                                     | die     |
> | .signature, .signaturefile | --signature                         | sig2    |
> | .signature, .signaturefile | --signature-file                    | file2   |
> | .signature, .signaturefile | --no-signature                      | none    |
> | .signature, .signaturefile | --no-signature-file                 | sig1    |
> | .signature, .signaturefile | --signature, --signature-file       | die     |
> | .signature, .signaturefile | --signature, --no-signature-file    | sig2    |
> | .signature, .signaturefile | --signature-file, --no-signature    | none    |
> | .signature, .signaturefile | --no-signature, --no-signature-file | none    |
> |----------------------------+-------------------------------------+---------|
>
>
> Thanks,