From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 21 May 2014 22:37:11 GMT
Subject: Re: [PATCH v6] format-patch --signature-file <file>
Message-ID: <xmqqbnuqoj1k.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140521221221.GC13956@hudson.localdomain>

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.