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

From: Jeremiah Mahler <jmmahler@gmail.com>
Date: May 21, 2014, 23:18 UTC
Message-ID: <20140521231819.GA24756@hudson.localdomain>
In-Reply-To: <xmqqbnuqoj1k.fsf@gitster.dls.corp.google.com>

On Wed, May 21, 2014 at 03:37:11PM -0700, Junio C Hamano wrote:

Show 27 quoted lines

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

Show 7 quoted lines

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

Show 25 quoted lines

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