| Message ID | 20181104072253.12357-1-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines | expand |
On 04/11/2018 07:22, Nguyễn Thái Ngọc Duy wrote: > When a commit is reverted (or cherry-picked with -x) we add an English > sentence recording that commit id in the new commit message. Make > these real trailer lines instead so that they are more friendly to > parsers (especially "git interpret-trailers"). > > A reverted commit will have a new trailer > > Revert: <commit-id> > > Similarly a cherry-picked commit with -x will have > > Cherry-Pick: <commit-id> I think this is a good idea though I wonder if it will break someones script that is looking for the messages generated by -x at the moment. I've got a couple of comments below. > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > I think standardizing how we record commit ids in the commit message > is a good idea. Though to be honest this started because of my irk of > an English string "cherry picked from..." that cannot be translated. > It might as well be a computer language that happens to look like > English. > > Documentation/git-cherry-pick.txt | 5 ++--- > sequencer.c | 20 ++++++-------------- > t/t3510-cherry-pick-sequence.sh | 12 ++++++------ > t/t3511-cherry-pick-x.sh | 26 +++++++++++++------------- > 4 files changed, 27 insertions(+), 36 deletions(-) > > diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt > index d35d771fc8..8ffbaed1a0 100644 > --- a/Documentation/git-cherry-pick.txt > +++ b/Documentation/git-cherry-pick.txt > @@ -58,9 +58,8 @@ OPTIONS > message prior to committing. > > -x:: > - When recording the commit, append a line that says > - "(cherry picked from commit ...)" to the original commit > - message in order to indicate which commit this change was > + When recording the commit, append "Cherry-Pick:" trailer line > + recording the commit name which commit this change was > cherry-picked from. This is done only for cherry > picks without conflicts. Do not use this option if > you are cherry-picking from your private branch because > diff --git a/sequencer.c b/sequencer.c > index 9e1ab3a2a7..f7318f2862 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -36,7 +36,6 @@ > #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" > > const char sign_off_header[] = "Signed-off-by: "; > -static const char cherry_picked_prefix[] = "(cherry picked from commit "; > > GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG") > > @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, > base_label = msg.label; > next = parent; > next_label = msg.parent_label; > - strbuf_addstr(&msgbuf, "Revert \""); > - strbuf_addstr(&msgbuf, msg.subject); > - strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit "); > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); > - > - if (commit->parents && commit->parents->next) { > - strbuf_addstr(&msgbuf, ", reversing\nchanges made to "); > - strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid)); > - } As revert currently records the parent given on the command line when reverting a merge commit it would probably be a good idea to add that either as a separate trailer or to the Revert: trailer and possibly also generate it for cherry picks. > - strbuf_addstr(&msgbuf, ".\n"); > + strbuf_addf(&msgbuf, "Revert \"%s\"\n\n", msg.subject); If the message already contains trailers then should we just append the Revert trailer those rather than inserting "\n\n"? Best Wishes Phillip > + > + strbuf_addf(&msgbuf, "Revert: %s\n", > + oid_to_hex(&commit->object.oid)); > } else { > const char *p; > > @@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, > strbuf_complete_line(&msgbuf); > if (!has_conforming_footer(&msgbuf, NULL, 0)) > strbuf_addch(&msgbuf, '\n'); > - strbuf_addstr(&msgbuf, cherry_picked_prefix); > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); > - strbuf_addstr(&msgbuf, ")\n"); > + strbuf_addf(&msgbuf, "Cherry-Pick: %s\n", > + oid_to_hex(&commit->object.oid)); > } > 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 c84eeefdc9..89b6e7fc1e 100755 > --- a/t/t3510-cherry-pick-sequence.sh > +++ b/t/t3510-cherry-pick-sequence.sh > @@ -390,10 +390,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-Pick:" initial_msg && > + grep "Cherry-Pick: " unrelatedpick_msg && > + grep "Cherry-Pick: " picked_msg && > + grep "Cherry-Pick: " anotherpick_msg > ' > > test_expect_success '--continue of single-pick respects -x' ' > @@ -404,7 +404,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-Pick:" msg > ' > > test_expect_success '--continue respects -x in first commit in multi-pick' ' > @@ -416,7 +416,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-Pick: $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 9888bf34b9..db11dd2430 100755 > --- a/t/t3511-cherry-pick-x.sh > +++ b/t/t3511-cherry-pick-x.sh > @@ -32,7 +32,7 @@ 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) > +Cherry-Pick: da39a3ee5e6b4b0d3255bfef95601890afd80709 > Tested-by: C.U. Thor <cuthor@example.com>" > > mesg_unclean="$mesg_one_line > @@ -81,7 +81,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-Pick: $sha1 > EOF > git log -1 --pretty=format:%B >actual && > test_cmp expect actual > @@ -129,7 +129,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-Pick: $sha1 > EOF > git log -1 --pretty=format:%B >actual && > test_cmp expect actual > @@ -154,7 +154,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-Pick: $sha1 > Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> > EOF > git log -1 --pretty=format:%B >actual && > @@ -178,7 +178,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-Pick: $sha1 > Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> > EOF > git log -1 --pretty=format:%B >actual && > @@ -201,7 +201,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-Pick: $sha1 > Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> > EOF > git log -1 --pretty=format:%B >actual && > @@ -215,7 +215,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-Pick: %s\n" $sha1 >>msg && > test_cmp msg actual > ' > > @@ -226,7 +226,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-Pick: %s\n" $sha1 >>msg && > test_cmp msg actual > ' > > @@ -252,19 +252,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-Pick:" 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-Pick: $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-Pick:" line as part of footer' ' > pristine_detach initial && > git cherry-pick -s mesg-with-cherry-footer && > cat <<-EOF >expect && > @@ -275,13 +275,13 @@ 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-Pick:..." 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-Pick: $sha1 > Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> > EOF > git log -1 --pretty=format:%B >actual && >
On Sun, Nov 4, 2018 at 5:45 PM Phillip Wood <phillip.wood@talktalk.net> wrote: > > On 04/11/2018 07:22, Nguyễn Thái Ngọc Duy wrote: > > When a commit is reverted (or cherry-picked with -x) we add an English > > sentence recording that commit id in the new commit message. Make > > these real trailer lines instead so that they are more friendly to > > parsers (especially "git interpret-trailers"). > > > > A reverted commit will have a new trailer > > > > Revert: <commit-id> > > > > Similarly a cherry-picked commit with -x will have > > > > Cherry-Pick: <commit-id> > > I think this is a good idea though I wonder if it will break someones > script that is looking for the messages generated by -x at the moment. It will [1] but I still think it's worth the trouble. The script will be less likely to break after, and you can use git-interpret-trailers instead of plain grep. [1] https://public-inbox.org/git/20181017143921.GR270328@devbig004.ftw2.facebook.com/ > > @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, > > base_label = msg.label; > > next = parent; > > next_label = msg.parent_label; > > - strbuf_addstr(&msgbuf, "Revert \""); > > - strbuf_addstr(&msgbuf, msg.subject); > > - strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit "); > > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); > > - > > - if (commit->parents && commit->parents->next) { > > - strbuf_addstr(&msgbuf, ", reversing\nchanges made to "); > > - strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid)); > > - } > > As revert currently records the parent given on the command line when > reverting a merge commit it would probably be a good idea to add that > either as a separate trailer or to the Revert: trailer and possibly also > generate it for cherry picks. My mistake. I didn't read carefully and thought it was logging commit->parents, which is pointless. So what should be the trailer for this (I don't think putting it in Revert: is a good idea, too much to parse)? Revert-parent: ? Revert-merge: ? > > - strbuf_addstr(&msgbuf, ".\n"); > > + strbuf_addf(&msgbuf, "Revert \"%s\"\n\n", msg.subject); > > If the message already contains trailers then should we just append the > Revert trailer those rather than inserting "\n\n"? Umm.. but this \n\n is for separating the subject and the body. I think we need it anyway, trailer or not. > > @@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, > > strbuf_complete_line(&msgbuf); > > if (!has_conforming_footer(&msgbuf, NULL, 0)) > > strbuf_addch(&msgbuf, '\n'); > > - strbuf_addstr(&msgbuf, cherry_picked_prefix); > > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); > > - strbuf_addstr(&msgbuf, ")\n"); > > + strbuf_addf(&msgbuf, "Cherry-Pick: %s\n", > > + oid_to_hex(&commit->object.oid)); I will probably make this "Cherry-picked-from:" to match our S-o-b style.
Hi Duy On 04/11/2018 17:41, Duy Nguyen wrote: > On Sun, Nov 4, 2018 at 5:45 PM Phillip Wood <phillip.wood@talktalk.net> wrote: >> >> On 04/11/2018 07:22, Nguyễn Thái Ngọc Duy wrote: >>> When a commit is reverted (or cherry-picked with -x) we add an English >>> sentence recording that commit id in the new commit message. Make >>> these real trailer lines instead so that they are more friendly to >>> parsers (especially "git interpret-trailers"). >>> >>> A reverted commit will have a new trailer >>> >>> Revert: <commit-id> >>> >>> Similarly a cherry-picked commit with -x will have >>> >>> Cherry-Pick: <commit-id> >> >> I think this is a good idea though I wonder if it will break someones >> script that is looking for the messages generated by -x at the moment. > > It will [1] but I still think it's worth the trouble. The script will > be less likely to break after, and you can use git-interpret-trailers > instead of plain grep. > > [1] https://public-inbox.org/git/20181017143921.GR270328@devbig004.ftw2.facebook.com/ > >>> @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, >>> base_label = msg.label; >>> next = parent; >>> next_label = msg.parent_label; >>> - strbuf_addstr(&msgbuf, "Revert \""); >>> - strbuf_addstr(&msgbuf, msg.subject); >>> - strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit "); >>> - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); >>> - >>> - if (commit->parents && commit->parents->next) { >>> - strbuf_addstr(&msgbuf, ", reversing\nchanges made to "); >>> - strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid)); >>> - } >> >> As revert currently records the parent given on the command line when >> reverting a merge commit it would probably be a good idea to add that >> either as a separate trailer or to the Revert: trailer and possibly also >> generate it for cherry picks. > > My mistake. I didn't read carefully and thought it was logging > commit->parents, which is pointless. > > So what should be the trailer for this (I don't think putting it in > Revert: is a good idea, too much to parse)? Revert-parent: ? > Revert-merge: ? I think Revert-parent: is good, though you seem to have gone for including it the Revert: trailer in v2. >>> - strbuf_addstr(&msgbuf, ".\n"); >>> + strbuf_addf(&msgbuf, "Revert \"%s\"\n\n", msg.subject); >> >> If the message already contains trailers then should we just append the >> Revert trailer those rather than inserting "\n\n"? > > Umm.. but this \n\n is for separating the subject and the body. I > think we need it anyway, trailer or not. Ah you're right, I had forgotten that the revert message body is empty, unlike cherry-pick where the message is copied (and that does the right thing already when there are existing trailers). Best wishes Phillip >>> @@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, >>> strbuf_complete_line(&msgbuf); >>> if (!has_conforming_footer(&msgbuf, NULL, 0)) >>> strbuf_addch(&msgbuf, '\n'); >>> - strbuf_addstr(&msgbuf, cherry_picked_prefix); >>> - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); >>> - strbuf_addstr(&msgbuf, ")\n"); >>> + strbuf_addf(&msgbuf, "Cherry-Pick: %s\n", >>> + oid_to_hex(&commit->object.oid)); > > I will probably make this "Cherry-picked-from:" to match our S-o-b style. >
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > A reverted commit will have a new trailer > > Revert: <commit-id> Please don't, unless you are keeping the current "the effect of commit X relative to its parent Y was reverted" writtein in prose, which is meant to be followed up with a manually written "because ..." and adding this as an extra footer that is meant solely for machine consumption. Of course reversion of a merge needs to say relative to which parent of the merge it is undoing. > Similarly a cherry-picked commit with -x will have > > Cherry-Pick: <commit-id> Unlike the "revert" change above, this probably is a good change, as a"(cherry-pickt-from: X)" does not try to convey anything more or anything less than such a standard looking trailer and it is in different shape only by historical accident. But people's scripts may need to have a long transition period for this change to happen. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > I think standardizing how we record commit ids in the commit message > is a good idea. Though to be honest this started because of my irk of > an English string "cherry picked from..." that cannot be translated. > It might as well be a computer language that happens to look like > English. > > Documentation/git-cherry-pick.txt | 5 ++--- > sequencer.c | 20 ++++++-------------- > t/t3510-cherry-pick-sequence.sh | 12 ++++++------ > t/t3511-cherry-pick-x.sh | 26 +++++++++++++------------- > 4 files changed, 27 insertions(+), 36 deletions(-) > > diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt > index d35d771fc8..8ffbaed1a0 100644 > --- a/Documentation/git-cherry-pick.txt > +++ b/Documentation/git-cherry-pick.txt > @@ -58,9 +58,8 @@ OPTIONS > message prior to committing. > > -x:: > - When recording the commit, append a line that says > - "(cherry picked from commit ...)" to the original commit > - message in order to indicate which commit this change was > + When recording the commit, append "Cherry-Pick:" trailer line > + recording the commit name which commit this change was > cherry-picked from. This is done only for cherry > picks without conflicts. Do not use this option if > you are cherry-picking from your private branch because > diff --git a/sequencer.c b/sequencer.c > index 9e1ab3a2a7..f7318f2862 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -36,7 +36,6 @@ > #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" > > const char sign_off_header[] = "Signed-off-by: "; > -static const char cherry_picked_prefix[] = "(cherry picked from commit "; > > GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG") > > @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, > base_label = msg.label; > next = parent; > next_label = msg.parent_label; > - strbuf_addstr(&msgbuf, "Revert \""); > - strbuf_addstr(&msgbuf, msg.subject); > - strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit "); > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); > - > - if (commit->parents && commit->parents->next) { > - strbuf_addstr(&msgbuf, ", reversing\nchanges made to "); > - strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid)); > - } > - strbuf_addstr(&msgbuf, ".\n"); > + strbuf_addf(&msgbuf, "Revert \"%s\"\n\n", msg.subject); > + > + strbuf_addf(&msgbuf, "Revert: %s\n", > + oid_to_hex(&commit->object.oid)); > } else { > const char *p; > > @@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, > strbuf_complete_line(&msgbuf); > if (!has_conforming_footer(&msgbuf, NULL, 0)) > strbuf_addch(&msgbuf, '\n'); > - strbuf_addstr(&msgbuf, cherry_picked_prefix); > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); > - strbuf_addstr(&msgbuf, ")\n"); > + strbuf_addf(&msgbuf, "Cherry-Pick: %s\n", > + oid_to_hex(&commit->object.oid)); > } > 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 c84eeefdc9..89b6e7fc1e 100755 > --- a/t/t3510-cherry-pick-sequence.sh > +++ b/t/t3510-cherry-pick-sequence.sh > @@ -390,10 +390,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-Pick:" initial_msg && > + grep "Cherry-Pick: " unrelatedpick_msg && > + grep "Cherry-Pick: " picked_msg && > + grep "Cherry-Pick: " anotherpick_msg > ' > > test_expect_success '--continue of single-pick respects -x' ' > @@ -404,7 +404,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-Pick:" msg > ' > > test_expect_success '--continue respects -x in first commit in multi-pick' ' > @@ -416,7 +416,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-Pick: $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 9888bf34b9..db11dd2430 100755 > --- a/t/t3511-cherry-pick-x.sh > +++ b/t/t3511-cherry-pick-x.sh > @@ -32,7 +32,7 @@ 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) > +Cherry-Pick: da39a3ee5e6b4b0d3255bfef95601890afd80709 > Tested-by: C.U. Thor <cuthor@example.com>" > > mesg_unclean="$mesg_one_line > @@ -81,7 +81,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-Pick: $sha1 > EOF > git log -1 --pretty=format:%B >actual && > test_cmp expect actual > @@ -129,7 +129,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-Pick: $sha1 > EOF > git log -1 --pretty=format:%B >actual && > test_cmp expect actual > @@ -154,7 +154,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-Pick: $sha1 > Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> > EOF > git log -1 --pretty=format:%B >actual && > @@ -178,7 +178,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-Pick: $sha1 > Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> > EOF > git log -1 --pretty=format:%B >actual && > @@ -201,7 +201,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-Pick: $sha1 > Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> > EOF > git log -1 --pretty=format:%B >actual && > @@ -215,7 +215,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-Pick: %s\n" $sha1 >>msg && > test_cmp msg actual > ' > > @@ -226,7 +226,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-Pick: %s\n" $sha1 >>msg && > test_cmp msg actual > ' > > @@ -252,19 +252,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-Pick:" 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-Pick: $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-Pick:" line as part of footer' ' > pristine_detach initial && > git cherry-pick -s mesg-with-cherry-footer && > cat <<-EOF >expect && > @@ -275,13 +275,13 @@ 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-Pick:..." 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-Pick: $sha1 > Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> > EOF > git log -1 --pretty=format:%B >actual &&
On Mon, Nov 5, 2018 at 1:56 AM Junio C Hamano <gitster@pobox.com> wrote: > > Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > > > A reverted commit will have a new trailer > > > > Revert: <commit-id> > > Please don't, unless you are keeping the current "the effect of > commit X relative to its parent Y was reverted" writtein in prose, > which is meant to be followed up with a manually written "because > ..." and adding this as an extra footer that is meant solely for > machine consumption. Of course reversion of a merge needs to say > relative to which parent of the merge it is undoing. I think the intent of writing "This reverts .... " to encourage writing "because ..." is good, but in practice many people just simply not do it. And by not describing anything at all (footers don't count) some commit hook can force people to actually write something. But for the transition period I think we need to keep both anyway, whether this "This reverts ..." should stay could be revisited another day (or not, even). > > Similarly a cherry-picked commit with -x will have > > > > Cherry-Pick: <commit-id> > > Unlike the "revert" change above, this probably is a good change, as > a"(cherry-pickt-from: X)" does not try to convey anything more or > anything less than such a standard looking trailer and it is in > different shape only by historical accident. But people's scripts > may need to have a long transition period for this change to happen. Yep. I'll code something up to print both by default with config knobs to disable either. Unless you have some better plan of course.
Hi, On Sun, 4 Nov 2018, Duy Nguyen wrote: > On Sun, Nov 4, 2018 at 5:45 PM Phillip Wood <phillip.wood@talktalk.net> wrote: > > > > On 04/11/2018 07:22, Nguyễn Thái Ngọc Duy wrote: > > > When a commit is reverted (or cherry-picked with -x) we add an English > > > sentence recording that commit id in the new commit message. Make > > > these real trailer lines instead so that they are more friendly to > > > parsers (especially "git interpret-trailers"). > > > > > > A reverted commit will have a new trailer > > > > > > Revert: <commit-id> > > > > > > Similarly a cherry-picked commit with -x will have > > > > > > Cherry-Pick: <commit-id> > > > > I think this is a good idea though I wonder if it will break someones > > script that is looking for the messages generated by -x at the moment. > > It will [1] but I still think it's worth the trouble. The script will > be less likely to break after, and you can use git-interpret-trailers > instead of plain grep. Since this is a wilfull backwards-incompatible patch, it needs to be done carefully. I am not enthused by the idea of breaking power users' setups, and I could imagine that a much better way to do it would be to introduce a config setting that guards the new behavior, then add a deprecation notice when -x is used without that config setting, and then let that simmer for a couple of versions. Ciao, Johannes > > [1] https://public-inbox.org/git/20181017143921.GR270328@devbig004.ftw2.facebook.com/ > > > > @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, > > > base_label = msg.label; > > > next = parent; > > > next_label = msg.parent_label; > > > - strbuf_addstr(&msgbuf, "Revert \""); > > > - strbuf_addstr(&msgbuf, msg.subject); > > > - strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit "); > > > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); > > > - > > > - if (commit->parents && commit->parents->next) { > > > - strbuf_addstr(&msgbuf, ", reversing\nchanges made to "); > > > - strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid)); > > > - } > > > > As revert currently records the parent given on the command line when > > reverting a merge commit it would probably be a good idea to add that > > either as a separate trailer or to the Revert: trailer and possibly also > > generate it for cherry picks. > > My mistake. I didn't read carefully and thought it was logging > commit->parents, which is pointless. > > So what should be the trailer for this (I don't think putting it in > Revert: is a good idea, too much to parse)? Revert-parent: ? > Revert-merge: ? > > > > - strbuf_addstr(&msgbuf, ".\n"); > > > + strbuf_addf(&msgbuf, "Revert \"%s\"\n\n", msg.subject); > > > > If the message already contains trailers then should we just append the > > Revert trailer those rather than inserting "\n\n"? > > Umm.. but this \n\n is for separating the subject and the body. I > think we need it anyway, trailer or not. > > > > @@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, > > > strbuf_complete_line(&msgbuf); > > > if (!has_conforming_footer(&msgbuf, NULL, 0)) > > > strbuf_addch(&msgbuf, '\n'); > > > - strbuf_addstr(&msgbuf, cherry_picked_prefix); > > > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); > > > - strbuf_addstr(&msgbuf, ")\n"); > > > + strbuf_addf(&msgbuf, "Cherry-Pick: %s\n", > > > + oid_to_hex(&commit->object.oid)); > > I will probably make this "Cherry-picked-from:" to match our S-o-b style. > -- > Duy >
Duy Nguyen <pclouds@gmail.com> writes: > I think the intent of writing "This reverts .... " to encourage > writing "because ..." is good, but in practice many people just simply > not do it. And by not describing anything at all (footers don't count) > some commit hook can force people to actually write something. > > But for the transition period I think we need to keep both anyway, I do not see any need to qualify the statement with "for the transition period". You showed *no* need to transition, but I do agree that adding a fixed-spelled footer in addition to what we produce in the body is a good idea. When we know a feature with good intent is not used properly by many people, the first thing to do is not talk about removing it, but to think how we can educate people to make good use of it. And if we know a feature with good intent is not used by many people but we know that "many" is not "100%", why are we talking about removing it at all? > Yep. I'll code something up to print both by default with config knobs > to disable either. Unless you have some better plan of course. Does it make sense to put both, with exactly the same piece of information? I am not sure whom it would help. The tools need to be updated to deal with both old and new format if the pick-origin information is used, instead of getting updated to learn new and forget old format, as existing commits in their history do not know about the new format and their tools need to understand them. I'd say it would be sufficient to have a per-repository knob that says which one to use, and between the release we add that knob and the release we make the new format the default, when we do not see the knob is set to either way, keep warning that in the future the default will change and give advice that the knob can be used to either keep the old format or update to the new format before or after the default switch (in addition to squelch the warning and the advice).
On Sun, Nov 04 2018, Nguyễn Thái Ngọc Duy wrote: > When a commit is reverted (or cherry-picked with -x) we add an English > sentence recording that commit id in the new commit message. Make > these real trailer lines instead so that they are more friendly to > parsers (especially "git interpret-trailers"). > > A reverted commit will have a new trailer > > Revert: <commit-id> > > Similarly a cherry-picked commit with -x will have > > Cherry-Pick: <commit-id> > [...] > I think standardizing how we record commit ids in the commit message > is a good idea. Though to be honest this started because of my irk of > an English string "cherry picked from..." that cannot be translated. > It might as well be a computer language that happens to look like > English. > [...] > @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, > base_label = msg.label; > next = parent; > next_label = msg.parent_label; > - strbuf_addstr(&msgbuf, "Revert \""); > - strbuf_addstr(&msgbuf, msg.subject); > - strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit "); > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); > - > - if (commit->parents && commit->parents->next) { > - strbuf_addstr(&msgbuf, ", reversing\nchanges made to "); > - strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid)); > - } > - strbuf_addstr(&msgbuf, ".\n"); > + strbuf_addf(&msgbuf, "Revert \"%s\"\n\n", msg.subject); > + > + strbuf_addf(&msgbuf, "Revert: %s\n", > + oid_to_hex(&commit->object.oid)); > } else { > const char *p; Others have already commented on the backwards-compatibility / switchover concerns, so I won't spend much time on that. Except to say that I don't think changing this would be a big deal. Anyone trying to parse out /This reverts commit/ or other pre-set English texts we put into the commit object is already needing to deal with users changing the message. E.g. I have a habit of doing partial reverts and changing it to "This partially reverts..." etc. Leaving aside the question of whether the pain of switching is worth it, I think it's a worthwihle to consider if we could stop hardcoding one specific human language in commit messages, and instead leave something machine-readable behind. We do that with reverts, and also with merge commits, which could be given a similar treatment where we change e.g. "Merge branches 'jc/convert', 'jc/bigfile' and 'jc/replacing' into jc/streaming" (to use git.git's 02071b27f1 as an example) to: Merge-branch-1: jc/convert Merge-branch-2: jc/bigfile Merge-branch-3: jc/replacing Merge-branch-into: jc/streaming Then, when rendering the commit in the UI we could parse that out, and put a "Merge branches[...]" message at the top, except this time in the user's own language.
On Tue, Nov 6, 2018 at 9:57 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > Leaving aside the question of whether the pain of switching is worth it, > I think it's a worthwihle to consider if we could stop hardcoding one > specific human language in commit messages, and instead leave something > machine-readable behind. > > We do that with reverts, and also with merge commits, which could be > given a similar treatment where we change e.g. "Merge branches > 'jc/convert', 'jc/bigfile' and 'jc/replacing' into jc/streaming" (to use > git.git's 02071b27f1 as an example) to: > > Merge-branch-1: jc/convert > Merge-branch-2: jc/bigfile > Merge-branch-3: jc/replacing > Merge-branch-into: jc/streaming > > Then, when rendering the commit in the UI we could parse that out, and > put a "Merge branches[...]" message at the top, except this time in the > user's own language. My first reaction of this was "but branch name is a local thing and not significant anyway!". But then if people use one branch as a bundle of other branches like 'pu', then the ability to recreate branches (with the right name of course) from those merges may be useful. So... yeah, maybe.
On Tue, Nov 06 2018, Duy Nguyen wrote: > On Tue, Nov 6, 2018 at 9:57 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: >> Leaving aside the question of whether the pain of switching is worth it, >> I think it's a worthwihle to consider if we could stop hardcoding one >> specific human language in commit messages, and instead leave something >> machine-readable behind. >> >> We do that with reverts, and also with merge commits, which could be >> given a similar treatment where we change e.g. "Merge branches >> 'jc/convert', 'jc/bigfile' and 'jc/replacing' into jc/streaming" (to use >> git.git's 02071b27f1 as an example) to: >> >> Merge-branch-1: jc/convert >> Merge-branch-2: jc/bigfile >> Merge-branch-3: jc/replacing >> Merge-branch-into: jc/streaming >> >> Then, when rendering the commit in the UI we could parse that out, and >> put a "Merge branches[...]" message at the top, except this time in the >> user's own language. > > My first reaction of this was "but branch name is a local thing and > not significant anyway!". But then if people use one branch as a > bundle of other branches like 'pu', then the ability to recreate > branches (with the right name of course) from those merges may be > useful. So... yeah, maybe. Yeah, in theory, in practice no. But all I'm observing is that we're *already* encoding this information, so to me at least the logical end game to changes like what you're proposing is something like the above, otherwise why bother? But having thought about it a bit more I wonder if git-interpret-trailers (or some command like it) shouldn't just as a special-case learn to do more parsing of commit messages we've historically generated. E.g. know how to parse out a merge message we've produced and spew out something like the above Merge-* output. Same for existing "Revert" output, if anyone cares about parsing this they'll need to do that anyway.
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt index d35d771fc8..8ffbaed1a0 100644 --- a/Documentation/git-cherry-pick.txt +++ b/Documentation/git-cherry-pick.txt @@ -58,9 +58,8 @@ OPTIONS message prior to committing. -x:: - When recording the commit, append a line that says - "(cherry picked from commit ...)" to the original commit - message in order to indicate which commit this change was + When recording the commit, append "Cherry-Pick:" trailer line + recording the commit name which commit this change was cherry-picked from. This is done only for cherry picks without conflicts. Do not use this option if you are cherry-picking from your private branch because diff --git a/sequencer.c b/sequencer.c index 9e1ab3a2a7..f7318f2862 100644 --- a/sequencer.c +++ b/sequencer.c @@ -36,7 +36,6 @@ #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" const char sign_off_header[] = "Signed-off-by: "; -static const char cherry_picked_prefix[] = "(cherry picked from commit "; GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG") @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, base_label = msg.label; next = parent; next_label = msg.parent_label; - strbuf_addstr(&msgbuf, "Revert \""); - strbuf_addstr(&msgbuf, msg.subject); - strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit "); - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); - - if (commit->parents && commit->parents->next) { - strbuf_addstr(&msgbuf, ", reversing\nchanges made to "); - strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid)); - } - strbuf_addstr(&msgbuf, ".\n"); + strbuf_addf(&msgbuf, "Revert \"%s\"\n\n", msg.subject); + + strbuf_addf(&msgbuf, "Revert: %s\n", + oid_to_hex(&commit->object.oid)); } else { const char *p; @@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, strbuf_complete_line(&msgbuf); if (!has_conforming_footer(&msgbuf, NULL, 0)) strbuf_addch(&msgbuf, '\n'); - strbuf_addstr(&msgbuf, cherry_picked_prefix); - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); - strbuf_addstr(&msgbuf, ")\n"); + strbuf_addf(&msgbuf, "Cherry-Pick: %s\n", + oid_to_hex(&commit->object.oid)); } 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 c84eeefdc9..89b6e7fc1e 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -390,10 +390,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-Pick:" initial_msg && + grep "Cherry-Pick: " unrelatedpick_msg && + grep "Cherry-Pick: " picked_msg && + grep "Cherry-Pick: " anotherpick_msg ' test_expect_success '--continue of single-pick respects -x' ' @@ -404,7 +404,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-Pick:" msg ' test_expect_success '--continue respects -x in first commit in multi-pick' ' @@ -416,7 +416,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-Pick: $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 9888bf34b9..db11dd2430 100755 --- a/t/t3511-cherry-pick-x.sh +++ b/t/t3511-cherry-pick-x.sh @@ -32,7 +32,7 @@ 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) +Cherry-Pick: da39a3ee5e6b4b0d3255bfef95601890afd80709 Tested-by: C.U. Thor <cuthor@example.com>" mesg_unclean="$mesg_one_line @@ -81,7 +81,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-Pick: $sha1 EOF git log -1 --pretty=format:%B >actual && test_cmp expect actual @@ -129,7 +129,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-Pick: $sha1 EOF git log -1 --pretty=format:%B >actual && test_cmp expect actual @@ -154,7 +154,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-Pick: $sha1 Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> EOF git log -1 --pretty=format:%B >actual && @@ -178,7 +178,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-Pick: $sha1 Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> EOF git log -1 --pretty=format:%B >actual && @@ -201,7 +201,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-Pick: $sha1 Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> EOF git log -1 --pretty=format:%B >actual && @@ -215,7 +215,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-Pick: %s\n" $sha1 >>msg && test_cmp msg actual ' @@ -226,7 +226,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-Pick: %s\n" $sha1 >>msg && test_cmp msg actual ' @@ -252,19 +252,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-Pick:" 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-Pick: $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-Pick:" line as part of footer' ' pristine_detach initial && git cherry-pick -s mesg-with-cherry-footer && cat <<-EOF >expect && @@ -275,13 +275,13 @@ 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-Pick:..." 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-Pick: $sha1 Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> EOF git log -1 --pretty=format:%B >actual &&
When a commit is reverted (or cherry-picked with -x) we add an English sentence recording that commit id in the new commit message. Make these real trailer lines instead so that they are more friendly to parsers (especially "git interpret-trailers"). A reverted commit will have a new trailer Revert: <commit-id> Similarly a cherry-picked commit with -x will have Cherry-Pick: <commit-id> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- I think standardizing how we record commit ids in the commit message is a good idea. Though to be honest this started because of my irk of an English string "cherry picked from..." that cannot be translated. It might as well be a computer language that happens to look like English. Documentation/git-cherry-pick.txt | 5 ++--- sequencer.c | 20 ++++++-------------- t/t3510-cherry-pick-sequence.sh | 12 ++++++------ t/t3511-cherry-pick-x.sh | 26 +++++++++++++------------- 4 files changed, 27 insertions(+), 36 deletions(-)