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

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

[Jeremiah Mahler]

On Tue, May 13, 2014 at 09:07:12AM -0700, Jonathan Nieder wrote:
> Hi,
> 
> Jeremiah Mahler wrote:
> 
> >   # 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.

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

[PATCH v2] 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 |  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

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

[Jeff King]

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

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

[Jeremiah Mahler]

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

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

[Jeff King]

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

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

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

[Jeremiah Mahler]

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

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

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

[Jeff King]

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

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

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

[Jeremiah Mahler]

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

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

[Junio C Hamano]

Jeremiah Mahler <jmmahler@gmail.com> writes:

> On Sat, May 17, 2014 at 06:00:14AM -0400, Jeff King wrote:
>> 
>> 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.

Well, "avoids" is not quite a correct phrasing, because !strcmp()
would be wrong there.  You cannot tell "the user did not set
anything and the variable stayed as the default" and "the user
explicitly gave us a string but it happened to be the same as the
default" apart with !strcmp().  Address comparison is not just
"avoids" but is the right thing to do in this case.

>> though it's a bit ugly that this code has to know what the default is.

Avoiding that is easy with an indirection, no?  Something like this
at the top:

  static const char *the_default_signature = git_version_string;
  static const char *signature = the_default_signature;

and comparing to see if signature points at the same address as
the_default_signature would give you what you want, I think.

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

[Jeremiah Mahler]

On Mon, May 19, 2014 at 09:54:33AM -0700, Junio C Hamano wrote:
> Jeremiah Mahler <jmmahler@gmail.com> writes:
> 
> > On Sat, May 17, 2014 at 06:00:14AM -0400, Jeff King wrote:
> >> 
>
> Avoiding that is easy with an indirection, no?  Something like this
> at the top:
> 
>   static const char *the_default_signature = git_version_string;
>   static const char *signature = the_default_signature;
> 
> and comparing to see if signature points at the same address as
> the_default_signature would give you what you want, I think.

I like your suggestion for a default signature variable.
Unfortunately, C doesn't like it as much.

static const char *the_default_signature = git_version_string;
static const char *signature = the_default_signature;  // ERROR
// initializer element is not constant

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

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

[Jeff King]

On Mon, May 19, 2014 at 10:46:21PM -0700, Jeremiah Mahler wrote:

> > Avoiding that is easy with an indirection, no?  Something like this
> > at the top:
> > 
> >   static const char *the_default_signature = git_version_string;
> >   static const char *signature = the_default_signature;
> > 
> > and comparing to see if signature points at the same address as
> > the_default_signature would give you what you want, I think.
> 
> I like your suggestion for a default signature variable.
> Unfortunately, C doesn't like it as much.
> 
> static const char *the_default_signature = git_version_string;
> static const char *signature = the_default_signature;  // ERROR
> // initializer element is not constant

You could do:

  #define DEFAULT_SIGNATURE git_version_string
  static const char *signature = DEFAULT_SIGNATURE;

  ...

  if (signature == DEFAULT_SIGNATURE)
     ...

but maybe that is getting a little unwieldy, considering the scope of
the original problem.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> You could do:
>
>   #define DEFAULT_SIGNATURE git_version_string
>   static const char *signature = DEFAULT_SIGNATURE;
>
>   ...
>
>   if (signature == DEFAULT_SIGNATURE)
>      ...
>
> but maybe that is getting a little unwieldy, considering the scope of
> the original problem.

I agree.  It is an invitation for doing something like

	#define DEFAULT_SIGNATURE "-- \nfoo bar\n"

which defeats the purpose.

I have an unrelated and larger design level suggestion, but it may
be better for me to refrain from sending it during the -rc freeze.

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

[Jeremiah Mahler]

On Tue, May 20, 2014 at 08:06:50AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > You could do:
> >
> >   #define DEFAULT_SIGNATURE git_version_string
> >   static const char *signature = DEFAULT_SIGNATURE;
> >
> >   ...
> >
> >   if (signature == DEFAULT_SIGNATURE)
> >      ...
> >
> > but maybe that is getting a little unwieldy, considering the scope of
> > the original problem.
> 
> I agree.  It is an invitation for doing something like
> 
> 	#define DEFAULT_SIGNATURE "-- \nfoo bar\n"
> 
> which defeats the purpose.
> 
> I have an unrelated and larger design level suggestion, but it may
> be better for me to refrain from sending it during the -rc freeze.

I will take DEFAULT_SIGNATURE out of the next patch revision.

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