git cherry-pick -x

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 <[email protected]>(cherry picked from commit 7b309aef0463340d3ad5449d1f605d14e10a4225)

On 2.9.3, this worked fine:

Acked-by Arnd Bergmann <[email protected]>

(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

+Jonathan, who worked on trailers
[...]

Quoted 2 lines

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

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 <[email protected]>

Signed-off-by Jonathan Tan <[email protected]>

---

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?

sequencer.c static const char *get_todo_path(const struct replay_opts *opts) · line 160

Quoted 12 lines

sequencer.c +6 -0

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

Quoted 3 lines · Stefan Beller

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

Quoted 8 lines · Jonathan Tan

Quoted 3 lines · Stefan Beller

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

Thanks!

Quoted 4 lines

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?)

Quoted 4 lines · Jonathan Tan

Quoted 3 lines · Stefan Beller

Quoted 2 lines

Do we want to test for this use case in the future?

Quoted 4 lines

Quoted 4 lines

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.

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?)

Quoted 2 lines · Stefan Beller

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

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.

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 <[email protected]>

Signed-off-by Jonathan Tan <[email protected]>

---

sequencer.c +6 -0

@@ -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)

Reported-by Brian Norris <[email protected]>

Signed-off-by Jonathan Tan <[email protected]>

---

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

Quoted 3 lines · Jonathan Tan

Reported-by Brian Norris <[email protected]>

Signed-off-by Jonathan Tan <[email protected]>

---

[...]

Quoted 3 lines

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

Reviewed-by Jonathan Nieder <[email protected]>

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

[...]

t/t3511-cherry-pick-x.sh test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo · line 213

Quoted 9 lines

t/t3511-cherry-pick-x.sh +3 -0

@@ -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 <[email protected]>" >msg &&
	sha1=$(git commit-tree -p initial mesg-with-footer^{tree} <msg) &&
	...

Quoted 17 lines · Jonathan Nieder

Jonathan Tan wrote:

[...]

t/t3511-cherry-pick-x.sh test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo · line 213

Quoted 9 lines

t/t3511-cherry-pick-x.sh +3 -0

@@ -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?

Quoted 5 lines

Something like

	printf "title\n\nSigned-off-by: S. I. Gner <[email protected]>" >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 +2 -1

index 6f518020b2..c2b143802d 100755
@@ -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 <[email protected]>" &&
+	printf "title\n\nSigned-off-by: %s" "$signer" >msg &&