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

On Tue, May 13, 2014 at 09:07:12AM -0700, Jonathan Nieder wrote:

Hi,

Jeremiah Mahler wrote:

quoted

  # from a string
  $ git format-patch --signature "from a string" origin

  # or from a file
  $ git format-patch --signature ~/.signature origin

Interesting.  But... what if I want my patch to end with

	-- 
	/home/jrnieder/.signature

?  It seems safer to introduce a separate --signature-file option.

It is probably smarter to avoid that corner case entirely.
Good idea.

[...]

quoted

 builtin/log.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

Tests?

I added a test which checks that a valid patch is produced and that
the signature from the file appears in the output.

Thanks and hope that helps,
Jonathan

In addition to addressing the suggestions from Jonathan I also
updated the Documentation.

This solution uses a static buffer to store the signature which does
create a size limitation (1024 bytes).  I considered a solution
using malloc but I could not figure out a clean way of determining when
to free the memory.

Thanks for the help and suggestions.

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

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

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

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

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 5c0a4ab..dd57841 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,12 @@ 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>::
+	Add a signature, by including the contents of a file, to each message
+	produced. Per RFC 3676 the signature is separated from the body by a
+	line with '-- ' on it. If the signature option is omitted the signature
+	defaults to the Git version number.
+
 --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..4a7308d 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -1147,6 +1147,27 @@ static int from_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+static int signature_file_callback(const struct option *opt, const char *arg,
+							int unset)
+{
+	const char **signature = opt->value;
+	static char buf[1024];
+	size_t sz;
+	FILE *fd;
+
+	fd = fopen(arg, "r");
+	if (fd) {
+		sz = sizeof(buf);
+		sz = fread(buf, 1, sz - 1, fd);
+		if (sz) {
+			buf[sz] = '\0';
+			*signature = buf;
+		}
+		fclose(fd);
+	}
+	return 0;
+}
+
 int cmd_format_patch(int argc, const char **argv, const char *prefix)
 {
 	struct commit *commit;

@@ -1230,6 +1251,9 @@ 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")),
+		{ OPTION_CALLBACK, 0, "signature-file", &signature, N_("signature-file"),
+				N_("add a signature from contents of a file"),
+			    PARSE_OPT_NONEG, signature_file_callback },
 		OPT__QUIET(&quiet, N_("don't print the patch filenames")),
 		OPT_END()
 	};

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 9c80633..19b67e3 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh

@@ -762,6 +762,19 @@ 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 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

On Thu, May 15, 2014 at 06:31:21PM -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.

I think this version looks nicer than the original.

A few questions/comments:

+static int signature_file_callback(const struct option *opt, const char *arg,
+							int unset)
+{
+	const char **signature = opt->value;
+	static char buf[1024];
+	size_t sz;
+	FILE *fd;
+
+	fd = fopen(arg, "r");
+	if (fd) {
+		sz = sizeof(buf);
+		sz = fread(buf, 1, sz - 1, fd);
+		if (sz) {
+			buf[sz] = '\0';
+			*signature = buf;
+		}
+		fclose(fd);
+	}
+	return 0;
+}

We have routines for reading directly into a strbuf, which eliminates
the need for this 1024-byte limit. We even have a wrapper that can make
this much shorter:

  struct strbuf buf = STRBUF_INIT;

  strbuf_read_file(&buf, arg, 128);
  *signature = strbuf_detach(&buf, NULL);

I notice that you ignore any errors. Is that intentional (so that we
silently ignore missing --signature files)? If so, should we actually
treat it as an empty file (e.g., in my code above, we always set
*signature, even if the file was missing)?

Finally, I suspect that:

  cd path/in/repo &&
  git format-patch --signature-file=foo

will not work, as we chdir() to the toplevel before evaluating the
arguments. You can fix that either by using parse-option's OPT_FILENAME
to save the filename, followed by opening the file after all arguments
are processed; or by manually fixing up the filename.

Since parse-options already knows how to do this fixup (it does it for
OPT_FILENAME), it would be nice if it were a flag rather than a full
type, so you could specify at as an option to your callback here:

+		{ OPTION_CALLBACK, 0, "signature-file", &signature, N_("signature-file"),
+				N_("add a signature from contents of a file"),
+			    PARSE_OPT_NONEG, signature_file_callback },

Noticing your OPT_NONEG, though, I wonder if you should simply use an
OPT_FILENAME. I would expect --no-signature-file to countermand any
earlier --signature-file on the command-line (or if we eventually grow a
config option, which seems sensible, it would tell git to ignore the
option). The usual ordering for that is:

  1. Read config and store format.signatureFile as a string
     "signature_file".

  2. Parse arguments. --signature-file=... sets signature_file, and
     --no-signature-file sets it to NULL.

  3. If signature_file is non-NULL, load it.

And I believe OPT_FILENAME will implement (2) for you.

One downside of doing it this way is that you need to specify what will
happen when both "--signature" (or format.signature) and
"--signature-file" are set. With your current code, I think
"--signature=foo --signature-file=bar" will take the second one. I think
it would be fine to prefer one to the other, or to just notice that both
are set and complain.

-Peff

On Fri, May 16, 2014 at 04:14:45AM -0400, Jeff King wrote:

On Thu, May 15, 2014 at 06:31:21PM -0700, Jeremiah Mahler wrote:

...

A few questions/comments:

quoted

+static int signature_file_callback(const struct option *opt, const char *arg,
+							int unset)
+{
+	const char **signature = opt->value;
+	static char buf[1024];
+	size_t sz;
+	FILE *fd;
+
+	fd = fopen(arg, "r");
+	if (fd) {
+		sz = sizeof(buf);
+		sz = fread(buf, 1, sz - 1, fd);
+		if (sz) {
+			buf[sz] = '\0';
+			*signature = buf;
+		}
+		fclose(fd);
+	}
+	return 0;
+}

We have routines for reading directly into a strbuf, which eliminates
the need for this 1024-byte limit. We even have a wrapper that can make
this much shorter:

  struct strbuf buf = STRBUF_INIT;

  strbuf_read_file(&buf, arg, 128);
  *signature = strbuf_detach(&buf, NULL);

Yes, that is much cleaner.
The memory returned by strbuf_detach() will have to be freed as well.

I notice that you ignore any errors. Is that intentional (so that we
silently ignore missing --signature files)? If so, should we actually
treat it as an empty file (e.g., in my code above, we always set
*signature, even if the file was missing)?

Finally, I suspect that:

  cd path/in/repo &&
  git format-patch --signature-file=foo

will not work, as we chdir() to the toplevel before evaluating the
arguments. You can fix that either by using parse-option's OPT_FILENAME
to save the filename, followed by opening the file after all arguments
are processed; or by manually fixing up the filename.

Yes, it wouldn't have worked.
Using OPT_FILENAME is a much better solution.

Since parse-options already knows how to do this fixup (it does it for
OPT_FILENAME), it would be nice if it were a flag rather than a full
type, so you could specify at as an option to your callback here:

quoted

+		{ OPTION_CALLBACK, 0, "signature-file", &signature, N_("signature-file"),
+				N_("add a signature from contents of a file"),
+			    PARSE_OPT_NONEG, signature_file_callback },

Noticing your OPT_NONEG, though, I wonder if you should simply use an
OPT_FILENAME. I would expect --no-signature-file to countermand any
earlier --signature-file on the command-line (or if we eventually grow a
config option, which seems sensible, it would tell git to ignore the
option). The usual ordering for that is:

Another case is when both --signature="foo" and --no-signature-file are used.
Currently this would only negate the file option which would allow
the --signature option to be used.

  1. Read config and store format.signatureFile as a string
     "signature_file".

  2. Parse arguments. --signature-file=... sets signature_file, and
     --no-signature-file sets it to NULL.

  3. If signature_file is non-NULL, load it.

And I believe OPT_FILENAME will implement (2) for you.

One downside of doing it this way is that you need to specify what will
happen when both "--signature" (or format.signature) and
"--signature-file" are set. With your current code, I think
"--signature=foo --signature-file=bar" will take the second one. I think
it would be fine to prefer one to the other, or to just notice that both
are set and complain.

-Peff

Having --signature-file override --signature seems simpler to implement.
The signature variable has a default value which complicates
determining whether it was set or not.

Thanks for the great suggestions.

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

On Sat, May 17, 2014 at 12:25:48AM -0700, Jeremiah Mahler wrote:

quoted

We have routines for reading directly into a strbuf, which eliminates
the need for this 1024-byte limit. We even have a wrapper that can make
this much shorter:

  struct strbuf buf = STRBUF_INIT;

  strbuf_read_file(&buf, arg, 128);
  *signature = strbuf_detach(&buf, NULL);

Yes, that is much cleaner.
The memory returned by strbuf_detach() will have to be freed as well.

In cases like this, we often let the memory leak. It's in a global that
stays valid through the whole program, so we just let the program's exit
clean it up.

Having --signature-file override --signature seems simpler to implement.
The signature variable has a default value which complicates
determining whether it was set or not.

Yeah, the default value complicates it. I think you can handle that just
by moving the default to the main logic, like:

  static const char *signature;
  static const char *signature_file;

  ...

  if (signature) {
	if (signature_file)
		die("you cannot specify both a signature and a signature-file");
	/* otherwise, we already have the value */
  } else if (signature_file) {
	struct strbuf buf = STRBUF_INIT;
	strbuf_read(&buf, signature_file, 128);
	signature = strbuf_detach(&buf);
  } else
	signature = git_version_string;

and as a bonus, that keeps all of the logic together in one (fairly
readable) chain.

-Peff

On Sat, May 17, 2014 at 03:42:24AM -0400, Jeff King wrote:

On Sat, May 17, 2014 at 12:25:48AM -0700, Jeremiah Mahler wrote:

quoted

quoted

We have routines for reading directly into a strbuf, which eliminates
the need for this 1024-byte limit. We even have a wrapper that can make
this much shorter:

  struct strbuf buf = STRBUF_INIT;

  strbuf_read_file(&buf, arg, 128);
  *signature = strbuf_detach(&buf, NULL);

Yes, that is much cleaner.
The memory returned by strbuf_detach() will have to be freed as well.

In cases like this, we often let the memory leak. It's in a global that
stays valid through the whole program, so we just let the program's exit
clean it up.

It bugs me but I see your point.
It works just fine in this situation.

quoted

Having --signature-file override --signature seems simpler to implement.
The signature variable has a default value which complicates
determining whether it was set or not.

Yeah, the default value complicates it. I think you can handle that just
by moving the default to the main logic, like:

  static const char *signature;
  static const char *signature_file;

  ...

  if (signature) {
	if (signature_file)
		die("you cannot specify both a signature and a signature-file");
	/* otherwise, we already have the value */
  } else if (signature_file) {
	struct strbuf buf = STRBUF_INIT;
	strbuf_read(&buf, signature_file, 128);
	signature = strbuf_detach(&buf);
  } else
	signature = git_version_string;

Before, --no-signature would clear the &signature.
With this code it sees it as not being set and assigns
the default version string.

and as a bonus, that keeps all of the logic together in one (fairly
readable) chain.

-Peff

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

On Sat, May 17, 2014 at 01:59:11AM -0700, Jeremiah Mahler wrote:

quoted

  if (signature) {
	if (signature_file)
		die("you cannot specify both a signature and a signature-file");
	/* otherwise, we already have the value */
  } else if (signature_file) {
	struct strbuf buf = STRBUF_INIT;
	strbuf_read(&buf, signature_file, 128);
	signature = strbuf_detach(&buf);
  } else
	signature = git_version_string;

Before, --no-signature would clear the &signature.
With this code it sees it as not being set and assigns
the default version string.

Ah, you're right. Thanks for catching it.

If you wanted to know whether it was set, I guess you'd have to compare
it to the default, like:

  if (signature_file) {
	if (signature && signature != git_version_string)
		die("you cannot specify both a signature and a signature-file");
	... read signature file ...
  }

though it's a bit ugly that this code has to know what the default is.
Having signature-file take precedence is OK with me, but it feels
somewhat arbitrary to me from the user's perspective.

-Peff

On Sat, May 17, 2014 at 06:00:14AM -0400, Jeff King wrote:

On Sat, May 17, 2014 at 01:59:11AM -0700, Jeremiah Mahler wrote:

quoted

quoted

  if (signature) {
	if (signature_file)
		die("you cannot specify both a signature and a signature-file");
	/* otherwise, we already have the value */
  } else if (signature_file) {
	struct strbuf buf = STRBUF_INIT;
	strbuf_read(&buf, signature_file, 128);
	signature = strbuf_detach(&buf);
  } else
	signature = git_version_string;

Before, --no-signature would clear the &signature.
With this code it sees it as not being set and assigns
the default version string.

Ah, you're right. Thanks for catching it.

If you wanted to know whether it was set, I guess you'd have to compare
it to the default, like:

  if (signature_file) {
	if (signature && signature != git_version_string)
		die("you cannot specify both a signature and a signature-file");
	... read signature file ...
  }

That works until someone changes the default value.
But if they did that then some tests should fail.

I like the address comparision which avoids a string comparision.

though it's a bit ugly that this code has to know what the default is.
Having signature-file take precedence is OK with me, but it feels
somewhat arbitrary to me from the user's perspective.

-Peff

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