From: Phillip Wood <phillip.wood@talktalk.net>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines
Date: Sun, 4 Nov 2018 16:45:00 +0000 [thread overview]
Message-ID: <27bcf7a6-8590-fa21-8381-697e1b030182@talktalk.net> (raw)
In-Reply-To: <20181104072253.12357-1-pclouds@gmail.com>
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 &&
>
next prev parent reply other threads:[~2018-11-04 16:45 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-04 7:22 [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines Nguyễn Thái Ngọc Duy
2018-11-04 16:45 ` Phillip Wood [this message]
2018-11-04 17:41 ` Duy Nguyen
2018-11-04 21:12 ` Phillip Wood
2018-11-05 21:57 ` Johannes Schindelin
2018-11-04 18:10 ` [PATCH/RFC v2] " Nguyễn Thái Ngọc Duy
2018-11-04 21:30 ` brian m. carlson
2018-11-05 16:08 ` Duy Nguyen
2018-11-06 17:16 ` [PATCH/RFC] Support --append-trailer in cherry-pick and revert Nguyễn Thái Ngọc Duy
2018-11-06 17:48 ` Ævar Arnfjörð Bjarmason
2018-11-06 22:11 ` Jeff King
2018-11-06 22:29 ` Jeff King
2018-11-07 0:42 ` Junio C Hamano
2018-11-07 15:30 ` Duy Nguyen
2018-11-07 21:02 ` Jeff King
2018-11-08 0:36 ` Junio C Hamano
2018-11-08 1:29 ` Jeff King
2018-11-08 3:34 ` Junio C Hamano
2018-11-07 0:31 ` Junio C Hamano
2018-11-05 0:56 ` [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines Junio C Hamano
2018-11-05 16:17 ` Duy Nguyen
2018-11-06 1:44 ` Junio C Hamano
2018-11-06 8:57 ` Ævar Arnfjörð Bjarmason
2018-11-06 16:13 ` Duy Nguyen
2018-11-06 17:44 ` Ævar Arnfjörð Bjarmason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=27bcf7a6-8590-fa21-8381-697e1b030182@talktalk.net \
--to=phillip.wood@talktalk.net \
--cc=git@vger.kernel.org \
--cc=pclouds@gmail.com \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).