Re: [PATCH v2] sequencer: require trailing NL in footers

[Junio C Hamano <gitster@pobox.com>]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jonathan Tan wrote:
>
>> Reported-by: Brian Norris <computersforpeace@gmail.com>
>> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
>> ---
> [...]
>>  sequencer.c              | 11 +++++++++++
>>  t/t3511-cherry-pick-x.sh | 14 ++++++++++++++
>>  2 files changed, 25 insertions(+)
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> This is still pretty subtle (using the added newline that is added after
> a non-footer to turn the invalid footer into a valid one), but
>
>  * it is clear from the code that it will work correctly
>  * the new test ensures we'll continue to support this case
>  * it is understandable after a little head scratching
>  * I'm hoping the subtlety will go away once the code learns to deal
>    with ordinary non-footer text that has a missing newline

Hmph, perhaps we need another test that documents a known failure as
well in the meantime?

> [...]
>> --- a/t/t3511-cherry-pick-x.sh
>> +++ b/t/t3511-cherry-pick-x.sh
>> @@ -208,6 +208,20 @@ test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo
>>  	test_cmp expect actual
>>  '
>>  
>> +test_expect_success 'cherry-pick -x handles commits with no NL at end of message' '
>> +	pristine_detach initial &&
>> +	sha1=$(printf "title\n\nSigned-off-by: a" | git commit-tree -p initial mesg-with-footer^{tree}) &&
>
> nit: Should this use a more typical sign-off line with an email
> address, to avoid a false-positive success in case git becomes more
> strict about its signoffs in the future?
>
> Something like
>
> 	printf "title\n\nSigned-off-by: S. I. Gner <signer@example.com>" >msg &&
> 	sha1=$(git commit-tree -p initial mesg-with-footer^{tree} <msg) &&
> 	...

That is a good point and has an added benefit that the test script
becomes easier to follow.

 t/t3511-cherry-pick-x.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 6f518020b2..c2b143802d 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -210,12 +210,14 @@ test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo
 
 test_expect_success 'cherry-pick -x handles commits with no NL at end of message' '
 	pristine_detach initial &&
-	sha1=$(printf "title\n\nSigned-off-by: a" | git commit-tree -p initial mesg-with-footer^{tree}) &&
+	signer="S. I. Gner <signer@example.com>" &&
+	printf "title\n\nSigned-off-by: %s" "$signer" >msg &&
+	sha1=$(git commit-tree -p initial mesg-with-footer^{tree} <msg) &&
 	git cherry-pick -x $sha1 &&
 	cat <<-EOF >expect &&
 		title
 
-		Signed-off-by: a
+		Signed-off-by: $signer
 		(cherry picked from commit $sha1)
 	EOF
 	git log -1 --pretty=format:%B >actual &&