[GIT 2.12.2 REGRESSION] git cherry-pick -x

[GIT 2.12.2 REGRESSION] git cherry-pick -x

[Brian Norris]

Hi all,

I haven't tried bisecting precisely, but somewhere along the way git
cherry-pick -x regressed between 2.9.3 and 2.12.2.

Looking at upstream linux.git, I can do:

git fetch git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
git cherry-pick -x 7b309aef0463340d3ad5449d1f605d14e10a4225

And see the following ending to the new commit:

Acked-by: Arnd Bergmann <arnd@arndb.de>(cherry picked from commit 7b309aef0463340d3ad5449d1f605d14e10a4225)

On 2.9.3, this worked fine:

Acked-by: Arnd Bergmann <arnd@arndb.de>
(cherry picked from commit 7b309aef0463340d3ad5449d1f605d14e10a4225)

This issue doesn't happen on all commits; maybe it's bad parsing looking
at 'Signed-off' and the like?

Regards,
Brian

Re: [GIT 2.12.2 REGRESSION] git cherry-pick -x

[Stefan Beller]

+Jonathan, who worked on trailers

On Fri, Apr 21, 2017 at 3:01 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> Hi all,
>
> I haven't tried bisecting precisely, but somewhere along the way git
> cherry-pick -x regressed between 2.9.3 and 2.12.2.
>
> Looking at upstream linux.git, I can do:
>
> git fetch git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
> git cherry-pick -x 7b309aef0463340d3ad5449d1f605d14e10a4225
>
> And see the following ending to the new commit:
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>(cherry picked from commit 7b309aef0463340d3ad5449d1f605d14e10a4225)
>
> On 2.9.3, this worked fine:
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> (cherry picked from commit 7b309aef0463340d3ad5449d1f605d14e10a4225)
>
> This issue doesn't happen on all commits; maybe it's bad parsing looking
> at 'Signed-off' and the like?
>
> Regards,
> Brian

Re: [GIT 2.12.2 REGRESSION] git cherry-pick -x

[Jeff King]

On Fri, Apr 21, 2017 at 03:10:03PM -0700, Stefan Beller wrote:

> +Jonathan, who worked on trailers
> [...]
> > I haven't tried bisecting precisely, but somewhere along the way git
> > cherry-pick -x regressed between 2.9.3 and 2.12.2.

Yeah, it bisects to 967dfd4d5 (sequencer: use trailer's trailer layout,
2016-11-02).

-Peff

[PATCH] sequencer: require trailing NL in footers

[Jonathan Tan]

In commit 967dfd4 ("sequencer: use trailer's trailer layout",
2016-11-29), sequencer was taught to use the same mechanism as
interpret-trailers to determine the nature of the trailer of a commit
message (referred to as the "footer" in sequencer.c). However, the
requirement that a footer end in a newline character was inadvertently
removed. Restore that requirement.

While writing this commit, I noticed that if the "ignore_footer"
parameter in "has_conforming_footer" is greater than the distance
between the trailer start and sb->len, "has_conforming_footer" will
return an unexpected result. This does not occur in practice, because
"ignore_footer" is either zero or the return value of an invocation to
"ignore_non_trailer", which only skips empty lines and comment lines.
This commit contains a comment explaining this in the function's
documentation.

Reported-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---

Thanks for the bug report. Here's a fix - I've verified this with the
way to reproduce provided in the original e-mail, and it seems to work
now.

The commit message of the referenced commit
(7b309aef0463340d3ad5449d1f605d14e10a4225) does not end in a newline,
which is probably why different behavior was observed with this commit
(as compared to others).

 sequencer.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 77afecaeb..4cbe64b7f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -151,6 +151,12 @@ static const char *get_todo_path(const struct replay_opts *opts)
  * Returns 1 for conforming footer
  * Returns 2 when sob exists within conforming footer
  * Returns 3 when sob exists within conforming footer as last entry
+ *
+ * A footer that does not end in a newline is considered non-conforming.
+ *
+ * ignore_footer, if not zero, should be the return value of an invocation to
+ * ignore_non_trailer. See the documentation of that function for more
+ * information.
  */
 static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
 	int ignore_footer)
@@ -159,6 +165,11 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
 	int i;
 	int found_sob = 0, found_sob_last = 0;
 
+	if (sb->len <= ignore_footer)
+		return 0;
+	if (sb->buf[sb->len - ignore_footer - 1] != '\n')
+		return 0;
+
 	trailer_info_get(&info, sb->buf);
 
 	if (info.trailer_start == info.trailer_end)
-- 
2.13.0.rc0.306.g87b477812d-goog

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

[Stefan Beller]

On Tue, Apr 25, 2017 at 12:06 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
> In commit 967dfd4 ("sequencer: use trailer's trailer layout",
> 2016-11-29), sequencer was taught to use the same mechanism as
> interpret-trailers to determine the nature of the trailer of a commit
> message (referred to as the "footer" in sequencer.c). However, the
> requirement that a footer end in a newline character was inadvertently
> removed. Restore that requirement.
>
> While writing this commit, I noticed that if the "ignore_footer"
> parameter in "has_conforming_footer" is greater than the distance
> between the trailer start and sb->len, "has_conforming_footer" will
> return an unexpected result. This does not occur in practice, because
> "ignore_footer" is either zero or the return value of an invocation to
> "ignore_non_trailer", which only skips empty lines and comment lines.
> This commit contains a comment explaining this in the function's
> documentation.
>
> Reported-by: Brian Norris <computersforpeace@gmail.com>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>
> Thanks for the bug report. Here's a fix - I've verified this with the
> way to reproduce provided in the original e-mail, and it seems to work
> now.
>
> The commit message of the referenced commit
> (7b309aef0463340d3ad5449d1f605d14e10a4225) does not end in a newline,
> which is probably why different behavior was observed with this commit
> (as compared to others).

Thanks for the fix. :)
Do we want to test for this use case in the future?

> @@ -151,6 +151,12 @@ static const char *get_todo_path(const struct replay_opts *opts)
>   * Returns 1 for conforming footer
>   * Returns 2 when sob exists within conforming footer
>   * Returns 3 when sob exists within conforming footer as last entry
> + *
> + * A footer that does not end in a newline is considered non-conforming.
> + *
> + * ignore_footer, if not zero, should be the return value of an invocation to
> + * ignore_non_trailer. See the documentation of that function for more
> + * information.
>   */

Makes sense. Maybe s/ignore_non_trailer/ignore_non_trailer()/ which makes
it easier to recognize it as a function? I'd also drop the last
sentence as it is
implied in the previous sentence (sort of).

Thanks,
Stefan

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

[Jonathan Tan]

On 04/25/2017 02:04 PM, Stefan Beller wrote:
> Thanks for the fix. :)
> Do we want to test for this use case in the future?

Thanks!

I'm not sure of the value of including a test for this specific use 
case, because Git normally does not create commit messages with no 
trailing newlines. (To test this, I suspect I would need to use 
hash-object with a specifically crafted commit object.)

>
>> @@ -151,6 +151,12 @@ static const char *get_todo_path(const struct replay_opts *opts)
>>   * Returns 1 for conforming footer
>>   * Returns 2 when sob exists within conforming footer
>>   * Returns 3 when sob exists within conforming footer as last entry
>> + *
>> + * A footer that does not end in a newline is considered non-conforming.
>> + *
>> + * ignore_footer, if not zero, should be the return value of an invocation to
>> + * ignore_non_trailer. See the documentation of that function for more
>> + * information.
>>   */
>
> Makes sense. Maybe s/ignore_non_trailer/ignore_non_trailer()/ which makes
> it easier to recognize it as a function? I'd also drop the last
> sentence as it is
> implied in the previous sentence (sort of).

OK, I'll add the parentheses if I need to reroll. As for the last 
sentence, the documentation for ignore_non_trailer is a bit unusual in 
that it says specifically "to be fed as the second parameter to 
append_signoff()", so I want to call extra attention to that.

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

[Stefan Beller]

On Tue, Apr 25, 2017 at 2:51 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
> On 04/25/2017 02:04 PM, Stefan Beller wrote:
>>
>> Thanks for the fix. :)
>> Do we want to test for this use case in the future?
>
>
> Thanks!
>
> I'm not sure of the value of including a test for this specific use case,
> because Git normally does not create commit messages with no trailing
> newlines. (To test this, I suspect I would need to use hash-object with a
> specifically crafted commit object.)

Okay, makes sense to omit a test.
In that case: Is it needed to hint at how this bug occurred in the wild?
(A different Git implementation, which may be fixed now?)

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

[Johannes Schindelin]

Hi Stefan,

On Tue, 25 Apr 2017, Stefan Beller wrote:

> On Tue, Apr 25, 2017 at 2:51 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
> > On 04/25/2017 02:04 PM, Stefan Beller wrote:
> >>
> >> Thanks for the fix. :)
> >> Do we want to test for this use case in the future?
> >
> >
> > Thanks!
> >
> > I'm not sure of the value of including a test for this specific use case,
> > because Git normally does not create commit messages with no trailing
> > newlines. (To test this, I suspect I would need to use hash-object with a
> > specifically crafted commit object.)
> 
> Okay, makes sense to omit a test.
> In that case: Is it needed to hint at how this bug occurred in the wild?
> (A different Git implementation, which may be fixed now?)

Just because Git usually does not create commit messages without trailing
newlines does not mean that it is a rare thing. We got bug reports about
this, so I think it is frequent enough that we could save time by adding
that test and avoid future bug reports/bug hunts.

Ciao,
Johannes

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

[Jonathan Nieder]

Johannes Schindelin wrote:
>> On Tue, Apr 25, 2017 at 2:51 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
>>> On 04/25/2017 02:04 PM, Stefan Beller wrote:

>>>> Do we want to test for this use case in the future?
[...]
>>> I'm not sure of the value of including a test for this specific use case,
>>> because Git normally does not create commit messages with no trailing
>>> newlines. (To test this, I suspect I would need to use hash-object with a
>>> specifically crafted commit object.)
[...]
> Just because Git usually does not create commit messages without trailing
> newlines does not mean that it is a rare thing. We got bug reports about
> this, so I think it is frequent enough that we could save time by adding
> that test and avoid future bug reports/bug hunts.

To put it another way: it wouldn't have been worth writing this patch
if it wasn't worth keeping the behavior.  So we do need a test.

Thanks,
Jonathan

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

[Brian Norris]

On Tue, Apr 25, 2017 at 02:56:53PM -0700, Stefan Beller wrote:
> In that case: Is it needed to hint at how this bug occurred in the wild?
> (A different Git implementation, which may be fixed now?)

I might contact the original author, but it's easy enough to imagine
automated 'filter-branch' scripts that could produce this. e.g., this
trivial example:

git filter-branch --msg-filter "perl -pe 'chomp if eof'" HEAD^..

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

[Brian Norris]

On Tue, Apr 25, 2017 at 03:30:56PM -0700, Brian Norris wrote:
> On Tue, Apr 25, 2017 at 02:56:53PM -0700, Stefan Beller wrote:
> > In that case: Is it needed to hint at how this bug occurred in the wild?
> > (A different Git implementation, which may be fixed now?)
> 
> I might contact the original author

Original author was using 'stgit', FYI.

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

[Johannes Schindelin]

Hi Jonathan,

On Tue, 25 Apr 2017, Jonathan Tan wrote:

> Thanks for the bug report. Here's a fix - I've verified this with the
> way to reproduce provided in the original e-mail, and it seems to work
> now.

If there is a straight-forward way to convert the manual test into an
automated one, it would be good to make it part of the patch so that we
can stave off future regressions of the fix.

Ciao,
Dscho

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

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> In commit 967dfd4 ("sequencer: use trailer's trailer layout",
> 2016-11-29), sequencer was taught to use the same mechanism as
> interpret-trailers to determine the nature of the trailer of a commit
> message (referred to as the "footer" in sequencer.c). However, the
> requirement that a footer end in a newline character was inadvertently
> removed. Restore that requirement.
>
> While writing this commit, I noticed that if the "ignore_footer"
> parameter in "has_conforming_footer" is greater than the distance
> between the trailer start and sb->len, "has_conforming_footer" will
> return an unexpected result. This does not occur in practice, because
> "ignore_footer" is either zero or the return value of an invocation to
> "ignore_non_trailer", which only skips empty lines and comment lines.
> This commit contains a comment explaining this in the function's
> documentation.
>
> Reported-by: Brian Norris <computersforpeace@gmail.com>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
[...]
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -151,6 +151,12 @@ static const char *get_todo_path(const struct replay_opts *opts)
>   * Returns 1 for conforming footer
>   * Returns 2 when sob exists within conforming footer
>   * Returns 3 when sob exists within conforming footer as last entry
> + *
> + * A footer that does not end in a newline is considered non-conforming.
> + *
> + * ignore_footer, if not zero, should be the return value of an invocation to
> + * ignore_non_trailer. See the documentation of that function for more
> + * information.
>   */
>  static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
>  	int ignore_footer)
> @@ -159,6 +165,11 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
>  	int i;
>  	int found_sob = 0, found_sob_last = 0;
>  
> +	if (sb->len <= ignore_footer)
> +		return 0;
> +	if (sb->buf[sb->len - ignore_footer - 1] != '\n')
> +		return 0;
> +

This is super subtle, but it does the right thing.  The caller will
notice it's not a conforming footer, add a newline to separate the new
footer from it, and repair the footer as a side effect.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Followup question: what should happen if there is a non-footer-shaped
thing with no trailing newline at the end of the commit message? Should
we add two newlines in that case when producing a new footer?

Thanks,
Jonathan

[PATCH v2] sequencer: require trailing NL in footers

[Jonathan Tan]

In commit 967dfd4 ("sequencer: use trailer's trailer layout",
2016-11-29), sequencer was taught to use the same mechanism as
interpret-trailers to determine the nature of the trailer of a commit
message (referred to as the "footer" in sequencer.c). However, the
requirement that a footer end in a newline character was inadvertently
removed. Restore that requirement.

While writing this commit, I noticed that if the "ignore_footer"
parameter in "has_conforming_footer" is greater than the distance
between the trailer start and sb->len, "has_conforming_footer" will
return an unexpected result. This does not occur in practice, because
"ignore_footer" is either zero or the return value of an invocation to
"ignore_non_trailer", which only skips empty lines and comment lines.
This commit contains a comment explaining this in the function's
documentation.

Reported-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---

jrnieder pointed out the existence of commit-tree to me, which I have
used to write the test below.

Changes from v1:
 - added test
 - used one of sbeller's documentation suggestions

 sequencer.c              | 11 +++++++++++
 t/t3511-cherry-pick-x.sh | 14 ++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 77afecaeb..500a76260 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -151,6 +151,12 @@ static const char *get_todo_path(const struct replay_opts *opts)
  * Returns 1 for conforming footer
  * Returns 2 when sob exists within conforming footer
  * Returns 3 when sob exists within conforming footer as last entry
+ *
+ * A footer that does not end in a newline is considered non-conforming.
+ *
+ * ignore_footer, if not zero, should be the return value of an invocation to
+ * ignore_non_trailer(). See the documentation of that function for more
+ * information.
  */
 static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
 	int ignore_footer)
@@ -159,6 +165,11 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
 	int i;
 	int found_sob = 0, found_sob_last = 0;
 
+	if (sb->len <= ignore_footer)
+		return 0;
+	if (sb->buf[sb->len - ignore_footer - 1] != '\n')
+		return 0;
+
 	trailer_info_get(&info, sb->buf);
 
 	if (info.trailer_start == info.trailer_end)
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index bf0a5c988..6f518020b 100755
--- 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}) &&
+	git cherry-pick -x $sha1 &&
+	cat <<-EOF >expect &&
+		title
+
+		Signed-off-by: a
+		(cherry picked from commit $sha1)
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick -x treats "(cherry picked from..." line as part of footer' '
 	pristine_detach initial &&
 	sha1=$(git rev-parse mesg-with-cherry-footer^0) &&
-- 
2.13.0.rc0.306.g87b477812d-goog

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

[Jonathan Nieder]

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

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

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

[Junio C Hamano]

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

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

[Johannes Schindelin]

Hi Junio,

On Tue, 25 Apr 2017, Junio C Hamano wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > Jonathan Tan wrote:
> >
> > [...]
> >> --- 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.

If you already try to make it easier to follow, you might just as well go
the whole nine yards:

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

It is even easier to read `--format=%B` than `--pretty=format:%B`, and as
`%B` does *not* indent, the indentation in the lines writing the `expect`
file is bogus anyway. And with the `msg` file having most of the stuff
already, we Do Not Need To Repeat Ourselves:

	printf '\n(cherry picked from commit %s)\n' $sha1 >>msg &&
	git log -1 --format=%B >actual &&
	test_cmp msg actual

Ciao,
Dscho

[PATCH v3] sequencer: add newline before adding footers

[Jonathan Tan]

When encountering a commit message that does not end in a newline,
sequencer does not complete the line before determining if a blank line
should be added.  This causes the "(cherry picked..." and sign-off lines
to sometimes appear on the same line as the last line of the commit
message.

This behavior was introduced by commit 967dfd4 ("sequencer: use
trailer's trailer layout", 2016-11-29). However, a revert of that commit
would not resolve this issue completely: prior to that commit, a
conforming footer was deemed to be non-conforming by
has_conforming_footer() if there was no terminating newline, resulting
in both conforming and non-conforming footers being treated the same
when they should not be.

Resolve this issue, both for conforming and non-conforming footers, and
in both do_pick_commit() and append_signoff(), by always adding a
newline to the commit message if it does not end in one before checking
the footer for conformity.

Reported-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---

The failure in the case of non-footers not being terminated by a newline
turns out to be quite easy to fix, so I've added that fix here. I think
this makes the overall fix more obvious too.

I've used Jonathan Nieder's and Dscho's suggestions for the tests
(except for --format vs --pretty, since --format seems to add an extra
newline to the output).

 sequencer.c              | 11 ++++-------
 t/t3511-cherry-pick-x.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 77afecaeb..ffac95545 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1045,6 +1045,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 			strbuf_addstr(&msgbuf, p);
 
 		if (opts->record_origin) {
+			strbuf_complete_line(&msgbuf);
 			if (!has_conforming_footer(&msgbuf, NULL, 0))
 				strbuf_addch(&msgbuf, '\n');
 			strbuf_addstr(&msgbuf, cherry_picked_prefix);
@@ -2335,6 +2336,9 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
 				getenv("GIT_COMMITTER_EMAIL")));
 	strbuf_addch(&sob, '\n');
 
+	if (!ignore_footer)
+		strbuf_complete_line(msgbuf);
+
 	/*
 	 * If the whole message buffer is equal to the sob, pretend that we
 	 * found a conforming footer with a matching sob
@@ -2355,13 +2359,6 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
 			 * the title and body to be filled in by the user.
 			 */
 			append_newlines = "\n\n";
-		} else if (msgbuf->buf[len - 1] != '\n') {
-			/*
-			 * Incomplete line.  Complete the line and add a
-			 * blank one so that there is an empty line between
-			 * the message body and the sob.
-			 */
-			append_newlines = "\n\n";
 		} else if (len == 1) {
 			/*
 			 * Buffer contains a single newline.  Add another
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index bf0a5c988..9888bf34b 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -208,6 +208,50 @@ 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 &&
+	printf "title\n\nSigned-off-by: A <a@example.com>" >msg &&
+	sha1=$(git commit-tree -p initial mesg-with-footer^{tree} <msg) &&
+	git cherry-pick -x $sha1 &&
+	git log -1 --pretty=format:%B >actual &&
+
+	printf "\n(cherry picked from commit %s)\n" $sha1 >>msg &&
+	test_cmp msg actual
+'
+
+test_expect_success 'cherry-pick -x handles commits with no footer and no NL at end of message' '
+	pristine_detach initial &&
+	printf "title\n\nnot a footer" >msg &&
+	sha1=$(git commit-tree -p initial mesg-with-footer^{tree} <msg) &&
+	git cherry-pick -x $sha1 &&
+	git log -1 --pretty=format:%B >actual &&
+
+	printf "\n\n(cherry picked from commit %s)\n" $sha1 >>msg &&
+	test_cmp msg actual
+'
+
+test_expect_success 'cherry-pick -s handles commits with no NL at end of message' '
+	pristine_detach initial &&
+	printf "title\n\nSigned-off-by: A <a@example.com>" >msg &&
+	sha1=$(git commit-tree -p initial mesg-with-footer^{tree} <msg) &&
+	git cherry-pick -s $sha1 &&
+	git log -1 --pretty=format:%B >actual &&
+
+	printf "\nSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n" >>msg &&
+	test_cmp msg actual
+'
+
+test_expect_success 'cherry-pick -s handles commits with no footer and no NL at end of message' '
+	pristine_detach initial &&
+	printf "title\n\nnot a footer" >msg &&
+	sha1=$(git commit-tree -p initial mesg-with-footer^{tree} <msg) &&
+	git cherry-pick -s $sha1 &&
+	git log -1 --pretty=format:%B >actual &&
+
+	printf "\n\nSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n" >>msg &&
+	test_cmp msg actual
+'
+
 test_expect_success 'cherry-pick -x treats "(cherry picked from..." line as part of footer' '
 	pristine_detach initial &&
 	sha1=$(git rev-parse mesg-with-cherry-footer^0) &&
-- 
2.13.0.rc0.306.g87b477812d-goog