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

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

[Jeremiah Mahler]

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>

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

[Jeff King]

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 <jmmahler@gmail.com>

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 <peff@peff.net>
---
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>

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

I actually think these "supress extra LFs" trying to be overly smart
and inviting unnecessary surprises.  Unlike log messages people type
(in which we do squash runs of blank lines and other prettifying),
mail-signature string is not something people keep typing, and it
would be better to keep it simple and consistent, i.e. we can
declare that the users who use non-default mail-signature can and
should learn to:

    --signature='this is the first line of my long sig

    with a blank line and then it ends here'

and be done with it, I think.

The trailing blank after the mail-signature is a different issue.  I
think it is safe to remove it and I also think the result may look
better, but at the same time, it is very close to the "if we were
writing format-patch today, then we would..." category, I would say.

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

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

[Jeff King]

On Tue, May 20, 2014 at 10:53:11AM -0700, Junio C Hamano wrote:

> I actually think these "supress extra LFs" trying to be overly smart
> and inviting unnecessary surprises.  Unlike log messages people type
> (in which we do squash runs of blank lines and other prettifying),
> mail-signature string is not something people keep typing, and it
> would be better to keep it simple and consistent, i.e. we can
> declare that the users who use non-default mail-signature can and
> should learn to:
> 
>     --signature='this is the first line of my long sig
> 
>     with a blank line and then it ends here'
> 
> and be done with it, I think.

If it were just "--signature", I'd agree. After all, nobody is even
complaining. But this is also in preparation for --signature-file.
Should the user create a file without a trailing newline?

We can special-case --signature-file to strip the final newline from the
read file, but it seems friendlier to handle it at the printing stage
(and then we handle the unlikely but possible --signature as above for
free). I don't think it would adversely impact any real-world case,
because somebody would have to:

  1. already be including an extra trailing newline

  2. really _want_ three newlines at the end

> The trailing blank after the mail-signature is a different issue.  I
> think it is safe to remove it and I also think the result may look
> better, but at the same time, it is very close to the "if we were
> writing format-patch today, then we would..." category, I would say.

Yeah. I think it is probably extraneous and would not hurt to remove.
But it may not be worth worrying about (it's really the _two_ lines
caused by the unconditional newline above that bugs me).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If it were just "--signature", I'd agree. After all, nobody is even
> complaining. But this is also in preparation for --signature-file.
> Should the user create a file without a trailing newline?

Ahh, I missed that part.

I am fine with processing it with stripspace().

By the way, at some point we may want to move that helper function
to strbuf.c, but that is a separate issue.

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

[Jeremiah Mahler]

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:
> 
...
> > +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 <peff@peff.net>
> ---
> 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

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

[Jeff King]

On Tue, May 20, 2014 at 11:46:51AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > If it were just "--signature", I'd agree. After all, nobody is even
> > complaining. But this is also in preparation for --signature-file.
> > Should the user create a file without a trailing newline?
> 
> Ahh, I missed that part.
> 
> I am fine with processing it with stripspace().

I wasn't planning on anything as drastic as stripspace. I really just
wanted to suppress the one newline, which is almost certainly the right
thing to include for "--signature", but the wrong thing for
"--signature-file" (i.e., the patch I posted earlier).

Stripspace() would drop all extra whitespace, and I wondered if people
would _want_ it in their sigs (e.g., a blank line after the "-- " but
before their .sig content).

I dunno. Maybe it is not worth caring too much about. I don't want to
hold up Jeremiah's patch for something that I suspect neither of us
cares _that_ much about (I know I am not planning on using
--signature-file myself). I just don't want to deal with a patch later
that says "oh, this spacing is wrong" and have to respond "yes, but we
have to retain it so as not to break existing users".

> By the way, at some point we may want to move that helper function
> to strbuf.c, but that is a separate issue.

Agreed. I was touching some string functions earlier today and noticed
that strbuf.c actually contains a lot of non-strbuf functions for
dealing with C strings. That's fine, I guess, but I also wondered if we
should have a separate file for C-string functions. I suppose it doesn't
matter that much either way, as long as it's in a libgit.a file (and
stripspace currently is _not_, which I assume is what you were
indicating above).

-Peff

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

[Jeremiah Mahler]

On Wed, May 21, 2014 at 12:42:55PM -0400, Jeff King wrote:
> On Tue, May 20, 2014 at 11:46:51AM -0700, Junio C Hamano wrote:
> 
> > Jeff King <peff@peff.net> writes:
> > 
> > > If it were just "--signature", I'd agree. After all, nobody is even
> > > complaining. But this is also in preparation for --signature-file.
> > > Should the user create a file without a trailing newline?
> > 
> > Ahh, I missed that part.
> > 
> > I am fine with processing it with stripspace().
> 
> I wasn't planning on anything as drastic as stripspace. I really just
> wanted to suppress the one newline, which is almost certainly the right
> thing to include for "--signature", but the wrong thing for
> "--signature-file" (i.e., the patch I posted earlier).
> 
> Stripspace() would drop all extra whitespace, and I wondered if people
> would _want_ it in their sigs (e.g., a blank line after the "-- " but
> before their .sig content).
> 
> I dunno. Maybe it is not worth caring too much about. I don't want to
> hold up Jeremiah's patch for something that I suspect neither of us
> cares _that_ much about (I know I am not planning on using
> --signature-file myself). I just don't want to deal with a patch later
> that says "oh, this spacing is wrong" and have to respond "yes, but we
> have to retain it so as not to break existing users".
> 
> > By the way, at some point we may want to move that helper function
> > to strbuf.c, but that is a separate issue.
> 
> Agreed. I was touching some string functions earlier today and noticed
> that strbuf.c actually contains a lot of non-strbuf functions for
> dealing with C strings. That's fine, I guess, but I also wondered if we
> should have a separate file for C-string functions. I suppose it doesn't
> matter that much either way, as long as it's in a libgit.a file (and
> stripspace currently is _not_, which I assume is what you were
> indicating above).
> 
> -Peff

I am fine with including your previous patch.

Would like me to test it out and create another patch set?

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

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

[Jeff King]

On Wed, May 21, 2014 at 09:55:43AM -0700, Jeremiah Mahler wrote:

> I am fine with including your previous patch.

I think that would be my preference, but we'll see what Junio says.

> Would like me to test it out and create another patch set?

Yeah, that would be the logical next step. I think the only thing you
should need to adjust in your patch is to strip only one line from the
output, rather than two.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, May 20, 2014 at 11:46:51AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > If it were just "--signature", I'd agree. After all, nobody is even
>> > complaining. But this is also in preparation for --signature-file.
>> > Should the user create a file without a trailing newline?
>> 
>> Ahh, I missed that part.
>> 
>> I am fine with processing it with stripspace().
>
> I wasn't planning on anything as drastic as stripspace. I really just
> wanted to suppress the one newline, which is almost certainly the right
> thing to include for "--signature", but the wrong thing for
> "--signature-file" (i.e., the patch I posted earlier).
> ...
> I dunno. Maybe it is not worth caring too much about.

I suggested stripspace() because I know we do not care too
much, actually.

Cleansing blank lines in one way for many types of user input
(e.g. commit log messages and tag messages) while doing it in a
completely different way just for "--signature-file" is warranted if
there is a good reason for them to be different, but I did not think
of any, and I still don't.  So...

>> By the way, at some point we may want to move that helper function
>> to strbuf.c, but that is a separate issue.
>
> Agreed. I was touching some string functions earlier today and noticed
> that strbuf.c actually contains a lot of non-strbuf functions for
> dealing with C strings. That's fine, I guess, but I also wondered if we
> should have a separate file for C-string functions. I suppose it doesn't
> matter that much either way, as long as it's in a libgit.a file (and
> stripspace currently is _not_, which I assume is what you were
> indicating above).

Yes, I thought you would have used it in your follow-up patch if it
were more prominent.

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

[Jeff King]

On Wed, May 21, 2014 at 10:37:05AM -0700, Junio C Hamano wrote:

> > I wasn't planning on anything as drastic as stripspace. I really just
> > wanted to suppress the one newline, which is almost certainly the right
> > thing to include for "--signature", but the wrong thing for
> > "--signature-file" (i.e., the patch I posted earlier).
> > ...
> > I dunno. Maybe it is not worth caring too much about.
> 
> I suggested stripspace() because I know we do not care too
> much, actually.
> 
> Cleansing blank lines in one way for many types of user input
> (e.g. commit log messages and tag messages) while doing it in a
> completely different way just for "--signature-file" is warranted if
> there is a good reason for them to be different, but I did not think
> of any, and I still don't.  So...

I didn't think of mine as cleansing. It is more like "do not duplicate a
newline ourselves if there is already one there".  But I guess those are
two sides of the same coin.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, May 21, 2014 at 10:37:05AM -0700, Junio C Hamano wrote:
>
>> > I wasn't planning on anything as drastic as stripspace. I really just
>> > wanted to suppress the one newline, which is almost certainly the right
>> > thing to include for "--signature", but the wrong thing for
>> > "--signature-file" (i.e., the patch I posted earlier).
>> > ...
>> > I dunno. Maybe it is not worth caring too much about.
>> 
>> I suggested stripspace() because I know we do not care too
>> much, actually.
>> 
>> Cleansing blank lines in one way for many types of user input
>> (e.g. commit log messages and tag messages) while doing it in a
>> completely different way just for "--signature-file" is warranted if
>> there is a good reason for them to be different, but I did not think
>> of any, and I still don't.  So...
>
> I didn't think of mine as cleansing. It is more like "do not duplicate a
> newline ourselves if there is already one there".  But I guess those are
> two sides of the same coin.

Yeah, I agree with the last sentence.  My mention of "cleansing"
took into account your "do we want to omit the leading blank as
well?" as well.  In any case, wouldn't reusing stripspace() make the
fix-up shorter?

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

[Jeff King]

On Wed, May 21, 2014 at 11:26:18AM -0700, Junio C Hamano wrote:

> Yeah, I agree with the last sentence.  My mention of "cleansing"
> took into account your "do we want to omit the leading blank as
> well?" as well.  In any case, wouldn't reusing stripspace() make the
> fix-up shorter?

Not really. Doing stripspace() on the file we read would be a one-liner,
but it doesn't address the extra line. We _already_ have
stripspace-compatible output in the file, with a trailing newline. It's
the one we add for the cmdline arg or default (which lack a newline
generally) that causes the doubling.

So I think it would be something like:

diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..0312402 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) {
+		struct strbuf buf = STRBUF_INIT;
+		strbuf_addstr(&buf, signature);
+		stripspace(&buf, 0);
+		printf("-- \n%s\n", buf.buf);
+		strbuf_release(&buf);
+	}
 }
 
 static void add_branch_description(struct strbuf *buf, const char *branch_name)

I.e., make sure we have consistent stripspace output, and then stop
adding the extra newline in the printf (notice we still include the
"extra" blank at the end, though we can obviously easily drop it here).
Compare to the previous patch I sent, or to the shortest possible:

diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..16b8771 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -845,7 +845,8 @@ 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);
+		printf("-- \n%s%s\n", signature,
+		       ends_with(signature, "\n") ? "" : "\n");
 }
 
 static void add_branch_description(struct strbuf *buf, const char *branch_name)

But I'm not sure "length of code" is the right metric. They're all
pretty easy to do, if we can decide on which.

I think the ratio of usefulness to number of words in this sub-thread is
getting off. I'd be OK with any of:

  1. Leave it as-is. Files get two blank lines.

  2. Conditional newline

  3. Stripspace.

-Peff

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

[Jeremiah Mahler]

On Wed, May 21, 2014 at 04:47:39PM -0400, Jeff King wrote:
> On Wed, May 21, 2014 at 11:26:18AM -0700, Junio C Hamano wrote:
> 
...
> I think the ratio of usefulness to number of words in this sub-thread is
> getting off. I'd be OK with any of:
...
> 
> -Peff

I think this has become a discussion about the airspeed of an unladen swallow.
:-)

I think we're close to a final patch. v7 sent.

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