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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 26, 2017, 00:07 UTC
Message-ID: <20170426000741.GS28740@aiede.svl.corp.google.com>
In-Reply-To: <20170425235741.32546-1-jonathantanmy@google.com>

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

[...]

Show 9 quoted lines

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

Thanks, Jonathan