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

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

[Jeremiah Mahler]

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

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

  - Adding #define DEFAULT_SIGNATURE was a good idea but it could be
    used in a way that nullifies the pointer comparison used to see if
    the default has changed.  So this was removed.

  - The test cases depend on there being two blank lines after a
    signature from a file.  This is an odd problem because if --signature
    or the default is used only one blank line is produced.
    This problem can be addressed in another patch.

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

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

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

[PATCH v6] 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            | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 56 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 39e8836..ab4718b 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;
@@ -1230,6 +1233,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 +1452,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 != git_version_string)
+			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..049493d 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 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 '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 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 v6] format-patch --signature-file <file>

[Junio C Hamano]

Jeremiah Mahler <jmmahler@gmail.com> writes:

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 9c80633..049493d 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

We have been trying not to do the above in recent test updates.  It
would be nice if this set-up did not have to be outside of the usual
test_expect_success structure.

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

Overlong line.  If we can't make this pipeline shorter, at least
fold it to a reasonable length, e.g.

	sed -n -e ... <output |
        sed -e '1d' -e "\$d" |
        sed -e "\$d" >output2 &&

or something.

The SP between the address "1" and insn "d" looks somewhat funny and
ugly, especially given that you write the other one "$d" without
such a SP.  Was there a specific reason why it was needed?

I would further think that renaming a few files and updating the way
the check is done may make the whole thing easier to understand.

 * rename the input for --signature-file to "mail-signature".

 * keep the name "output" to hold the format-patch output, i.e.

     git format-patch -1 --stdout --signature-file=mail-signature >output

 * Instead of munging the "mail signature" part of the output too
   excessively to match the input, formulate the expected output
   using "mail-signature" as an input, i.e.

     sed -e '1,/^-- $/d' <output >actual &&
     {
	cat mail-signature && echo && echo
     } >expect &&
     test_cmp expect actual

Alternatively, the third-bullet point above may want to be further
future-proofed by using stripspace, e.g.

    sed -e '1/^-- $/d' <output | git stripspace >actual &&
    git stripspace <mail-signature >expect &&
    test_cmp expect actual

Thanks.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeremiah Mahler <jmmahler@gmail.com> writes:
>
>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
>> index 9c80633..049493d 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
>
> We have been trying not to do the above in recent test updates.  It
> would be nice if this set-up did not have to be outside of the usual
> test_expect_success structure.

It seems you sent v7 before I had a chance to comment on this one.
The above was merely "it would be nicer..." and I am OK as-is.  The
comments on the rest are a bit more serious, though.

Thanks.

>
>> +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 &&
>
> Overlong line.  If we can't make this pipeline shorter, at least
> fold it to a reasonable length, e.g.
>
> 	sed -n -e ... <output |
>         sed -e '1d' -e "\$d" |
>         sed -e "\$d" >output2 &&
>
> or something.
>
> The SP between the address "1" and insn "d" looks somewhat funny and
> ugly, especially given that you write the other one "$d" without
> such a SP.  Was there a specific reason why it was needed?
>
> I would further think that renaming a few files and updating the way
> the check is done may make the whole thing easier to understand.
>
>  * rename the input for --signature-file to "mail-signature".
>
>  * keep the name "output" to hold the format-patch output, i.e.
>
>      git format-patch -1 --stdout --signature-file=mail-signature >output
>
>  * Instead of munging the "mail signature" part of the output too
>    excessively to match the input, formulate the expected output
>    using "mail-signature" as an input, i.e.
>
>      sed -e '1,/^-- $/d' <output >actual &&
>      {
> 	cat mail-signature && echo && echo
>      } >expect &&
>      test_cmp expect actual
>
> Alternatively, the third-bullet point above may want to be further
> future-proofed by using stripspace, e.g.
>
>     sed -e '1/^-- $/d' <output | git stripspace >actual &&
>     git stripspace <mail-signature >expect &&
>     test_cmp expect actual
>
> Thanks.

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

[Jeremiah Mahler]

On Wed, May 21, 2014 at 02:24:27PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
...
> 
> It seems you sent v7 before I had a chance to comment on this one.
> The above was merely "it would be nicer..." and I am OK as-is.  The
> comments on the rest are a bit more serious, though.
> 
> Thanks.
> 

It is not a problem.  Thanks again for all your feedback.

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

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

[Jeremiah Mahler]

On Wed, May 21, 2014 at 02:13:06PM -0700, Junio C Hamano wrote:
> Jeremiah Mahler <jmmahler@gmail.com> writes:
> 
...
> >  	! grep "^-- \$" output
...
> 
> We have been trying not to do the above in recent test updates.  It
> would be nice if this set-up did not have to be outside of the usual
> test_expect_success structure.
> 

Jeff caught those "! grep" instances in my patch.

I can submit a separate patch to fix instances like those in test cases
unrelated to this signature-file patch.

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

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

[Junio C Hamano]

Jeremiah Mahler <jmmahler@gmail.com> writes:

> On Wed, May 21, 2014 at 02:13:06PM -0700, Junio C Hamano wrote:
>> Jeremiah Mahler <jmmahler@gmail.com> writes:
>> 
> ...
>> >  	! grep "^-- \$" output
> ...
>> 
>> We have been trying not to do the above in recent test updates.  It
>> would be nice if this set-up did not have to be outside of the usual
>> test_expect_success structure.
>> 
>
> Jeff caught those "! grep" instances in my patch.

Hmm, I didn't mean that one, and I do not offhand what is wrong
about "! grep" that says "output should not contain this string".

The problem is a "cat" you added outside test_expect_*; the recent
push is to have as little executable outside them, especially the
"set-up" code to prepare for the real tests.

i.e. we have been trying to write new tests (and convert old ones)
like this:

        test_expect_success 'I test such and such ' '
                cat >input-for-test <<-\EOF &&
                here comes input
                EOF
                git command-to-be-tested <input-for-test >actual &&
                cat >expected <<-\EOF &&
                here comes expected output
                EOF
                test_cmp expected actual
        '

not like this:

        cat >input-for-test <<-\EOF &&
        here comes input
        EOF
        test_expect_success 'I test such and such ' '
                git command-to-be-tested <input-for-test >actual &&
                cat >expected <<-\EOF &&
                here comes expected output
                EOF
                test_cmp expected actual
        '

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

[Jeff King]

On Wed, May 21, 2014 at 02:58:42PM -0700, Junio C Hamano wrote:

> >> >  	! grep "^-- \$" output
> > ...
> >> 
> >> We have been trying not to do the above in recent test updates.  It
> >> would be nice if this set-up did not have to be outside of the usual
> >> test_expect_success structure.
> >> 
> >
> > Jeff caught those "! grep" instances in my patch.
> 
> Hmm, I didn't mean that one, and I do not offhand what is wrong
> about "! grep" that says "output should not contain this string".

I think he is responding to my earlier request to use test_must_fail
instead of "!".  But there is a subtlety there he does not know, which
is that we typically only use the former for git programs, and rely on
"!" for normal Unix commands.

> The problem is a "cat" you added outside test_expect_*; the recent
> push is to have as little executable outside them, especially the
> "set-up" code to prepare for the real tests.
> 
> i.e. we have been trying to write new tests (and convert old ones)
> like this:
> 
>         test_expect_success 'I test such and such ' '
>                 cat >input-for-test <<-\EOF &&
>                 here comes input
>                 EOF
>                 git command-to-be-tested <input-for-test >actual &&
>                 cat >expected <<-\EOF &&
>                 here comes expected output
>                 EOF
>                 test_cmp expected actual
>         '
> 
> not like this:
> 
>         cat >input-for-test <<-\EOF &&
>         here comes input
>         EOF
>         test_expect_success 'I test such and such ' '
>                 git command-to-be-tested <input-for-test >actual &&
>                 cat >expected <<-\EOF &&
>                 here comes expected output
>                 EOF
>                 test_cmp expected actual
>         '

Yeah, I noticed and gave a pass on this in earlier review, because the
file is used across many tests. So burying it in the first test that
uses it is probably a bad thing. However, it could go in its own setup
test.

-Peff

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

[Jeremiah Mahler]

On Wed, May 21, 2014 at 02:58:42PM -0700, Junio C Hamano wrote:
> Jeremiah Mahler <jmmahler@gmail.com> writes:
> 
...
> The problem is a "cat" you added outside test_expect_*; the recent
> push is to have as little executable outside them, especially the
> "set-up" code to prepare for the real tests.
> 
> i.e. we have been trying to write new tests (and convert old ones)
> like this:
> 
>         test_expect_success 'I test such and such ' '
>                 cat >input-for-test <<-\EOF &&
>                 here comes input
>                 EOF
>                 git command-to-be-tested <input-for-test >actual &&
>                 cat >expected <<-\EOF &&
>                 here comes expected output
>                 EOF
>                 test_cmp expected actual
>         '
> 
> not like this:
> 
>         cat >input-for-test <<-\EOF &&
>         here comes input
>         EOF
>         test_expect_success 'I test such and such ' '
>                 git command-to-be-tested <input-for-test >actual &&
>                 cat >expected <<-\EOF &&
>                 here comes expected output
>                 EOF
>                 test_cmp expected actual
>         '

Now I understand.

Below is one of the updated test cases.

test_expect_success 'format-patch --signature-file=mail-signature' '
	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
	git format-patch --stdout --signature-file=mail-signature -1 >output &&
	check_patch output &&
	sed -n -e "/^-- $/,\$p" <output | sed -e "1d" | sed -e "\$d" >output2 &&
	test_cmp mail-signature output2
'

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think he is responding to my earlier request to use test_must_fail
> instead of "!".  But there is a subtlety there he does not know, which
> is that we typically only use the former for git programs, and rely on
> "!" for normal Unix commands.

Ok, that explains it.  Thanks.

> Yeah, I noticed and gave a pass on this in earlier review, because the
> file is used across many tests. So burying it in the first test that
> uses it is probably a bad thing. However, it could go in its own setup
> test.

Yeah, placing it in its own setup may be the best.  There are quite
a many set-ups outside the tests in this script from the olden days,
so I am OK if left it as-is and have a separate clean-up patch after
this topic settles.  I am also OK to add a new one "the new right way"
so that a later clean-up patch does not have to change what is added
in this step.

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

[Jeremiah Mahler]

On Wed, May 21, 2014 at 03:15:55PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> Yeah, placing it in its own setup may be the best.  There are quite
> a many set-ups outside the tests in this script from the olden days,
> so I am OK if left it as-is and have a separate clean-up patch after
> this topic settles.  I am also OK to add a new one "the new right way"
> so that a later clean-up patch does not have to change what is added
> in this step.

I like the idea of limiting the scope of this data so it couldn't
inadvertently impact later tests.

But placing the same data inside multiple test cases creates duplication.

Is there a way to define data once for a limited set of tests?

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

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

[Junio C Hamano]

Jeremiah Mahler <jmmahler@gmail.com> writes:

> Below is one of the updated test cases.
>
> test_expect_success 'format-patch --signature-file=mail-signature' '
> 	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
> 	git format-patch --stdout --signature-file=mail-signature -1 >output &&
> 	check_patch output &&
> 	sed -n -e "/^-- $/,\$p" <output | sed -e "1d" | sed -e "\$d" >output2 &&
> 	test_cmp mail-signature output2
> '

Hmph, there are still few things I do not understand in the above.

 * Why does mail-signature file have a leading blank line?  Is it
   typical users would want to have one there?

 * Similarly, why does mail-signature file need a trailing blank
   line?  Is it usual users would want to have one there?

 * Why three sed in the pipeline?  Wouldn't

    sed -e "1,/^-- \$/d" <output | ...

   be more direct way to start the pipeline without having to strip
   the "-- \n" line with the extra one?

 * Given a mail-signature file that does not end with an incomplete
   line (i.e. we did not have to add the newline to make it
   complete), what are the expected lines in the output after the
   "-- \n" separator?

   Whatever it is, I think it is easier to read the tests if done
   like so:

     sed -e "1,/^-- \$/d" <output >actual &&
     {
         do something to turn mail-signature to what we expect
     } >expect &&
     test_cmp expect actual

   If that "do something" is "to append an extra newline", then it
   would make it a lot clear to do

     {
         cat mail-signature && echo
     } >expect

   than a 'sed -e "\$d"' tucked at the end of the pipeline that only
   tells us we are removing one line that has _something_ without
   explicitly saying what we are removing, no?

Thanks.



   

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

[Junio C Hamano]

Jeremiah Mahler <jmmahler@gmail.com> writes:

> On Wed, May 21, 2014 at 03:15:55PM -0700, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>> 
>> Yeah, placing it in its own setup may be the best.  There are quite
>> a many set-ups outside the tests in this script from the olden days,
>> so I am OK if left it as-is and have a separate clean-up patch after
>> this topic settles.  I am also OK to add a new one "the new right way"
>> so that a later clean-up patch does not have to change what is added
>> in this step.
>
> I like the idea of limiting the scope of this data so it couldn't
> inadvertently impact later tests.
>
> But placing the same data inside multiple test cases creates duplication.
>
> Is there a way to define data once for a limited set of tests?

That is what Jeff ment by "used across many tests. ... it could go
in its own setup".

In other words,

	test_expect_success 'prepare mail-signature input' '
        	cat >mail-signature <<-\EOF
                ...
                EOF
	'

	test_expect_success 'one test that uses mail-signature' '
        	use mail-signature &&
                test the output
	'

	test_expect_success 'another test that uses mail-signature' '
        	use mail-signature in a different way &&
                test the output
	'

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

[Jeremiah Mahler]

On Wed, May 21, 2014 at 03:37:11PM -0700, Junio C Hamano wrote:
> Jeremiah Mahler <jmmahler@gmail.com> writes:
> 
> > Below is one of the updated test cases.
> >
> > test_expect_success 'format-patch --signature-file=mail-signature' '
> > 	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
> > 	git format-patch --stdout --signature-file=mail-signature -1 >output &&
> > 	check_patch output &&
> > 	sed -n -e "/^-- $/,\$p" <output | sed -e "1d" | sed -e "\$d" >output2 &&
> > 	test_cmp mail-signature output2
> > '
> 
> Hmph, there are still few things I do not understand in the above.
> 
>  * Why does mail-signature file have a leading blank line?  Is it
>    typical users would want to have one there?
> 
>  * Similarly, why does mail-signature file need a trailing blank
>    line?  Is it usual users would want to have one there?
> 

I think whatever the user puts in their signature should be reproduced.
It is probably isn't common to have leading or trailing blank lines.
But if they are there they should be reproduced.

>  * Why three sed in the pipeline?  Wouldn't
> 
>     sed -e "1,/^-- \$/d" <output | ...
> 
>    be more direct way to start the pipeline without having to strip
>    the "-- \n" line with the extra one?
> 

Yes, that is much cleaner.

>  * Given a mail-signature file that does not end with an incomplete
>    line (i.e. we did not have to add the newline to make it
>    complete), what are the expected lines in the output after the
>    "-- \n" separator?
> 
>    Whatever it is, I think it is easier to read the tests if done
>    like so:
> 
>      sed -e "1,/^-- \$/d" <output >actual &&
>      {
>          do something to turn mail-signature to what we expect
>      } >expect &&
>      test_cmp expect actual
> 
>    If that "do something" is "to append an extra newline", then it
>    would make it a lot clear to do
> 
>      {
>          cat mail-signature && echo
>      } >expect
> 
>    than a 'sed -e "\$d"' tucked at the end of the pipeline that only
>    tells us we are removing one line that has _something_ without
>    explicitly saying what we are removing, no?
> 

This does make more sense.  Since Git prints a blank line when it prints
the signature, this test also adds a blank line to produce the same
expected result.  This is much better than deleting two lines for no
obvious reason.

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