cherry-pick: use trailer instead of free-text for `-x`

From: Sean Allred <[email protected]>

When recording the origin commit during a cherry-pick, the current label used is not understood by git-interpret-trailers. Standardize onto the 'normal' trailer format that can be reasonably/reliably parsed and used by external tooling leveraging git-interpret-trailers.

This also somewhat improves the readability of resulting commit messages in some scenarios where trailers are already in use. Consider the example already present in cd650a4e (2023-02-12, "recognize '(cherry picked from ...' as part of s-o-b footer"):

This will now show as

Most tests are adjusted for the new format. A test is added to demonstrate that the old free-text format in existing commit data is still considered part of the trailer block (i.e., the problem fixed by the above commit has not been re-introduced).

---
cherry-pick: use trailer instead of free-text for -x

I considered (but did not pursue) a new configuration option for two reasons:

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1519%2Fvermiculus%2Fsa%2Fcherry-pick-origin-trailer-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1519/vermiculus/sa/cherry-pick-origin-trailer-v1
Pull-Request: https://github.com/git/git/pull/1519

1. Without regard to historical data, using a 'real' trailer seems inherently better than the current free-text state.

2. Regarding historical data, adding a user-configurable option doesn't make things simpler for systems maintainers; those systems still have to handle both formats if they have such a need to begin with. As it's still a clear and readable format, end-user developers are unlikely to care to change it back.

The maintenance and cognitive costs of a new configuration option are not worth the minimal benefit it seems it would bring.

---
 sequencer.c                     |  6 ++---
 t/t3510-cherry-pick-sequence.sh | 12 ++++-----
 t/t3511-cherry-pick-x.sh        | 47 +++++++++++++++++++++++----------
 3 files changed, 42 insertions(+), 23 deletions(-)

index bceb6abcb6c..410f8469379 100644
@@ -51,7 +51,7 @@
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
 static const char sign_off_header[] = "Signed-off-by: ";
-static const char cherry_picked_prefix[] = "(cherry picked from commit ";
+static const char cherry_picked_header[] = "Cherry-Picked-From-Commit: ";
 
 GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
 

@@ -2277,9 +2277,9 @@ static int do_pick_commit(struct repository *r,
 			strbuf_complete_line(&msgbuf);
 			if (!has_conforming_footer(&msgbuf, NULL, 0))
 				strbuf_addch(&msgbuf, '\n');
-			strbuf_addstr(&msgbuf, cherry_picked_prefix);
+			strbuf_addstr(&msgbuf, cherry_picked_header);
 			strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));
-			strbuf_addstr(&msgbuf, ")\n");
+			strbuf_addstr(&msgbuf, "\n");
 		}
 		if (!is_fixup(command))
 			author = get_author(msg.message);

index 3b0fa66c33d..958fa019aed 100755
@@ -548,10 +548,10 @@ test_expect_success '--continue respects opts' '
 	git cat-file commit HEAD~1 >picked_msg &&
 	git cat-file commit HEAD~2 >unrelatedpick_msg &&
 	git cat-file commit HEAD~3 >initial_msg &&
-	! grep "cherry picked from" initial_msg &&
-	grep "cherry picked from" unrelatedpick_msg &&
-	grep "cherry picked from" picked_msg &&
-	grep "cherry picked from" anotherpick_msg
+	! grep "Cherry-Picked-From-Commit" initial_msg &&
+	grep "Cherry-Picked-From-Commit" unrelatedpick_msg &&
+	grep "Cherry-Picked-From-Commit" picked_msg &&
+	grep "Cherry-Picked-From-Commit" anotherpick_msg
 '
 
 test_expect_success '--continue of single-pick respects -x' '

@@ -562,7 +562,7 @@ test_expect_success '--continue of single-pick respects -x' '
 	git cherry-pick --continue &&
 	test_path_is_missing .git/sequencer &&
 	git cat-file commit HEAD >msg &&
-	grep "cherry picked from" msg
+	grep "Cherry-Picked-From-Commit" msg
 '
 
 test_expect_success '--continue respects -x in first commit in multi-pick' '

@@ -574,7 +574,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '
 	test_path_is_missing .git/sequencer &&
 	git cat-file commit HEAD^ >msg &&
 	picked=$(git rev-parse --verify picked) &&
-	grep "cherry picked from.*$picked" msg
+	grep "Cherry-Picked-From-Commit: $picked" msg
 '
 
 test_expect_failure '--signoff is automatically propagated to resolved conflict' '

base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09

gitgitgadget

From: Sean Allred <[email protected]>

When recording the origin commit during a cherry-pick, the current label used is not understood by git-interpret-trailers. Standardize onto the 'normal' trailer format that can be reasonably/reliably parsed and used by external tooling leveraging git-interpret-trailers.

The prior language was introduced way back in 2005 (48313592, "Redo 'revert' using three-way merge machinery"), long before git-interpret-trailers was introduced in 2014 (dfd66ddf, "add documentation for 'git interpret-trailers'").

This also somewhat improves the readability of resulting commit messages in some scenarios where trailers are already in use. Consider the example already present in cd650a4e (2023-02-12, "recognize '(cherry picked from ...' as part of s-o-b footer"):

This will now show as

Most tests are adjusted for the new format. A test is added to demonstrate that the old free-text format in existing commit data is still considered part of the trailer block (i.e., the problem fixed by the above commit has not been re-introduced).

The change to trailer.c is not necessary for current tests to pass, but appear to be necessary to maintain the stated goal and semantics of the find_trailer_start with the addition of this new generated header. The old format is left, of course, to handle historical commit data.

---
cherry-pick: use trailer instead of free-text for -x

Sincere apologies for the very quick v2; while I've been sitting on this patch for a while in one form or another, I neglected to update the documentation. This has now been addressed, as well as addressing another reference to the old format in trailer.c. (I've now evaluated results of regexp searches of cherry.picked and picked.from; there is nothing left that seems to be necessary to update.)

I considered (but did not pursue) a new configuration option for two reasons:

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1519%2Fvermiculus%2Fsa%2Fcherry-pick-origin-trailer-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1519/vermiculus/sa/cherry-pick-origin-trailer-v2
Pull-Request: https://github.com/git/git/pull/1519

Range-diff vs v1:

1: 14c9e39be69 ! 1: b163a45b48a cherry-pick: use trailer instead of free-text for -x
@@ Commit message
'normal' trailer format that can be reasonably/reliably parsed and used by external tooling leveraging git-interpret-trailers.

+ The prior language was introduced way back in 2005 (48313592, "Redo
+ 'revert' using three-way merge machinery"), long before
+ git-interpret-trailers was introduced in 2014 (dfd66ddf, "add
+ documentation for 'git interpret-trailers'").
+
This also somewhat improves the readability of resulting commit messages in some scenarios where trailers are already in use. Consider the example already present in cd650a4e (2023-02-12, "recognize '(cherry
@@ Commit message
still considered part of the trailer block (i.e., the problem fixed by the above commit has not been re-introduced).

+ The change to trailer.c is not necessary for current tests to pass, but
+ appear to be necessary to maintain the stated goal and semantics of the
+ find_trailer_start with the addition of this new generated header. The
+ old format is left, of course, to handle historical commit data.
+
---

I considered (but did not pursue) a new configuration option for two
@@ Commit message

+ ## Documentation/git-cherry-pick.txt ##
+@@ Documentation/git-cherry-pick.txt: OPTIONS
+
+ -x::
+ When recording the commit, append a line that says
+- "(cherry picked from commit ...)" to the original commit
++ "Cherry-Picked-From-Commit:" to the original commit
+ message in order to indicate which commit this change was
+ cherry-picked from. This is done only for cherry
+ picks without conflicts. Do not use this option if
+@@ Documentation/git-cherry-pick.txt: OPTIONS
+ visible branches (e.g. backporting a fix to a
+ maintenance branch for an older release from a
+ development branch), adding this information can be
+- useful.
++ useful. See also linkgit:git-interpret-trailers[1].
+
+ -r::
+ It used to be that the command defaulted to do -x
+
## sequencer.c ##
@@
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
@@ t/t3511-cherry-pick-x.sh: test_expect_success 'cherry-pick -x respects commit.cl
"$mesg_unclean" $(git rev-parse mesg-unclean) |
git stripspace -s >expect &&
test_cmp expect actual
+
+ ## trailer.c ##
+@@ trailer.c: static int configured;
+ static const char *git_generated_prefixes[] = {
+ "Signed-off-by: ",
+ "(cherry picked from commit ",
++ "Cherry-Picked-From-Commit: ",
+ NULL
+ };
+

1. Without regard to historical data, using a 'real' trailer seems inherently better than the current free-text state.

2. Regarding historical data, adding a user-configurable option doesn't make things simpler for systems maintainers; those systems still have to handle both formats if they have such a need to begin with. As it's still a clear and readable format, end-user developers are unlikely to care to change it back.

The maintenance and cognitive costs of a new configuration option are not worth the minimal benefit it seems it would bring.

---
 Documentation/git-cherry-pick.txt |  4 +--
 sequencer.c                       |  6 ++--
 t/t3510-cherry-pick-sequence.sh   | 12 ++++----
 t/t3511-cherry-pick-x.sh          | 47 ++++++++++++++++++++++---------
 trailer.c                         |  1 +
 5 files changed, 45 insertions(+), 25 deletions(-)

index fdcad3d2006..22217480e45 100644
@@ -64,7 +64,7 @@ OPTIONS
 
 -x::
 	When recording the commit, append a line that says
-	"(cherry picked from commit ...)" to the original commit
+	"Cherry-Picked-From-Commit:" to the original commit
 	message in order to indicate which commit this change was
 	cherry-picked from.  This is done only for cherry
 	picks without conflicts.  Do not use this option if

@@ -74,7 +74,7 @@ OPTIONS
 	visible branches (e.g. backporting a fix to a
 	maintenance branch for an older release from a
 	development branch), adding this information can be
-	useful.
+	useful. See also linkgit:git-interpret-trailers[1].
 
 -r::
 	It used to be that the command defaulted to do `-x`

index bceb6abcb6c..410f8469379 100644
@@ -51,7 +51,7 @@
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
 static const char sign_off_header[] = "Signed-off-by: ";
-static const char cherry_picked_prefix[] = "(cherry picked from commit ";
+static const char cherry_picked_header[] = "Cherry-Picked-From-Commit: ";
 
 GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
 

@@ -2277,9 +2277,9 @@ static int do_pick_commit(struct repository *r,
 			strbuf_complete_line(&msgbuf);
 			if (!has_conforming_footer(&msgbuf, NULL, 0))
 				strbuf_addch(&msgbuf, '\n');
-			strbuf_addstr(&msgbuf, cherry_picked_prefix);
+			strbuf_addstr(&msgbuf, cherry_picked_header);
 			strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));
-			strbuf_addstr(&msgbuf, ")\n");
+			strbuf_addstr(&msgbuf, "\n");
 		}
 		if (!is_fixup(command))
 			author = get_author(msg.message);

index 3b0fa66c33d..958fa019aed 100755
@@ -548,10 +548,10 @@ test_expect_success '--continue respects opts' '
 	git cat-file commit HEAD~1 >picked_msg &&
 	git cat-file commit HEAD~2 >unrelatedpick_msg &&
 	git cat-file commit HEAD~3 >initial_msg &&
-	! grep "cherry picked from" initial_msg &&
-	grep "cherry picked from" unrelatedpick_msg &&
-	grep "cherry picked from" picked_msg &&
-	grep "cherry picked from" anotherpick_msg
+	! grep "Cherry-Picked-From-Commit" initial_msg &&
+	grep "Cherry-Picked-From-Commit" unrelatedpick_msg &&
+	grep "Cherry-Picked-From-Commit" picked_msg &&
+	grep "Cherry-Picked-From-Commit" anotherpick_msg
 '
 
 test_expect_success '--continue of single-pick respects -x' '

@@ -562,7 +562,7 @@ test_expect_success '--continue of single-pick respects -x' '
 	git cherry-pick --continue &&
 	test_path_is_missing .git/sequencer &&
 	git cat-file commit HEAD >msg &&
-	grep "cherry picked from" msg
+	grep "Cherry-Picked-From-Commit" msg
 '
 
 test_expect_success '--continue respects -x in first commit in multi-pick' '

@@ -574,7 +574,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '
 	test_path_is_missing .git/sequencer &&
 	git cat-file commit HEAD^ >msg &&
 	picked=$(git rev-parse --verify picked) &&
-	grep "cherry picked from.*$picked" msg
+	grep "Cherry-Picked-From-Commit: $picked" msg
 '
 
 test_expect_failure '--signoff is automatically propagated to resolved conflict' '

index a2c3ed6f28c..59f7ef92b29 100644
@@ -53,6 +53,7 @@ static int configured;
 static const char *git_generated_prefixes[] = {
 	"Signed-off-by: ",
 	"(cherry picked from commit ",
+	"Cherry-Picked-From-Commit: ",
 	NULL
 };
 

base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09

gitgitgadget

I am somewhat negative on going in this direction, as we originally added these "cherry-picked-from" by default, but stopped doing so for a reason [*1*]. I'd be hesitant to see us spend any engineering resources on a feature we discourage (not even to deprecate and to remove). It is a different story if we change the previous stance on the "cherry-picked-from" information, though.

I admit I've suggested "Cherry-picked-from:" long time ago [*2*], as an aside in a discussion, but the discussion was more about treating the line as a very distinct thing that is different from any other "trailer" lines. The last time this was brought up, I thought that it was deemed unnecessary because interpret-trailer code already understood by the trailer code [*3*], and we _could_ teach the interpret-trailer code to rewrite it to "Cherry-picked-from:". Any renewed efforts should build on the discussion there, addressing points raised during the discussion, I think.

Thanks.

[Footnotes and references]

*1* Unlike "revert", "cherry-pick" is done from an unrelated and
often not even published history, and referring to such a commit that the end-user cannot do "git show" does not add much value to the history.

*2* https://lore.kernel.org/git/[email protected]/
*3* https://lore.kernel.org/git/[email protected]/