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

STALE3738d

4 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Jeremiah Mahler <hidden>
Date: 2016-06-15 23:01:15

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

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

  - Use git_config_pathname instead of git_config_string for ~ expansion.

  - Eliminated head/tail --lines which is not POSIX compliant.
    Replaced with sed equivalents.

  - Used test_config instead of git config which also handles unsetting.

  - Use a DEFAULT_SIGNATURE variable to better describe its purpose
    instead of git_version_string which could be confusing.


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

 Documentation/config.txt           |  4 ++++
 Documentation/git-format-patch.txt |  4 ++++
 builtin/log.c                      | 19 ++++++++++++++++++-
 t/t4014-format-patch.sh            | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 58 insertions(+), 1 deletion(-)

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

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

From: Jeremiah Mahler <hidden>
Date: 2016-06-15 23:01:15

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 <redacted>
---
 Documentation/config.txt           |  4 ++++
 Documentation/git-format-patch.txt |  4 ++++
 builtin/log.c                      | 19 ++++++++++++++++++-
 t/t4014-format-patch.sh            | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 58 insertions(+), 1 deletion(-)
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 39e8836..7ebe302 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -672,7 +672,9 @@ static void add_header(const char *value)
 #define THREAD_DEEP 2
 static int thread;
 static int do_signoff;
-static const char *signature = git_version_string;
+#define DEFAULT_SIGNATURE git_version_string
+static const char *signature = DEFAULT_SIGNATURE;
+static const char *signature_file;
 static int config_cover_letter;
 
 enum {
@@ -742,6 +744,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;
@@ -1230,6 +1234,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 +1453,17 @@ 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 != DEFAULT_SIGNATURE)
+			die(_("cannot specify both signature and signature-file"));
+
+		struct strbuf buf = STRBUF_INIT;
+
+		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..74f0aec 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -762,6 +762,38 @@ 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 &&
+	sed -n -e "/^-- $/,\$p" <output | sed -e "1 d" | sed -e "\$d" | sed -e "\$d" >output2 &&
+	test_cmp expect output2
+'
+
+test_expect_success 'format-patch --signature and --signature-file die' '
+	test_must_fail 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
+'
+
+test_expect_success 'format-patch with format.signaturefile config' '
+	test_config format.signaturefile expect &&
+	git format-patch --stdout -1 >output &&
+	check_patch output &&
+	sed -n -e "/^-- $/,\$p" <output | sed -e "1 d" | sed -e "\$d" | sed -e "\$d" >output2 &&
+	test_cmp expect output2
+'
+
 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 v5] format-patch --signature-file <file>

From: Jeff King <hidden>
Date: 2016-06-15 23:01:15

On Tue, May 20, 2014 at 01:00:06AM -0700, Jeremiah Mahler wrote:
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 <redacted>
Thanks for keeping at it. This looks good, but I have one question:
+test_expect_success 'format-patch --signature-file=file' '
+	git format-patch --stdout --signature-file=expect -1 >output &&
+	check_patch output &&
+	sed -n -e "/^-- $/,\$p" <output | sed -e "1 d" | sed -e "\$d" | sed -e "\$d" >output2 &&
+	test_cmp expect output2
+'
Here we drop two final lines from the email output. But if I just use
vanilla git and try this, I get nothing:

  $ git format-patch -1 --stdout |
    sed -n -e "/^-- $/,\$p" | sed -e "1 d" | sed -e "\$d" | sed -e "\$d"

Removing the second dropped line works:

  $ git format-patch -1 --stdout |
    sed -n -e "/^-- $/,\$p" | sed -e "1 d" | sed -e "\$d"
  2.0.0.rc1.436.g03cb729

I guess the signature code is adding its own newline, so that the
default signature (or "--signature=foo") works. But it adds it
unconditionally, which is probably not what we want for signatures read
from a file.

Do we maybe want something like this right before your patch?

-- >8 --
Subject: format-patch: make newline after signature conditional

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 <redacted>
---
In the example above, there's also a newline before the signature
starts. Should we suppress the first one, too?

Also, I'm not clear on the purpose of the extra trailing line in the
first place. Emails now end with (">" added to show blanks):

  > -- 
  > 2.0.0-rc3...
  >

Is there are a reason for that final blank line?

 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)
-- 
2.0.0.rc1.436.g03cb729

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

From: Jeremiah Mahler <hidden>
Date: 2016-06-15 23:01:16

On Tue, May 20, 2014 at 04:27:40AM -0400, Jeff King wrote:
On Tue, May 20, 2014 at 01:00:06AM -0700, Jeremiah Mahler wrote:
...
quoted hunk
quoted
+test_expect_success 'format-patch --signature-file=file' '
+	git format-patch --stdout --signature-file=expect -1 >output &&
+	check_patch output &&
+	sed -n -e "/^-- $/,\$p" <output | sed -e "1 d" | sed -e "\$d" | sed -e "\$d" >output2 &&
+	test_cmp expect output2
+'
Here we drop two final lines from the email output. But if I just use
vanilla git and try this, I get nothing:

  $ git format-patch -1 --stdout |
    sed -n -e "/^-- $/,\$p" | sed -e "1 d" | sed -e "\$d" | sed -e "\$d"

Removing the second dropped line works:

  $ git format-patch -1 --stdout |
    sed -n -e "/^-- $/,\$p" | sed -e "1 d" | sed -e "\$d"
  2.0.0.rc1.436.g03cb729

I guess the signature code is adding its own newline, so that the
default signature (or "--signature=foo") works. But it adds it
unconditionally, which is probably not what we want for signatures read
from a file.

Do we maybe want something like this right before your patch?

-- >8 --
Subject: format-patch: make newline after signature conditional

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 <redacted>
---
In the example above, there's also a newline before the signature
starts. Should we suppress the first one, too?

Also, I'm not clear on the purpose of the extra trailing line in the
first place. Emails now end with (">" added to show blanks):

  > -- 
  > 2.0.0-rc3...
  >

Is there are a reason for that final blank line?

 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)
-- 
2.0.0.rc1.436.g03cb729
The simple solution, leaving things as they are, appeals to me.
For the purposes of testing just expect whatever odd number of blank
lines are produced.  Or perhaps slurp all the trailing blank lines and
then compare.

It would be nice if every signature was followed by exactly one blank line.
But if it had two it wouldn't bother me much.

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help