From: Sean Allred <allred.sean@xxxxxxxxx>
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"):
> Signed-off-by: A U Thor <author@xxxxxxxxxxx>
> (cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)
> Signed-off-by: C O Mmitter <committer@xxxxxxxxxxx>
This will now show as
> Signed-off-by: A U Thor <author@xxxxxxxxxxx>
> Cherry-Picked-From-Commit: da39a3ee5e6b4b0d3255bfef95601890afd80709
> Signed-off-by: C O Mmitter <committer@xxxxxxxxxxx>
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
Signed-off-by: Sean Allred <allred.sean@xxxxxxxxx>
+ ## 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.
Signed-off-by: Sean Allred <allred.sean@xxxxxxxxx>
---
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(-)
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index fdcad3d2006..22217480e45 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -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`
diff --git a/sequencer.c b/sequencer.c
index bceb6abcb6c..410f8469379 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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);
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 3b0fa66c33d..958fa019aed 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -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' '
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index dd5d92ef302..809afba48e1 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -33,6 +33,10 @@ mesg_with_footer_sob="$mesg_with_footer
Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
mesg_with_cherry_footer="$mesg_with_footer_sob
+Cherry-Picked-From-Commit: da39a3ee5e6b4b0d3255bfef95601890afd80709
+Tested-by: C.U. Thor <cuthor@xxxxxxxxxxx>"
+
+mesg_with_old_cherry_footer="$mesg_with_footer_sob
(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)
Tested-by: C.U. Thor <cuthor@xxxxxxxxxxx>"
@@ -68,6 +72,8 @@ test_expect_success setup '
git reset --hard initial &&
test_commit "$mesg_with_cherry_footer" foo b mesg-with-cherry-footer &&
git reset --hard initial &&
+ test_commit "$mesg_with_old_cherry_footer" foo b mesg-with-old-cherry-footer &&
+ git reset --hard initial &&
test_config commit.cleanup verbatim &&
test_commit "$mesg_unclean" foo b mesg-unclean &&
test_unconfig commit.cleanup &&
@@ -82,7 +88,7 @@ test_expect_success 'cherry-pick -x inserts blank line after one line subject' '
cat <<-EOF >expect &&
$mesg_one_line
- (cherry picked from commit $sha1)
+ Cherry-Picked-From-Commit: $sha1
EOF
git log -1 --pretty=format:%B >actual &&
test_cmp expect actual
@@ -130,7 +136,7 @@ test_expect_success 'cherry-pick -x inserts blank line when conforming footer no
cat <<-EOF >expect &&
$mesg_no_footer
- (cherry picked from commit $sha1)
+ Cherry-Picked-From-Commit: $sha1
EOF
git log -1 --pretty=format:%B >actual &&
test_cmp expect actual
@@ -155,7 +161,7 @@ test_expect_success 'cherry-pick -x -s inserts blank line when conforming footer
cat <<-EOF >expect &&
$mesg_no_footer
- (cherry picked from commit $sha1)
+ Cherry-Picked-From-Commit: $sha1
Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
EOF
git log -1 --pretty=format:%B >actual &&
@@ -179,7 +185,7 @@ test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match commi
git cherry-pick -x -s mesg-with-footer &&
cat <<-EOF >expect &&
$mesg_with_footer
- (cherry picked from commit $sha1)
+ Cherry-Picked-From-Commit: $sha1
Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
EOF
git log -1 --pretty=format:%B >actual &&
@@ -202,7 +208,7 @@ test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo
git cherry-pick -x -s mesg-with-footer-sob &&
cat <<-EOF >expect &&
$mesg_with_footer_sob
- (cherry picked from commit $sha1)
+ Cherry-Picked-From-Commit: $sha1
Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
EOF
git log -1 --pretty=format:%B >actual &&
@@ -216,7 +222,7 @@ test_expect_success 'cherry-pick -x handles commits with no NL at end of message
git cherry-pick -x $sha1 &&
git log -1 --pretty=format:%B >actual &&
- printf "\n(cherry picked from commit %s)\n" $sha1 >>msg &&
+ printf "\nCherry-Picked-From-Commit: %s\n" $sha1 >>msg &&
test_cmp msg actual
'
@@ -227,7 +233,7 @@ test_expect_success 'cherry-pick -x handles commits with no footer and no NL at
git cherry-pick -x $sha1 &&
git log -1 --pretty=format:%B >actual &&
- printf "\n\n(cherry picked from commit %s)\n" $sha1 >>msg &&
+ printf "\n\nCherry-Picked-From-Commit: %s\n" $sha1 >>msg &&
test_cmp msg actual
'
@@ -253,19 +259,19 @@ test_expect_success 'cherry-pick -s handles commits with no footer and no NL at
test_cmp msg actual
'
-test_expect_success 'cherry-pick -x treats "(cherry picked from..." line as part of footer' '
+test_expect_success 'cherry-pick -x treats "Cherry-Picked-From-Commit" line as part of footer' '
pristine_detach initial &&
sha1=$(git rev-parse mesg-with-cherry-footer^0) &&
git cherry-pick -x mesg-with-cherry-footer &&
cat <<-EOF >expect &&
$mesg_with_cherry_footer
- (cherry picked from commit $sha1)
+ Cherry-Picked-From-Commit: $sha1
EOF
git log -1 --pretty=format:%B >actual &&
test_cmp expect actual
'
-test_expect_success 'cherry-pick -s treats "(cherry picked from..." line as part of footer' '
+test_expect_success 'cherry-pick -s treats "Cherry-Picked-From-Commit" line as part of footer' '
pristine_detach initial &&
git cherry-pick -s mesg-with-cherry-footer &&
cat <<-EOF >expect &&
@@ -276,13 +282,26 @@ test_expect_success 'cherry-pick -s treats "(cherry picked from..." line as part
test_cmp expect actual
'
-test_expect_success 'cherry-pick -x -s treats "(cherry picked from..." line as part of footer' '
+test_expect_success 'cherry-pick -x -s treats "Cherry-Picked-From-Commit" line as part of footer' '
pristine_detach initial &&
sha1=$(git rev-parse mesg-with-cherry-footer^0) &&
git cherry-pick -x -s mesg-with-cherry-footer &&
cat <<-EOF >expect &&
$mesg_with_cherry_footer
- (cherry picked from commit $sha1)
+ Cherry-Picked-From-Commit: $sha1
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick -x -s still treats "(cherry picked from commit.." line as part of footer' '
+ pristine_detach initial &&
+ sha1=$(git rev-parse mesg-with-old-cherry-footer^0) &&
+ git cherry-pick -x -s mesg-with-old-cherry-footer &&
+ cat <<-EOF >expect &&
+ $mesg_with_old_cherry_footer
+ Cherry-Picked-From-Commit: $sha1
Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
EOF
git log -1 --pretty=format:%B >actual &&
@@ -303,7 +322,7 @@ test_expect_success 'cherry-pick -x cleans commit message' '
pristine_detach initial &&
git cherry-pick -x mesg-unclean &&
git log -1 --pretty=format:%B >actual &&
- printf "%s\n(cherry picked from commit %s)\n" \
+ printf "%s\nCherry-Picked-From-Commit: %s\n" \
"$mesg_unclean" $(git rev-parse mesg-unclean) |
git stripspace >expect &&
test_cmp expect actual
@@ -313,7 +332,7 @@ test_expect_success 'cherry-pick -x respects commit.cleanup' '
pristine_detach initial &&
git -c commit.cleanup=strip cherry-pick -x mesg-unclean &&
git log -1 --pretty=format:%B >actual &&
- printf "%s\n(cherry picked from commit %s)\n" \
+ printf "%s\nCherry-Picked-From-Commit: %s\n" \
"$mesg_unclean" $(git rev-parse mesg-unclean) |
git stripspace -s >expect &&
test_cmp expect actual
diff --git a/trailer.c b/trailer.c
index a2c3ed6f28c..59f7ef92b29 100644
--- a/trailer.c
+++ b/trailer.c
@@ -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
