{"thread":{"id":"49753","subject":"[PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines","startedAt":"2018-11-04T07:24:43Z","lastAt":"2018-11-08T03:34:46Z","messageCount":25,"participants":["Nguyễn Thái Ngọc Duy","Phillip Wood","Duy Nguyen","brian m. carlson","Junio C Hamano","Johannes Schindelin","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"362389","messageId":"20181104072253.12357-1-pclouds@gmail.com","threadId":"49753","inReplyTo":null,"subject":"[PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-04T07:22:53Z","receivedAt":"2018-11-04T07:24:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When a commit is reverted (or cherry-picked with -x) we add an English\nsentence recording that commit id in the new commit message. Make\nthese real trailer lines instead so that they are more friendly to\nparsers (especially \"git interpret-trailers\").\n\nA reverted commit will have a new trailer\n\n Revert: \n\nSimilarly a cherry-picked commit with -x will have\n\n Cherry-Pick: \n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n I think standardizing how we record commit ids in the commit message\n is a good idea. Though to be honest this started because of my irk of\n an English string \"cherry picked from...\" that cannot be translated.\n It might as well be a computer language that happens to look like\n English.\n\n Documentation/git-cherry-pick.txt | 5 ++---\n sequencer.c | 20 ++++++--------------\n t/t3510-cherry-pick-sequence.sh | 12 ++++++------\n t/t3511-cherry-pick-x.sh | 26 +++++++++++++-------------\n 4 files changed, 27 insertions(+), 36 deletions(-)\n\ndiff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\nindex d35d771fc8..8ffbaed1a0 100644\n--- a/Documentation/git-cherry-pick.txt\n+++ b/Documentation/git-cherry-pick.txt\n@@ -58,9 +58,8 @@ OPTIONS\n \tmessage prior to committing.\n \n -x::\n-\tWhen recording the commit, append a line that says\n-\t\"(cherry picked from commit ...)\" to the original commit\n-\tmessage in order to indicate which commit this change was\n+\tWhen recording the commit, append \"Cherry-Pick:\" trailer line\n+\trecording the commit name which commit this change was\n \tcherry-picked from. This is done only for cherry\n \tpicks without conflicts. Do not use this option if\n \tyou are cherry-picking from your private branch because\ndiff --git a/sequencer.c b/sequencer.c\nindex 9e1ab3a2a7..f7318f2862 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -36,7 +36,6 @@\n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n \n const char sign_off_header[] = \"Signed-off-by: \";\n-static const char cherry_picked_prefix[] = \"(cherry picked from commit \";\n \n GIT_PATH_FUNC(git_path_commit_editmsg, \"COMMIT_EDITMSG\")\n \n@@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\tbase_label = msg.label;\n \t\tnext = parent;\n \t\tnext_label = msg.parent_label;\n-\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n-\t\tstrbuf_addstr(&msgbuf, msg.subject);\n-\t\tstrbuf_addstr(&msgbuf, \"\\\"\\n\\nThis reverts commit \");\n-\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n-\n-\t\tif (commit->parents && commit->parents->next) {\n-\t\t\tstrbuf_addstr(&msgbuf, \", reversing\\nchanges made to \");\n-\t\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid));\n-\t\t}\n-\t\tstrbuf_addstr(&msgbuf, \".\\n\");\n+\t\tstrbuf_addf(&msgbuf, \"Revert \\\"%s\\\"\\n\\n\", msg.subject);\n+\n+\t\tstrbuf_addf(&msgbuf, \"Revert: %s\\n\",\n+\t\t\t oid_to_hex(&commit->object.oid));\n \t} else {\n \t\tconst char *p;\n \n@@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\tstrbuf_complete_line(&msgbuf);\n \t\t\tif (!has_conforming_footer(&msgbuf, NULL, 0))\n \t\t\t\tstrbuf_addch(&msgbuf, '\\n');\n-\t\t\tstrbuf_addstr(&msgbuf, cherry_picked_prefix);\n-\t\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n-\t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n+\t\t\tstrbuf_addf(&msgbuf, \"Cherry-Pick: %s\\n\",\n+\t\t\t\t oid_to_hex(&commit->object.oid));\n \t\t}\n \t\tif (!is_fixup(command))\n \t\t\tauthor = get_author(msg.message);\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex c84eeefdc9..89b6e7fc1e 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -390,10 +390,10 @@ test_expect_success '--continue respects opts' '\n \tgit cat-file commit HEAD~1 >picked_msg &&\n \tgit cat-file commit HEAD~2 >unrelatedpick_msg &&\n \tgit cat-file commit HEAD~3 >initial_msg &&\n-\t! grep \"cherry picked from\" initial_msg &&\n-\tgrep \"cherry picked from\" unrelatedpick_msg &&\n-\tgrep \"cherry picked from\" picked_msg &&\n-\tgrep \"cherry picked from\" anotherpick_msg\n+\t! grep \"Cherry-Pick:\" initial_msg &&\n+\tgrep \"Cherry-Pick: \" unrelatedpick_msg &&\n+\tgrep \"Cherry-Pick: \" picked_msg &&\n+\tgrep \"Cherry-Pick: \" anotherpick_msg\n '\n \n test_expect_success '--continue of single-pick respects -x' '\n@@ -404,7 +404,7 @@ test_expect_success '--continue of single-pick respects -x' '\n \tgit cherry-pick --continue &&\n \ttest_path_is_missing .git/sequencer &&\n \tgit cat-file commit HEAD >msg &&\n-\tgrep \"cherry picked from\" msg\n+\tgrep \"Cherry-Pick:\" msg\n '\n \n test_expect_success '--continue respects -x in first commit in multi-pick' '\n@@ -416,7 +416,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '\n \ttest_path_is_missing .git/sequencer &&\n \tgit cat-file commit HEAD^ >msg &&\n \tpicked=$(git rev-parse --verify picked) &&\n-\tgrep \"cherry picked from.*$picked\" msg\n+\tgrep \"Cherry-Pick: $picked\" msg\n '\n \n test_expect_failure '--signoff is automatically propagated to resolved conflict' '\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex 9888bf34b9..db11dd2430 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -32,7 +32,7 @@ mesg_with_footer_sob=\"$mesg_with_footer\n Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n \n mesg_with_cherry_footer=\"$mesg_with_footer_sob\n-(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)\n+Cherry-Pick: da39a3ee5e6b4b0d3255bfef95601890afd80709\n Tested-by: C.U. Thor \"\n \n mesg_unclean=\"$mesg_one_line\n@@ -81,7 +81,7 @@ test_expect_success 'cherry-pick -x inserts blank line after one line subject' '\n \tcat <<-EOF >expect &&\n \t\t$mesg_one_line\n \n-\t\t(cherry picked from commit $sha1)\n+\t\tCherry-Pick: $sha1\n \tEOF\n \tgit log -1 --pretty=format:%B >actual &&\n \ttest_cmp expect actual\n@@ -129,7 +129,7 @@ test_expect_success 'cherry-pick -x inserts blank line when conforming footer no\n \tcat <<-EOF >expect &&\n \t\t$mesg_no_footer\n \n-\t\t(cherry picked from commit $sha1)\n+\t\tCherry-Pick: $sha1\n \tEOF\n \tgit log -1 --pretty=format:%B >actual &&\n \ttest_cmp expect actual\n@@ -154,7 +154,7 @@ test_expect_success 'cherry-pick -x -s inserts blank line when conforming footer\n \tcat <<-EOF >expect &&\n \t\t$mesg_no_footer\n \n-\t\t(cherry picked from commit $sha1)\n+\t\tCherry-Pick: $sha1\n \t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n \tEOF\n \tgit log -1 --pretty=format:%B >actual &&\n@@ -178,7 +178,7 @@ test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match commi\n \tgit cherry-pick -x -s mesg-with-footer &&\n \tcat <<-EOF >expect &&\n \t\t$mesg_with_footer\n-\t\t(cherry picked from commit $sha1)\n+\t\tCherry-Pick: $sha1\n \t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n \tEOF\n \tgit log -1 --pretty=format:%B >actual &&\n@@ -201,7 +201,7 @@ test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo\n \tgit cherry-pick -x -s mesg-with-footer-sob &&\n \tcat <<-EOF >expect &&\n \t\t$mesg_with_footer_sob\n-\t\t(cherry picked from commit $sha1)\n+\t\tCherry-Pick: $sha1\n \t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n \tEOF\n \tgit log -1 --pretty=format:%B >actual &&\n@@ -215,7 +215,7 @@ test_expect_success 'cherry-pick -x handles commits with no NL at end of message\n \tgit cherry-pick -x $sha1 &&\n \tgit log -1 --pretty=format:%B >actual &&\n \n-\tprintf \"\\n(cherry picked from commit %s)\\n\" $sha1 >>msg &&\n+\tprintf \"\\nCherry-Pick: %s\\n\" $sha1 >>msg &&\n \ttest_cmp msg actual\n '\n \n@@ -226,7 +226,7 @@ test_expect_success 'cherry-pick -x handles commits with no footer and no NL at\n \tgit cherry-pick -x $sha1 &&\n \tgit log -1 --pretty=format:%B >actual &&\n \n-\tprintf \"\\n\\n(cherry picked from commit %s)\\n\" $sha1 >>msg &&\n+\tprintf \"\\n\\nCherry-Pick: %s\\n\" $sha1 >>msg &&\n \ttest_cmp msg actual\n '\n \n@@ -252,19 +252,19 @@ test_expect_success 'cherry-pick -s handles commits with no footer and no NL at\n \ttest_cmp msg actual\n '\n \n-test_expect_success 'cherry-pick -x treats \"(cherry picked from...\" line as part of footer' '\n+test_expect_success 'cherry-pick -x treats \"Cherry-Pick:\" line as part of footer' '\n \tpristine_detach initial &&\n \tsha1=$(git rev-parse mesg-with-cherry-footer^0) &&\n \tgit cherry-pick -x mesg-with-cherry-footer &&\n \tcat <<-EOF >expect &&\n \t\t$mesg_with_cherry_footer\n-\t\t(cherry picked from commit $sha1)\n+\t\tCherry-Pick: $sha1\n \tEOF\n \tgit log -1 --pretty=format:%B >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'cherry-pick -s treats \"(cherry picked from...\" line as part of footer' '\n+test_expect_success 'cherry-pick -s treats \"Cherry-Pick:\" line as part of footer' '\n \tpristine_detach initial &&\n \tgit cherry-pick -s mesg-with-cherry-footer &&\n \tcat <<-EOF >expect &&\n@@ -275,13 +275,13 @@ test_expect_success 'cherry-pick -s treats \"(cherry picked from...\" line as part\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'cherry-pick -x -s treats \"(cherry picked from...\" line as part of footer' '\n+test_expect_success 'cherry-pick -x -s treats \"Cherry-Pick:...\" line as part of footer' '\n \tpristine_detach initial &&\n \tsha1=$(git rev-parse mesg-with-cherry-footer^0) &&\n \tgit cherry-pick -x -s mesg-with-cherry-footer &&\n \tcat <<-EOF >expect &&\n \t\t$mesg_with_cherry_footer\n-\t\t(cherry picked from commit $sha1)\n+\t\tCherry-Pick: $sha1\n \t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n \tEOF\n \tgit log -1 --pretty=format:%B >actual &&\n-- \n2.19.1.1005.gac84295441\n\n"},{"id":"362403","messageId":"27bcf7a6-8590-fa21-8381-697e1b030182@talktalk.net","threadId":"49753","inReplyTo":"20181104072253.12357-1-pclouds@gmail.com","subject":"Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-04T16:45:00Z","receivedAt":"2018-11-04T16:45:07Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 04/11/2018 07:22, Nguyễn Thái Ngọc Duy wrote:\n> When a commit is reverted (or cherry-picked with -x) we add an English\n> sentence recording that commit id in the new commit message. Make\n> these real trailer lines instead so that they are more friendly to\n> parsers (especially \"git interpret-trailers\").\n> \n> A reverted commit will have a new trailer\n> \n> Revert: \n> \n> Similarly a cherry-picked commit with -x will have\n> \n> Cherry-Pick: \n\nI think this is a good idea though I wonder if it will break someones \nscript that is looking for the messages generated by -x at the moment. \nI've got a couple of comments below.\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> I think standardizing how we record commit ids in the commit message\n> is a good idea. Though to be honest this started because of my irk of\n> an English string \"cherry picked from...\" that cannot be translated.\n> It might as well be a computer language that happens to look like\n> English.\n> \n> Documentation/git-cherry-pick.txt | 5 ++---\n> sequencer.c | 20 ++++++--------------\n> t/t3510-cherry-pick-sequence.sh | 12 ++++++------\n> t/t3511-cherry-pick-x.sh | 26 +++++++++++++-------------\n> 4 files changed, 27 insertions(+), 36 deletions(-)\n> \n> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\n> index d35d771fc8..8ffbaed1a0 100644\n> --- a/Documentation/git-cherry-pick.txt\n> +++ b/Documentation/git-cherry-pick.txt\n> @@ -58,9 +58,8 @@ OPTIONS\n> \tmessage prior to committing.\n> \n> -x::\n> -\tWhen recording the commit, append a line that says\n> -\t\"(cherry picked from commit ...)\" to the original commit\n> -\tmessage in order to indicate which commit this change was\n> +\tWhen recording the commit, append \"Cherry-Pick:\" trailer line\n> +\trecording the commit name which commit this change was\n> \tcherry-picked from. This is done only for cherry\n> \tpicks without conflicts. Do not use this option if\n> \tyou are cherry-picking from your private branch because\n> diff --git a/sequencer.c b/sequencer.c\n> index 9e1ab3a2a7..f7318f2862 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -36,7 +36,6 @@\n> #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n> \n> const char sign_off_header[] = \"Signed-off-by: \";\n> -static const char cherry_picked_prefix[] = \"(cherry picked from commit \";\n> \n> GIT_PATH_FUNC(git_path_commit_editmsg, \"COMMIT_EDITMSG\")\n> \n> @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n> \t\tbase_label = msg.label;\n> \t\tnext = parent;\n> \t\tnext_label = msg.parent_label;\n> -\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n> -\t\tstrbuf_addstr(&msgbuf, msg.subject);\n> -\t\tstrbuf_addstr(&msgbuf, \"\\\"\\n\\nThis reverts commit \");\n> -\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n> -\n> -\t\tif (commit->parents && commit->parents->next) {\n> -\t\t\tstrbuf_addstr(&msgbuf, \", reversing\\nchanges made to \");\n> -\t\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid));\n> -\t\t}\n\nAs revert currently records the parent given on the command line when \nreverting a merge commit it would probably be a good idea to add that \neither as a separate trailer or to the Revert: trailer and possibly also \ngenerate it for cherry picks.\n\n> -\t\tstrbuf_addstr(&msgbuf, \".\\n\");\n> +\t\tstrbuf_addf(&msgbuf, \"Revert \\\"%s\\\"\\n\\n\", msg.subject);\n\nIf the message already contains trailers then should we just append the \nRevert trailer those rather than inserting \"\\n\\n\"?\n\nBest Wishes\n\nPhillip\n\n> +\n> +\t\tstrbuf_addf(&msgbuf, \"Revert: %s\\n\",\n> +\t\t\t oid_to_hex(&commit->object.oid));\n\n> \t} else {\n> \t\tconst char *p;\n> \n> @@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n> \t\t\tstrbuf_complete_line(&msgbuf);\n> \t\t\tif (!has_conforming_footer(&msgbuf, NULL, 0))\n> \t\t\t\tstrbuf_addch(&msgbuf, '\\n');\n> -\t\t\tstrbuf_addstr(&msgbuf, cherry_picked_prefix);\n> -\t\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n> -\t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n> +\t\t\tstrbuf_addf(&msgbuf, \"Cherry-Pick: %s\\n\",\n> +\t\t\t\t oid_to_hex(&commit->object.oid));\n> \t\t}\n> \t\tif (!is_fixup(command))\n> \t\t\tauthor = get_author(msg.message);\n> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\n> index c84eeefdc9..89b6e7fc1e 100755\n> --- a/t/t3510-cherry-pick-sequence.sh\n> +++ b/t/t3510-cherry-pick-sequence.sh\n> @@ -390,10 +390,10 @@ test_expect_success '--continue respects opts' '\n> \tgit cat-file commit HEAD~1 >picked_msg &&\n> \tgit cat-file commit HEAD~2 >unrelatedpick_msg &&\n> \tgit cat-file commit HEAD~3 >initial_msg &&\n> -\t! grep \"cherry picked from\" initial_msg &&\n> -\tgrep \"cherry picked from\" unrelatedpick_msg &&\n> -\tgrep \"cherry picked from\" picked_msg &&\n> -\tgrep \"cherry picked from\" anotherpick_msg\n> +\t! grep \"Cherry-Pick:\" initial_msg &&\n> +\tgrep \"Cherry-Pick: \" unrelatedpick_msg &&\n> +\tgrep \"Cherry-Pick: \" picked_msg &&\n> +\tgrep \"Cherry-Pick: \" anotherpick_msg\n> '\n> \n> test_expect_success '--continue of single-pick respects -x' '\n> @@ -404,7 +404,7 @@ test_expect_success '--continue of single-pick respects -x' '\n> \tgit cherry-pick --continue &&\n> \ttest_path_is_missing .git/sequencer &&\n> \tgit cat-file commit HEAD >msg &&\n> -\tgrep \"cherry picked from\" msg\n> +\tgrep \"Cherry-Pick:\" msg\n> '\n> \n> test_expect_success '--continue respects -x in first commit in multi-pick' '\n> @@ -416,7 +416,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '\n> \ttest_path_is_missing .git/sequencer &&\n> \tgit cat-file commit HEAD^ >msg &&\n> \tpicked=$(git rev-parse --verify picked) &&\n> -\tgrep \"cherry picked from.*$picked\" msg\n> +\tgrep \"Cherry-Pick: $picked\" msg\n> '\n> \n> test_expect_failure '--signoff is automatically propagated to resolved conflict' '\n> diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\n> index 9888bf34b9..db11dd2430 100755\n> --- a/t/t3511-cherry-pick-x.sh\n> +++ b/t/t3511-cherry-pick-x.sh\n> @@ -32,7 +32,7 @@ mesg_with_footer_sob=\"$mesg_with_footer\n> Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n> \n> mesg_with_cherry_footer=\"$mesg_with_footer_sob\n> -(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)\n> +Cherry-Pick: da39a3ee5e6b4b0d3255bfef95601890afd80709\n> Tested-by: C.U. Thor \"\n> \n> mesg_unclean=\"$mesg_one_line\n> @@ -81,7 +81,7 @@ test_expect_success 'cherry-pick -x inserts blank line after one line subject' '\n> \tcat <<-EOF >expect &&\n> \t\t$mesg_one_line\n> \n> -\t\t(cherry picked from commit $sha1)\n> +\t\tCherry-Pick: $sha1\n> \tEOF\n> \tgit log -1 --pretty=format:%B >actual &&\n> \ttest_cmp expect actual\n> @@ -129,7 +129,7 @@ test_expect_success 'cherry-pick -x inserts blank line when conforming footer no\n> \tcat <<-EOF >expect &&\n> \t\t$mesg_no_footer\n> \n> -\t\t(cherry picked from commit $sha1)\n> +\t\tCherry-Pick: $sha1\n> \tEOF\n> \tgit log -1 --pretty=format:%B >actual &&\n> \ttest_cmp expect actual\n> @@ -154,7 +154,7 @@ test_expect_success 'cherry-pick -x -s inserts blank line when conforming footer\n> \tcat <<-EOF >expect &&\n> \t\t$mesg_no_footer\n> \n> -\t\t(cherry picked from commit $sha1)\n> +\t\tCherry-Pick: $sha1\n> \t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n> \tEOF\n> \tgit log -1 --pretty=format:%B >actual &&\n> @@ -178,7 +178,7 @@ test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match commi\n> \tgit cherry-pick -x -s mesg-with-footer &&\n> \tcat <<-EOF >expect &&\n> \t\t$mesg_with_footer\n> -\t\t(cherry picked from commit $sha1)\n> +\t\tCherry-Pick: $sha1\n> \t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n> \tEOF\n> \tgit log -1 --pretty=format:%B >actual &&\n> @@ -201,7 +201,7 @@ test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo\n> \tgit cherry-pick -x -s mesg-with-footer-sob &&\n> \tcat <<-EOF >expect &&\n> \t\t$mesg_with_footer_sob\n> -\t\t(cherry picked from commit $sha1)\n> +\t\tCherry-Pick: $sha1\n> \t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n> \tEOF\n> \tgit log -1 --pretty=format:%B >actual &&\n> @@ -215,7 +215,7 @@ test_expect_success 'cherry-pick -x handles commits with no NL at end of message\n> \tgit cherry-pick -x $sha1 &&\n> \tgit log -1 --pretty=format:%B >actual &&\n> \n> -\tprintf \"\\n(cherry picked from commit %s)\\n\" $sha1 >>msg &&\n> +\tprintf \"\\nCherry-Pick: %s\\n\" $sha1 >>msg &&\n> \ttest_cmp msg actual\n> '\n> \n> @@ -226,7 +226,7 @@ test_expect_success 'cherry-pick -x handles commits with no footer and no NL at\n> \tgit cherry-pick -x $sha1 &&\n> \tgit log -1 --pretty=format:%B >actual &&\n> \n> -\tprintf \"\\n\\n(cherry picked from commit %s)\\n\" $sha1 >>msg &&\n> +\tprintf \"\\n\\nCherry-Pick: %s\\n\" $sha1 >>msg &&\n> \ttest_cmp msg actual\n> '\n> \n> @@ -252,19 +252,19 @@ test_expect_success 'cherry-pick -s handles commits with no footer and no NL at\n> \ttest_cmp msg actual\n> '\n> \n> -test_expect_success 'cherry-pick -x treats \"(cherry picked from...\" line as part of footer' '\n> +test_expect_success 'cherry-pick -x treats \"Cherry-Pick:\" line as part of footer' '\n> \tpristine_detach initial &&\n> \tsha1=$(git rev-parse mesg-with-cherry-footer^0) &&\n> \tgit cherry-pick -x mesg-with-cherry-footer &&\n> \tcat <<-EOF >expect &&\n> \t\t$mesg_with_cherry_footer\n> -\t\t(cherry picked from commit $sha1)\n> +\t\tCherry-Pick: $sha1\n> \tEOF\n> \tgit log -1 --pretty=format:%B >actual &&\n> \ttest_cmp expect actual\n> '\n> \n> -test_expect_success 'cherry-pick -s treats \"(cherry picked from...\" line as part of footer' '\n> +test_expect_success 'cherry-pick -s treats \"Cherry-Pick:\" line as part of footer' '\n> \tpristine_detach initial &&\n> \tgit cherry-pick -s mesg-with-cherry-footer &&\n> \tcat <<-EOF >expect &&\n> @@ -275,13 +275,13 @@ test_expect_success 'cherry-pick -s treats \"(cherry picked from...\" line as part\n> \ttest_cmp expect actual\n> '\n> \n> -test_expect_success 'cherry-pick -x -s treats \"(cherry picked from...\" line as part of footer' '\n> +test_expect_success 'cherry-pick -x -s treats \"Cherry-Pick:...\" line as part of footer' '\n> \tpristine_detach initial &&\n> \tsha1=$(git rev-parse mesg-with-cherry-footer^0) &&\n> \tgit cherry-pick -x -s mesg-with-cherry-footer &&\n> \tcat <<-EOF >expect &&\n> \t\t$mesg_with_cherry_footer\n> -\t\t(cherry picked from commit $sha1)\n> +\t\tCherry-Pick: $sha1\n> \t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n> \tEOF\n> \tgit log -1 --pretty=format:%B >actual &&\n> \n\n"},{"id":"362406","messageId":"CACsJy8CAw6yR5vbsFJbSd+UTR_1UpS=8PP3hEtbSNSi3SkMNag@mail.gmail.com","threadId":"49753","inReplyTo":"27bcf7a6-8590-fa21-8381-697e1b030182@talktalk.net","subject":"Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-04T17:41:38Z","receivedAt":"2018-11-04T17:42:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Nov 4, 2018 at 5:45 PM Phillip Wood wrote:\n>\n> On 04/11/2018 07:22, Nguyễn Thái Ngọc Duy wrote:\n> > When a commit is reverted (or cherry-picked with -x) we add an English\n> > sentence recording that commit id in the new commit message. Make\n> > these real trailer lines instead so that they are more friendly to\n> > parsers (especially \"git interpret-trailers\").\n> >\n> > A reverted commit will have a new trailer\n> >\n> > Revert: \n> >\n> > Similarly a cherry-picked commit with -x will have\n> >\n> > Cherry-Pick: \n>\n> I think this is a good idea though I wonder if it will break someones\n> script that is looking for the messages generated by -x at the moment.\n\nIt will [1] but I still think it's worth the trouble. The script will\nbe less likely to break after, and you can use git-interpret-trailers\ninstead of plain grep.\n\n[1] https://public-inbox.org/git/20181017143921.GR270328@devbig004.ftw2.facebook.com/\n\n> > @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n> > base_label = msg.label;\n> > next = parent;\n> > next_label = msg.parent_label;\n> > - strbuf_addstr(&msgbuf, \"Revert \\\"\");\n> > - strbuf_addstr(&msgbuf, msg.subject);\n> > - strbuf_addstr(&msgbuf, \"\\\"\\n\\nThis reverts commit \");\n> > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n> > -\n> > - if (commit->parents && commit->parents->next) {\n> > - strbuf_addstr(&msgbuf, \", reversing\\nchanges made to \");\n> > - strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid));\n> > - }\n>\n> As revert currently records the parent given on the command line when\n> reverting a merge commit it would probably be a good idea to add that\n> either as a separate trailer or to the Revert: trailer and possibly also\n> generate it for cherry picks.\n\nMy mistake. I didn't read carefully and thought it was logging\ncommit->parents, which is pointless.\n\nSo what should be the trailer for this (I don't think putting it in\nRevert: is a good idea, too much to parse)? Revert-parent: ?\nRevert-merge: ?\n\n> > - strbuf_addstr(&msgbuf, \".\\n\");\n> > + strbuf_addf(&msgbuf, \"Revert \\\"%s\\\"\\n\\n\", msg.subject);\n>\n> If the message already contains trailers then should we just append the\n> Revert trailer those rather than inserting \"\\n\\n\"?\n\nUmm.. but this \\n\\n is for separating the subject and the body. I\nthink we need it anyway, trailer or not.\n\n> > @@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n> > strbuf_complete_line(&msgbuf);\n> > if (!has_conforming_footer(&msgbuf, NULL, 0))\n> > strbuf_addch(&msgbuf, '\\n');\n> > - strbuf_addstr(&msgbuf, cherry_picked_prefix);\n> > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n> > - strbuf_addstr(&msgbuf, \")\\n\");\n> > + strbuf_addf(&msgbuf, \"Cherry-Pick: %s\\n\",\n> > + oid_to_hex(&commit->object.oid));\n\nI will probably make this \"Cherry-picked-from:\" to match our S-o-b style.\n-- \nDuy\n"},{"id":"362408","messageId":"20181104181026.8451-1-pclouds@gmail.com","threadId":"49753","inReplyTo":"20181104072253.12357-1-pclouds@gmail.com","subject":"[PATCH/RFC v2] sequencer.c: record revert/cherry-pick commit with trailer lines","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-04T18:10:26Z","receivedAt":"2018-11-04T18:10:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When a commit is reverted (or cherry-picked with -x) we add an English\nsentence recording that commit id in the new commit message. Make\nthese real trailer lines instead so that they are more friendly to\nparsers (especially \"git interpret-trailers\").\n\nA reverted commit will have a new trailer\n\n Revert: \n\nSimilarly a cherry-picked commit with -x will have\n\n Cherry-picked-from: \n\nWhen reverting or cherry picking a merge, the reverted/cherry-picked\nbranch will be shown using extended SHA-1 syntax, e.g.\n\n Revert: ~2\n\nSince we're not producing the old lines \"This reverts commit ...\" and\n\"(cherry picked from commit ..\" anymore, scripts that look for these\nlines will need to be updated to handle both. Fresh new history could\njust rely on git-interpret-trailers instead.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n v2 adds the third syntax \"~2\" and renames Cherry-Pick: to\n Cherry-picked-from: and acknowledge the problem with breaking\n scripts. I don't have a pretty solution for that though.\n\n Documentation/git-cherry-pick.txt | 5 ++---\n sequencer.c | 33 +++++++++++++++++--------------\n t/t3510-cherry-pick-sequence.sh | 12 +++++------\n t/t3511-cherry-pick-x.sh | 26 ++++++++++++------------\n 4 files changed, 39 insertions(+), 37 deletions(-)\n\ndiff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\nindex d35d771fc8..54e73e74c0 100644\n--- a/Documentation/git-cherry-pick.txt\n+++ b/Documentation/git-cherry-pick.txt\n@@ -58,9 +58,8 @@ OPTIONS\n \tmessage prior to committing.\n \n -x::\n-\tWhen recording the commit, append a line that says\n-\t\"(cherry picked from commit ...)\" to the original commit\n-\tmessage in order to indicate which commit this change was\n+\tWhen recording the commit, append \"Cherry-picked-from:\" trailer line\n+\trecording the commit name which commit this change was\n \tcherry-picked from. This is done only for cherry\n \tpicks without conflicts. Do not use this option if\n \tyou are cherry-picking from your private branch because\ndiff --git a/sequencer.c b/sequencer.c\nindex 9e1ab3a2a7..f804406b68 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -36,7 +36,6 @@\n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n \n const char sign_off_header[] = \"Signed-off-by: \";\n-static const char cherry_picked_prefix[] = \"(cherry picked from commit \";\n \n GIT_PATH_FUNC(git_path_commit_editmsg, \"COMMIT_EDITMSG\")\n \n@@ -1669,7 +1668,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \tchar *author = NULL;\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n \tstruct strbuf msgbuf = STRBUF_INIT;\n-\tint res, unborn = 0, allow;\n+\tint res, unborn = 0, allow, parent_id = -1;\n \n \tif (opts->no_commit) {\n \t\t/*\n@@ -1716,6 +1715,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\treturn error(_(\"commit %s does not have parent %d\"),\n \t\t\t\toid_to_hex(&commit->object.oid), opts->mainline);\n \t\tparent = p->item;\n+\t\tparent_id = cnt;\n \t} else if (0 < opts->mainline)\n \t\treturn error(_(\"mainline was specified but commit %s is not a merge.\"),\n \t\t\toid_to_hex(&commit->object.oid));\n@@ -1758,16 +1758,15 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\tbase_label = msg.label;\n \t\tnext = parent;\n \t\tnext_label = msg.parent_label;\n-\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n-\t\tstrbuf_addstr(&msgbuf, msg.subject);\n-\t\tstrbuf_addstr(&msgbuf, \"\\\"\\n\\nThis reverts commit \");\n-\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n-\n-\t\tif (commit->parents && commit->parents->next) {\n-\t\t\tstrbuf_addstr(&msgbuf, \", reversing\\nchanges made to \");\n-\t\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid));\n-\t\t}\n-\t\tstrbuf_addstr(&msgbuf, \".\\n\");\n+\t\tstrbuf_addf(&msgbuf, \"Revert \\\"%s\\\"\\n\\n\", msg.subject);\n+\n+\t\tif (parent_id != -1)\n+\t\t\tstrbuf_addf(&msgbuf, \"Revert: %s~%d\\n\",\n+\t\t\t\t oid_to_hex(&commit->object.oid),\n+\t\t\t\t parent_id);\n+\t\telse\n+\t\t\tstrbuf_addf(&msgbuf, \"Revert: %s\\n\",\n+\t\t\t\t oid_to_hex(&commit->object.oid));\n \t} else {\n \t\tconst char *p;\n \n@@ -1784,9 +1783,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\tstrbuf_complete_line(&msgbuf);\n \t\t\tif (!has_conforming_footer(&msgbuf, NULL, 0))\n \t\t\t\tstrbuf_addch(&msgbuf, '\\n');\n-\t\t\tstrbuf_addstr(&msgbuf, cherry_picked_prefix);\n-\t\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n-\t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n+\t\t\tif (parent_id != -1)\n+\t\t\t\tstrbuf_addf(&msgbuf, \"Cherry-picked-from: %s~%d\\n\",\n+\t\t\t\t\t oid_to_hex(&commit->object.oid),\n+\t\t\t\t\t parent_id);\n+\t\t\telse\n+\t\t\t\tstrbuf_addf(&msgbuf, \"Cherry-picked-from: %s\\n\",\n+\t\t\t\t\t oid_to_hex(&commit->object.oid));\n \t\t}\n \t\tif (!is_fixup(command))\n \t\t\tauthor = get_author(msg.message);\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex c84eeefdc9..504423ec20 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -390,10 +390,10 @@ test_expect_success '--continue respects opts' '\n \tgit cat-file commit HEAD~1 >picked_msg &&\n \tgit cat-file commit HEAD~2 >unrelatedpick_msg &&\n \tgit cat-file commit HEAD~3 >initial_msg &&\n-\t! grep \"cherry picked from\" initial_msg &&\n-\tgrep \"cherry picked from\" unrelatedpick_msg &&\n-\tgrep \"cherry picked from\" picked_msg &&\n-\tgrep \"cherry picked from\" anotherpick_msg\n+\t! grep \"Cherry-picked-from:\" initial_msg &&\n+\tgrep \"Cherry-picked-from: \" unrelatedpick_msg &&\n+\tgrep \"Cherry-picked-from: \" picked_msg &&\n+\tgrep \"Cherry-picked-from: \" anotherpick_msg\n '\n \n test_expect_success '--continue of single-pick respects -x' '\n@@ -404,7 +404,7 @@ test_expect_success '--continue of single-pick respects -x' '\n \tgit cherry-pick --continue &&\n \ttest_path_is_missing .git/sequencer &&\n \tgit cat-file commit HEAD >msg &&\n-\tgrep \"cherry picked from\" msg\n+\tgrep \"Cherry-picked-from:\" msg\n '\n \n test_expect_success '--continue respects -x in first commit in multi-pick' '\n@@ -416,7 +416,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '\n \ttest_path_is_missing .git/sequencer &&\n \tgit cat-file commit HEAD^ >msg &&\n \tpicked=$(git rev-parse --verify picked) &&\n-\tgrep \"cherry picked from.*$picked\" msg\n+\tgrep \"Cherry-picked-from: $picked\" msg\n '\n \n test_expect_failure '--signoff is automatically propagated to resolved conflict' '\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex 9888bf34b9..798fdaf8da 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -32,7 +32,7 @@ mesg_with_footer_sob=\"$mesg_with_footer\n Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n \n mesg_with_cherry_footer=\"$mesg_with_footer_sob\n-(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)\n+Cherry-picked-from: da39a3ee5e6b4b0d3255bfef95601890afd80709\n Tested-by: C.U. Thor \"\n \n mesg_unclean=\"$mesg_one_line\n@@ -81,7 +81,7 @@ test_expect_success 'cherry-pick -x inserts blank line after one line subject' '\n \tcat <<-EOF >expect &&\n \t\t$mesg_one_line\n \n-\t\t(cherry picked from commit $sha1)\n+\t\tCherry-picked-from: $sha1\n \tEOF\n \tgit log -1 --pretty=format:%B >actual &&\n \ttest_cmp expect actual\n@@ -129,7 +129,7 @@ test_expect_success 'cherry-pick -x inserts blank line when conforming footer no\n \tcat <<-EOF >expect &&\n \t\t$mesg_no_footer\n \n-\t\t(cherry picked from commit $sha1)\n+\t\tCherry-picked-from: $sha1\n \tEOF\n \tgit log -1 --pretty=format:%B >actual &&\n \ttest_cmp expect actual\n@@ -154,7 +154,7 @@ test_expect_success 'cherry-pick -x -s inserts blank line when conforming footer\n \tcat <<-EOF >expect &&\n \t\t$mesg_no_footer\n \n-\t\t(cherry picked from commit $sha1)\n+\t\tCherry-picked-from: $sha1\n \t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n \tEOF\n \tgit log -1 --pretty=format:%B >actual &&\n@@ -178,7 +178,7 @@ test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match commi\n \tgit cherry-pick -x -s mesg-with-footer &&\n \tcat <<-EOF >expect &&\n \t\t$mesg_with_footer\n-\t\t(cherry picked from commit $sha1)\n+\t\tCherry-picked-from: $sha1\n \t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n \tEOF\n \tgit log -1 --pretty=format:%B >actual &&\n@@ -201,7 +201,7 @@ test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo\n \tgit cherry-pick -x -s mesg-with-footer-sob &&\n \tcat <<-EOF >expect &&\n \t\t$mesg_with_footer_sob\n-\t\t(cherry picked from commit $sha1)\n+\t\tCherry-picked-from: $sha1\n \t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n \tEOF\n \tgit log -1 --pretty=format:%B >actual &&\n@@ -215,7 +215,7 @@ test_expect_success 'cherry-pick -x handles commits with no NL at end of message\n \tgit cherry-pick -x $sha1 &&\n \tgit log -1 --pretty=format:%B >actual &&\n \n-\tprintf \"\\n(cherry picked from commit %s)\\n\" $sha1 >>msg &&\n+\tprintf \"\\nCherry-picked-from: %s\\n\" $sha1 >>msg &&\n \ttest_cmp msg actual\n '\n \n@@ -226,7 +226,7 @@ test_expect_success 'cherry-pick -x handles commits with no footer and no NL at\n \tgit cherry-pick -x $sha1 &&\n \tgit log -1 --pretty=format:%B >actual &&\n \n-\tprintf \"\\n\\n(cherry picked from commit %s)\\n\" $sha1 >>msg &&\n+\tprintf \"\\n\\nCherry-picked-from: %s\\n\" $sha1 >>msg &&\n \ttest_cmp msg actual\n '\n \n@@ -252,19 +252,19 @@ test_expect_success 'cherry-pick -s handles commits with no footer and no NL at\n \ttest_cmp msg actual\n '\n \n-test_expect_success 'cherry-pick -x treats \"(cherry picked from...\" line as part of footer' '\n+test_expect_success 'cherry-pick -x treats \"Cherry-picked-from:\" line as part of footer' '\n \tpristine_detach initial &&\n \tsha1=$(git rev-parse mesg-with-cherry-footer^0) &&\n \tgit cherry-pick -x mesg-with-cherry-footer &&\n \tcat <<-EOF >expect &&\n \t\t$mesg_with_cherry_footer\n-\t\t(cherry picked from commit $sha1)\n+\t\tCherry-picked-from: $sha1\n \tEOF\n \tgit log -1 --pretty=format:%B >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'cherry-pick -s treats \"(cherry picked from...\" line as part of footer' '\n+test_expect_success 'cherry-pick -s treats \"Cherry-picked-from:\" line as part of footer' '\n \tpristine_detach initial &&\n \tgit cherry-pick -s mesg-with-cherry-footer &&\n \tcat <<-EOF >expect &&\n@@ -275,13 +275,13 @@ test_expect_success 'cherry-pick -s treats \"(cherry picked from...\" line as part\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'cherry-pick -x -s treats \"(cherry picked from...\" line as part of footer' '\n+test_expect_success 'cherry-pick -x -s treats \"Cherry-picked-from:...\" line as part of footer' '\n \tpristine_detach initial &&\n \tsha1=$(git rev-parse mesg-with-cherry-footer^0) &&\n \tgit cherry-pick -x -s mesg-with-cherry-footer &&\n \tcat <<-EOF >expect &&\n \t\t$mesg_with_cherry_footer\n-\t\t(cherry picked from commit $sha1)\n+\t\tCherry-picked-from: $sha1\n \t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n \tEOF\n \tgit log -1 --pretty=format:%B >actual &&\n-- \n2.19.1.1005.gac84295441\n\n"},{"id":"362414","messageId":"1a667e9c-3944-4bc3-fb60-68e79dff0c34@talktalk.net","threadId":"49753","inReplyTo":"CACsJy8CAw6yR5vbsFJbSd+UTR_1UpS=8PP3hEtbSNSi3SkMNag@mail.gmail.com","subject":"Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-04T21:12:53Z","receivedAt":"2018-11-04T21:12:58Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Duy\n\nOn 04/11/2018 17:41, Duy Nguyen wrote:\n> On Sun, Nov 4, 2018 at 5:45 PM Phillip Wood wrote:\n>>\n>> On 04/11/2018 07:22, Nguyễn Thái Ngọc Duy wrote:\n>>> When a commit is reverted (or cherry-picked with -x) we add an English\n>>> sentence recording that commit id in the new commit message. Make\n>>> these real trailer lines instead so that they are more friendly to\n>>> parsers (especially \"git interpret-trailers\").\n>>>\n>>> A reverted commit will have a new trailer\n>>>\n>>> Revert: \n>>>\n>>> Similarly a cherry-picked commit with -x will have\n>>>\n>>> Cherry-Pick: \n>>\n>> I think this is a good idea though I wonder if it will break someones\n>> script that is looking for the messages generated by -x at the moment.\n> \n> It will [1] but I still think it's worth the trouble. The script will\n> be less likely to break after, and you can use git-interpret-trailers\n> instead of plain grep.\n> \n> [1] https://public-inbox.org/git/20181017143921.GR270328@devbig004.ftw2.facebook.com/\n> \n>>> @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n>>> base_label = msg.label;\n>>> next = parent;\n>>> next_label = msg.parent_label;\n>>> - strbuf_addstr(&msgbuf, \"Revert \\\"\");\n>>> - strbuf_addstr(&msgbuf, msg.subject);\n>>> - strbuf_addstr(&msgbuf, \"\\\"\\n\\nThis reverts commit \");\n>>> - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n>>> -\n>>> - if (commit->parents && commit->parents->next) {\n>>> - strbuf_addstr(&msgbuf, \", reversing\\nchanges made to \");\n>>> - strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid));\n>>> - }\n>>\n>> As revert currently records the parent given on the command line when\n>> reverting a merge commit it would probably be a good idea to add that\n>> either as a separate trailer or to the Revert: trailer and possibly also\n>> generate it for cherry picks.\n> \n> My mistake. I didn't read carefully and thought it was logging\n> commit->parents, which is pointless.\n> \n> So what should be the trailer for this (I don't think putting it in\n> Revert: is a good idea, too much to parse)? Revert-parent: ?\n> Revert-merge: ?\n\nI think Revert-parent: is good, though you seem to have gone for \nincluding it the Revert: trailer in v2.\n>>> - strbuf_addstr(&msgbuf, \".\\n\");\n>>> + strbuf_addf(&msgbuf, \"Revert \\\"%s\\\"\\n\\n\", msg.subject);\n>>\n>> If the message already contains trailers then should we just append the\n>> Revert trailer those rather than inserting \"\\n\\n\"?\n> \n> Umm.. but this \\n\\n is for separating the subject and the body. I\n> think we need it anyway, trailer or not.\n\nAh you're right, I had forgotten that the revert message body is empty, \nunlike cherry-pick where the message is copied (and that does the right \nthing already when there are existing trailers).\n\nBest wishes\n\nPhillip\n\n\n>>> @@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n>>> strbuf_complete_line(&msgbuf);\n>>> if (!has_conforming_footer(&msgbuf, NULL, 0))\n>>> strbuf_addch(&msgbuf, '\\n');\n>>> - strbuf_addstr(&msgbuf, cherry_picked_prefix);\n>>> - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n>>> - strbuf_addstr(&msgbuf, \")\\n\");\n>>> + strbuf_addf(&msgbuf, \"Cherry-Pick: %s\\n\",\n>>> + oid_to_hex(&commit->object.oid));\n> \n> I will probably make this \"Cherry-picked-from:\" to match our S-o-b style.\n> \n\n"},{"id":"362415","messageId":"20181104213007.GL731755@genre.crustytoothpaste.net","threadId":"49753","inReplyTo":"20181104181026.8451-1-pclouds@gmail.com","subject":"Re: [PATCH/RFC v2] sequencer.c: record revert/cherry-pick commit with trailer lines","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-11-04T21:30:07Z","receivedAt":"2018-11-04T21:30:16Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Nov 04, 2018 at 07:10:26PM +0100, Nguyễn Thái Ngọc Duy wrote:\n> When a commit is reverted (or cherry-picked with -x) we add an English\n> sentence recording that commit id in the new commit message. Make\n> these real trailer lines instead so that they are more friendly to\n> parsers (especially \"git interpret-trailers\").\n> \n> A reverted commit will have a new trailer\n> \n> Revert: \n> \n> Similarly a cherry-picked commit with -x will have\n> \n> Cherry-picked-from: \n> \n> When reverting or cherry picking a merge, the reverted/cherry-picked\n> branch will be shown using extended SHA-1 syntax, e.g.\n> \n> Revert: ~2\n> \n> Since we're not producing the old lines \"This reverts commit ...\" and\n> \"(cherry picked from commit ..\" anymore, scripts that look for these\n> lines will need to be updated to handle both. Fresh new history could\n> just rely on git-interpret-trailers instead.\n\nOverall, I like the idea of this series. This is a much cleaner way to\nhandle things and much better for machine-readability. I foresee git\ncherry potentially learning how to parse this, for example, for cases\nwhere the patch-id doesn't match due to context changes.\n\nHowever, I do have concerns about breaking compatibility with existing\nscripts. I wonder if we could add a long alias for git cherry-pick -x,\nsay \"--notate\" and have \"--notate=text\" mean \"-x\" and \"--notate=trailer\"\nmean this new format. Similarly, git revert could learn such an option\nas well.\n\nOne final thought: since our trailers seem to act as if we wrote \"this\ncommit\" (has been), I wonder if we should say \"Reverts\" instead of\n\"Revert\" for consistency.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"362434","messageId":"xmqqtvkwl4u8.fsf@gitster-ct.c.googlers.com","threadId":"49753","inReplyTo":"20181104072253.12357-1-pclouds@gmail.com","subject":"Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-05T00:56:31Z","receivedAt":"2018-11-05T00:56:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> A reverted commit will have a new trailer\n>\n> Revert: \n\nPlease don't, unless you are keeping the current \"the effect of\ncommit X relative to its parent Y was reverted\" writtein in prose,\nwhich is meant to be followed up with a manually written \"because\n...\" and adding this as an extra footer that is meant solely for\nmachine consumption. Of course reversion of a merge needs to say\nrelative to which parent of the merge it is undoing.\n\n> Similarly a cherry-picked commit with -x will have\n>\n> Cherry-Pick: \n\nUnlike the \"revert\" change above, this probably is a good change, as\na\"(cherry-pickt-from: X)\" does not try to convey anything more or\nanything less than such a standard looking trailer and it is in\ndifferent shape only by historical accident. But people's scripts\nmay need to have a long transition period for this change to happen.\n\n\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> I think standardizing how we record commit ids in the commit message\n> is a good idea. Though to be honest this started because of my irk of\n> an English string \"cherry picked from...\" that cannot be translated.\n> It might as well be a computer language that happens to look like\n> English.\n>\n> Documentation/git-cherry-pick.txt | 5 ++---\n> sequencer.c | 20 ++++++--------------\n> t/t3510-cherry-pick-sequence.sh | 12 ++++++------\n> t/t3511-cherry-pick-x.sh | 26 +++++++++++++-------------\n> 4 files changed, 27 insertions(+), 36 deletions(-)\n>\n> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\n> index d35d771fc8..8ffbaed1a0 100644\n> --- a/Documentation/git-cherry-pick.txt\n> +++ b/Documentation/git-cherry-pick.txt\n> @@ -58,9 +58,8 @@ OPTIONS\n> \tmessage prior to committing.\n> \n> -x::\n> -\tWhen recording the commit, append a line that says\n> -\t\"(cherry picked from commit ...)\" to the original commit\n> -\tmessage in order to indicate which commit this change was\n> +\tWhen recording the commit, append \"Cherry-Pick:\" trailer line\n> +\trecording the commit name which commit this change was\n> \tcherry-picked from. This is done only for cherry\n> \tpicks without conflicts. Do not use this option if\n> \tyou are cherry-picking from your private branch because\n> diff --git a/sequencer.c b/sequencer.c\n> index 9e1ab3a2a7..f7318f2862 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -36,7 +36,6 @@\n> #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n> \n> const char sign_off_header[] = \"Signed-off-by: \";\n> -static const char cherry_picked_prefix[] = \"(cherry picked from commit \";\n> \n> GIT_PATH_FUNC(git_path_commit_editmsg, \"COMMIT_EDITMSG\")\n> \n> @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n> \t\tbase_label = msg.label;\n> \t\tnext = parent;\n> \t\tnext_label = msg.parent_label;\n> -\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n> -\t\tstrbuf_addstr(&msgbuf, msg.subject);\n> -\t\tstrbuf_addstr(&msgbuf, \"\\\"\\n\\nThis reverts commit \");\n> -\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n> -\n> -\t\tif (commit->parents && commit->parents->next) {\n> -\t\t\tstrbuf_addstr(&msgbuf, \", reversing\\nchanges made to \");\n> -\t\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid));\n> -\t\t}\n> -\t\tstrbuf_addstr(&msgbuf, \".\\n\");\n> +\t\tstrbuf_addf(&msgbuf, \"Revert \\\"%s\\\"\\n\\n\", msg.subject);\n> +\n> +\t\tstrbuf_addf(&msgbuf, \"Revert: %s\\n\",\n> +\t\t\t oid_to_hex(&commit->object.oid));\n> \t} else {\n> \t\tconst char *p;\n> \n> @@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n> \t\t\tstrbuf_complete_line(&msgbuf);\n> \t\t\tif (!has_conforming_footer(&msgbuf, NULL, 0))\n> \t\t\t\tstrbuf_addch(&msgbuf, '\\n');\n> -\t\t\tstrbuf_addstr(&msgbuf, cherry_picked_prefix);\n> -\t\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n> -\t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n> +\t\t\tstrbuf_addf(&msgbuf, \"Cherry-Pick: %s\\n\",\n> +\t\t\t\t oid_to_hex(&commit->object.oid));\n> \t\t}\n> \t\tif (!is_fixup(command))\n> \t\t\tauthor = get_author(msg.message);\n> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\n> index c84eeefdc9..89b6e7fc1e 100755\n> --- a/t/t3510-cherry-pick-sequence.sh\n> +++ b/t/t3510-cherry-pick-sequence.sh\n> @@ -390,10 +390,10 @@ test_expect_success '--continue respects opts' '\n> \tgit cat-file commit HEAD~1 >picked_msg &&\n> \tgit cat-file commit HEAD~2 >unrelatedpick_msg &&\n> \tgit cat-file commit HEAD~3 >initial_msg &&\n> -\t! grep \"cherry picked from\" initial_msg &&\n> -\tgrep \"cherry picked from\" unrelatedpick_msg &&\n> -\tgrep \"cherry picked from\" picked_msg &&\n> -\tgrep \"cherry picked from\" anotherpick_msg\n> +\t! grep \"Cherry-Pick:\" initial_msg &&\n> +\tgrep \"Cherry-Pick: \" unrelatedpick_msg &&\n> +\tgrep \"Cherry-Pick: \" picked_msg &&\n> +\tgrep \"Cherry-Pick: \" anotherpick_msg\n> '\n> \n> test_expect_success '--continue of single-pick respects -x' '\n> @@ -404,7 +404,7 @@ test_expect_success '--continue of single-pick respects -x' '\n> \tgit cherry-pick --continue &&\n> \ttest_path_is_missing .git/sequencer &&\n> \tgit cat-file commit HEAD >msg &&\n> -\tgrep \"cherry picked from\" msg\n> +\tgrep \"Cherry-Pick:\" msg\n> '\n> \n> test_expect_success '--continue respects -x in first commit in multi-pick' '\n> @@ -416,7 +416,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '\n> \ttest_path_is_missing .git/sequencer &&\n> \tgit cat-file commit HEAD^ >msg &&\n> \tpicked=$(git rev-parse --verify picked) &&\n> -\tgrep \"cherry picked from.*$picked\" msg\n> +\tgrep \"Cherry-Pick: $picked\" msg\n> '\n> \n> test_expect_failure '--signoff is automatically propagated to resolved conflict' '\n> diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\n> index 9888bf34b9..db11dd2430 100755\n> --- a/t/t3511-cherry-pick-x.sh\n> +++ b/t/t3511-cherry-pick-x.sh\n> @@ -32,7 +32,7 @@ mesg_with_footer_sob=\"$mesg_with_footer\n> Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n> \n> mesg_with_cherry_footer=\"$mesg_with_footer_sob\n> -(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)\n> +Cherry-Pick: da39a3ee5e6b4b0d3255bfef95601890afd80709\n> Tested-by: C.U. Thor \"\n> \n> mesg_unclean=\"$mesg_one_line\n> @@ -81,7 +81,7 @@ test_expect_success 'cherry-pick -x inserts blank line after one line subject' '\n> \tcat <<-EOF >expect &&\n> \t\t$mesg_one_line\n> \n> -\t\t(cherry picked from commit $sha1)\n> +\t\tCherry-Pick: $sha1\n> \tEOF\n> \tgit log -1 --pretty=format:%B >actual &&\n> \ttest_cmp expect actual\n> @@ -129,7 +129,7 @@ test_expect_success 'cherry-pick -x inserts blank line when conforming footer no\n> \tcat <<-EOF >expect &&\n> \t\t$mesg_no_footer\n> \n> -\t\t(cherry picked from commit $sha1)\n> +\t\tCherry-Pick: $sha1\n> \tEOF\n> \tgit log -1 --pretty=format:%B >actual &&\n> \ttest_cmp expect actual\n> @@ -154,7 +154,7 @@ test_expect_success 'cherry-pick -x -s inserts blank line when conforming footer\n> \tcat <<-EOF >expect &&\n> \t\t$mesg_no_footer\n> \n> -\t\t(cherry picked from commit $sha1)\n> +\t\tCherry-Pick: $sha1\n> \t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n> \tEOF\n> \tgit log -1 --pretty=format:%B >actual &&\n> @@ -178,7 +178,7 @@ test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match commi\n> \tgit cherry-pick -x -s mesg-with-footer &&\n> \tcat <<-EOF >expect &&\n> \t\t$mesg_with_footer\n> -\t\t(cherry picked from commit $sha1)\n> +\t\tCherry-Pick: $sha1\n> \t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n> \tEOF\n> \tgit log -1 --pretty=format:%B >actual &&\n> @@ -201,7 +201,7 @@ test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo\n> \tgit cherry-pick -x -s mesg-with-footer-sob &&\n> \tcat <<-EOF >expect &&\n> \t\t$mesg_with_footer_sob\n> -\t\t(cherry picked from commit $sha1)\n> +\t\tCherry-Pick: $sha1\n> \t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n> \tEOF\n> \tgit log -1 --pretty=format:%B >actual &&\n> @@ -215,7 +215,7 @@ test_expect_success 'cherry-pick -x handles commits with no NL at end of message\n> \tgit cherry-pick -x $sha1 &&\n> \tgit log -1 --pretty=format:%B >actual &&\n> \n> -\tprintf \"\\n(cherry picked from commit %s)\\n\" $sha1 >>msg &&\n> +\tprintf \"\\nCherry-Pick: %s\\n\" $sha1 >>msg &&\n> \ttest_cmp msg actual\n> '\n> \n> @@ -226,7 +226,7 @@ test_expect_success 'cherry-pick -x handles commits with no footer and no NL at\n> \tgit cherry-pick -x $sha1 &&\n> \tgit log -1 --pretty=format:%B >actual &&\n> \n> -\tprintf \"\\n\\n(cherry picked from commit %s)\\n\" $sha1 >>msg &&\n> +\tprintf \"\\n\\nCherry-Pick: %s\\n\" $sha1 >>msg &&\n> \ttest_cmp msg actual\n> '\n> \n> @@ -252,19 +252,19 @@ test_expect_success 'cherry-pick -s handles commits with no footer and no NL at\n> \ttest_cmp msg actual\n> '\n> \n> -test_expect_success 'cherry-pick -x treats \"(cherry picked from...\" line as part of footer' '\n> +test_expect_success 'cherry-pick -x treats \"Cherry-Pick:\" line as part of footer' '\n> \tpristine_detach initial &&\n> \tsha1=$(git rev-parse mesg-with-cherry-footer^0) &&\n> \tgit cherry-pick -x mesg-with-cherry-footer &&\n> \tcat <<-EOF >expect &&\n> \t\t$mesg_with_cherry_footer\n> -\t\t(cherry picked from commit $sha1)\n> +\t\tCherry-Pick: $sha1\n> \tEOF\n> \tgit log -1 --pretty=format:%B >actual &&\n> \ttest_cmp expect actual\n> '\n> \n> -test_expect_success 'cherry-pick -s treats \"(cherry picked from...\" line as part of footer' '\n> +test_expect_success 'cherry-pick -s treats \"Cherry-Pick:\" line as part of footer' '\n> \tpristine_detach initial &&\n> \tgit cherry-pick -s mesg-with-cherry-footer &&\n> \tcat <<-EOF >expect &&\n> @@ -275,13 +275,13 @@ test_expect_success 'cherry-pick -s treats \"(cherry picked from...\" line as part\n> \ttest_cmp expect actual\n> '\n> \n> -test_expect_success 'cherry-pick -x -s treats \"(cherry picked from...\" line as part of footer' '\n> +test_expect_success 'cherry-pick -x -s treats \"Cherry-Pick:...\" line as part of footer' '\n> \tpristine_detach initial &&\n> \tsha1=$(git rev-parse mesg-with-cherry-footer^0) &&\n> \tgit cherry-pick -x -s mesg-with-cherry-footer &&\n> \tcat <<-EOF >expect &&\n> \t\t$mesg_with_cherry_footer\n> -\t\t(cherry picked from commit $sha1)\n> +\t\tCherry-Pick: $sha1\n> \t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n> \tEOF\n> \tgit log -1 --pretty=format:%B >actual &&\n"},{"id":"362490","messageId":"CACsJy8AJY=J=YF1w-aWZkMYeNa9R05ThAqka7vDFNpvre5TB0g@mail.gmail.com","threadId":"49753","inReplyTo":"20181104213007.GL731755@genre.crustytoothpaste.net","subject":"Re: [PATCH/RFC v2] sequencer.c: record revert/cherry-pick commit with trailer lines","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-05T16:08:22Z","receivedAt":"2018-11-05T16:08:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Nov 4, 2018 at 10:30 PM brian m. carlson\n wrote:\n> However, I do have concerns about breaking compatibility with existing\n> scripts. I wonder if we could add a long alias for git cherry-pick -x,\n> say \"--notate\" and have \"--notate=text\" mean \"-x\" and \"--notate=trailer\"\n> mean this new format. Similarly, git revert could learn such an option\n> as well.\n\nI don't think it will help unless you are the only developer on some\nrepo. If you have some scripts parsing the old format, people could\nchoose to commit using the new format anyway and your scripts will\nhave to adapt (it's too late to revert because it's already part of\ngit history).\n\nThe transition plan could be outputing both old and new formats at the\nsame time (optionally allowing to disable the old one) and leave it\nlike that for a couple releases. Then we could stop producing the old\noutput and hope that all the scripts in the world have caught up. Not\na great plan.\n\n> One final thought: since our trailers seem to act as if we wrote \"this\n> commit\" (has been), I wonder if we should say \"Reverts\" instead of\n> \"Revert\" for consistency.\n\nYeah that or Reverting:, both are better than Revert:. I guess I'll\njust go with Reverts: if this patch moves forward.\n-- \nDuy\n"},{"id":"362491","messageId":"CACsJy8AzriX4YYSSZN-U06HHKgL9XmkJ1WhWbLo4LwZvECa8hw@mail.gmail.com","threadId":"49753","inReplyTo":"xmqqtvkwl4u8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-05T16:17:32Z","receivedAt":"2018-11-05T16:18:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Nov 5, 2018 at 1:56 AM Junio C Hamano wrote:\n>\n> Nguyễn Thái Ngọc Duy writes:\n>\n> > A reverted commit will have a new trailer\n> >\n> > Revert: \n>\n> Please don't, unless you are keeping the current \"the effect of\n> commit X relative to its parent Y was reverted\" writtein in prose,\n> which is meant to be followed up with a manually written \"because\n> ...\" and adding this as an extra footer that is meant solely for\n> machine consumption. Of course reversion of a merge needs to say\n> relative to which parent of the merge it is undoing.\n\nI think the intent of writing \"This reverts .... \" to encourage\nwriting \"because ...\" is good, but in practice many people just simply\nnot do it. And by not describing anything at all (footers don't count)\nsome commit hook can force people to actually write something.\n\nBut for the transition period I think we need to keep both anyway,\nwhether this \"This reverts ...\" should stay could be revisited another\nday (or not, even).\n\n> > Similarly a cherry-picked commit with -x will have\n> >\n> > Cherry-Pick: \n>\n> Unlike the \"revert\" change above, this probably is a good change, as\n> a\"(cherry-pickt-from: X)\" does not try to convey anything more or\n> anything less than such a standard looking trailer and it is in\n> different shape only by historical accident. But people's scripts\n> may need to have a long transition period for this change to happen.\n\nYep. I'll code something up to print both by default with config knobs\nto disable either. Unless you have some better plan of course.\n-- \nDuy\n"},{"id":"362534","messageId":"nycvar.QRO.7.76.6.1811052255080.86@tvgsbejvaqbjf.bet","threadId":"49753","inReplyTo":"CACsJy8CAw6yR5vbsFJbSd+UTR_1UpS=8PP3hEtbSNSi3SkMNag@mail.gmail.com","subject":"Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-05T21:57:14Z","receivedAt":"2018-11-05T21:57:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 4 Nov 2018, Duy Nguyen wrote:\n\n> On Sun, Nov 4, 2018 at 5:45 PM Phillip Wood wrote:\n> >\n> > On 04/11/2018 07:22, Nguyễn Thái Ngọc Duy wrote:\n> > > When a commit is reverted (or cherry-picked with -x) we add an English\n> > > sentence recording that commit id in the new commit message. Make\n> > > these real trailer lines instead so that they are more friendly to\n> > > parsers (especially \"git interpret-trailers\").\n> > >\n> > > A reverted commit will have a new trailer\n> > >\n> > > Revert: \n> > >\n> > > Similarly a cherry-picked commit with -x will have\n> > >\n> > > Cherry-Pick: \n> >\n> > I think this is a good idea though I wonder if it will break someones\n> > script that is looking for the messages generated by -x at the moment.\n> \n> It will [1] but I still think it's worth the trouble. The script will\n> be less likely to break after, and you can use git-interpret-trailers\n> instead of plain grep.\n\nSince this is a wilfull backwards-incompatible patch, it needs to be done\ncarefully.\n\nI am not enthused by the idea of breaking power users' setups, and I could\nimagine that a much better way to do it would be to introduce a config\nsetting that guards the new behavior, then add a deprecation notice when\n-x is used without that config setting, and then let that simmer for a\ncouple of versions.\n\nCiao,\nJohannes\n\n> \n> [1] https://public-inbox.org/git/20181017143921.GR270328@devbig004.ftw2.facebook.com/\n> \n> > > @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n> > > base_label = msg.label;\n> > > next = parent;\n> > > next_label = msg.parent_label;\n> > > - strbuf_addstr(&msgbuf, \"Revert \\\"\");\n> > > - strbuf_addstr(&msgbuf, msg.subject);\n> > > - strbuf_addstr(&msgbuf, \"\\\"\\n\\nThis reverts commit \");\n> > > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n> > > -\n> > > - if (commit->parents && commit->parents->next) {\n> > > - strbuf_addstr(&msgbuf, \", reversing\\nchanges made to \");\n> > > - strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid));\n> > > - }\n> >\n> > As revert currently records the parent given on the command line when\n> > reverting a merge commit it would probably be a good idea to add that\n> > either as a separate trailer or to the Revert: trailer and possibly also\n> > generate it for cherry picks.\n> \n> My mistake. I didn't read carefully and thought it was logging\n> commit->parents, which is pointless.\n> \n> So what should be the trailer for this (I don't think putting it in\n> Revert: is a good idea, too much to parse)? Revert-parent: ?\n> Revert-merge: ?\n> \n> > > - strbuf_addstr(&msgbuf, \".\\n\");\n> > > + strbuf_addf(&msgbuf, \"Revert \\\"%s\\\"\\n\\n\", msg.subject);\n> >\n> > If the message already contains trailers then should we just append the\n> > Revert trailer those rather than inserting \"\\n\\n\"?\n> \n> Umm.. but this \\n\\n is for separating the subject and the body. I\n> think we need it anyway, trailer or not.\n> \n> > > @@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n> > > strbuf_complete_line(&msgbuf);\n> > > if (!has_conforming_footer(&msgbuf, NULL, 0))\n> > > strbuf_addch(&msgbuf, '\\n');\n> > > - strbuf_addstr(&msgbuf, cherry_picked_prefix);\n> > > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n> > > - strbuf_addstr(&msgbuf, \")\\n\");\n> > > + strbuf_addf(&msgbuf, \"Cherry-Pick: %s\\n\",\n> > > + oid_to_hex(&commit->object.oid));\n> \n> I will probably make this \"Cherry-picked-from:\" to match our S-o-b style.\n> -- \n> Duy\n> "},{"id":"362541","messageId":"xmqqva5bgetj.fsf@gitster-ct.c.googlers.com","threadId":"49753","inReplyTo":"CACsJy8AzriX4YYSSZN-U06HHKgL9XmkJ1WhWbLo4LwZvECa8hw@mail.gmail.com","subject":"Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-06T01:44:24Z","receivedAt":"2018-11-06T01:44:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> I think the intent of writing \"This reverts .... \" to encourage\n> writing \"because ...\" is good, but in practice many people just simply\n> not do it. And by not describing anything at all (footers don't count)\n> some commit hook can force people to actually write something.\n>\n> But for the transition period I think we need to keep both anyway,\n\nI do not see any need to qualify the statement with \"for the\ntransition period\". You showed *no* need to transition, but\nI do agree that adding a fixed-spelled footer in addition to\nwhat we produce in the body is a good idea.\n\nWhen we know a feature with good intent is not used properly by many\npeople, the first thing to do is not talk about removing it, but to\nthink how we can educate people to make good use of it.\n\nAnd if we know a feature with good intent is not used by many people\nbut we know that \"many\" is not \"100%\", why are we talking about\nremoving it at all?\n\n> Yep. I'll code something up to print both by default with config knobs\n> to disable either. Unless you have some better plan of course.\n\nDoes it make sense to put both, with exactly the same piece of\ninformation? I am not sure whom it would help.\n\nThe tools need to be updated to deal with both old and new format if\nthe pick-origin information is used, instead of getting updated to\nlearn new and forget old format, as existing commits in their\nhistory do not know about the new format and their tools need to\nunderstand them.\n\nI'd say it would be sufficient to have a per-repository knob that\nsays which one to use, and between the release we add that knob and\nthe release we make the new format the default, when we do not see\nthe knob is set to either way, keep warning that in the future the\ndefault will change and give advice that the knob can be used to\neither keep the old format or update to the new format before or\nafter the default switch (in addition to squelch the warning and the\nadvice).\n"},{"id":"362565","messageId":"87bm727fcw.fsf@evledraar.gmail.com","threadId":"49753","inReplyTo":"20181104072253.12357-1-pclouds@gmail.com","subject":"Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-06T08:57:35Z","receivedAt":"2018-11-06T08:57:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Nov 04 2018, Nguyễn Thái Ngọc Duy wrote:\n\n> When a commit is reverted (or cherry-picked with -x) we add an English\n> sentence recording that commit id in the new commit message. Make\n> these real trailer lines instead so that they are more friendly to\n> parsers (especially \"git interpret-trailers\").\n>\n> A reverted commit will have a new trailer\n>\n> Revert: \n>\n> Similarly a cherry-picked commit with -x will have\n>\n> Cherry-Pick: \n> [...]\n> I think standardizing how we record commit ids in the commit message\n> is a good idea. Though to be honest this started because of my irk of\n> an English string \"cherry picked from...\" that cannot be translated.\n> It might as well be a computer language that happens to look like\n> English.\n> [...]\n> @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n> \t\tbase_label = msg.label;\n> \t\tnext = parent;\n> \t\tnext_label = msg.parent_label;\n> -\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n> -\t\tstrbuf_addstr(&msgbuf, msg.subject);\n> -\t\tstrbuf_addstr(&msgbuf, \"\\\"\\n\\nThis reverts commit \");\n> -\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n> -\n> -\t\tif (commit->parents && commit->parents->next) {\n> -\t\t\tstrbuf_addstr(&msgbuf, \", reversing\\nchanges made to \");\n> -\t\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid));\n> -\t\t}\n> -\t\tstrbuf_addstr(&msgbuf, \".\\n\");\n> +\t\tstrbuf_addf(&msgbuf, \"Revert \\\"%s\\\"\\n\\n\", msg.subject);\n> +\n> +\t\tstrbuf_addf(&msgbuf, \"Revert: %s\\n\",\n> +\t\t\t oid_to_hex(&commit->object.oid));\n> \t} else {\n> \t\tconst char *p;\n\nOthers have already commented on the backwards-compatibility /\nswitchover concerns, so I won't spend much time on that. Except to say\nthat I don't think changing this would be a big deal.\n\nAnyone trying to parse out /This reverts commit/ or other pre-set\nEnglish texts we put into the commit object is already needing to deal\nwith users changing the message. E.g. I have a habit of doing partial\nreverts and changing it to \"This partially reverts...\" etc.\n\nLeaving aside the question of whether the pain of switching is worth it,\nI think it's a worthwihle to consider if we could stop hardcoding one\nspecific human language in commit messages, and instead leave something\nmachine-readable behind.\n\nWe do that with reverts, and also with merge commits, which could be\ngiven a similar treatment where we change e.g. \"Merge branches\n'jc/convert', 'jc/bigfile' and 'jc/replacing' into jc/streaming\" (to use\ngit.git's 02071b27f1 as an example) to:\n\n Merge-branch-1: jc/convert\n Merge-branch-2: jc/bigfile\n Merge-branch-3: jc/replacing\n Merge-branch-into: jc/streaming\n\nThen, when rendering the commit in the UI we could parse that out, and\nput a \"Merge branches[...]\" message at the top, except this time in the\nuser's own language.\n"},{"id":"362585","messageId":"CACsJy8Cm6EBVDf6pk47dBdbpe+qdJAM97vQXXW-B1VHF10nTmw@mail.gmail.com","threadId":"49753","inReplyTo":"87bm727fcw.fsf@evledraar.gmail.com","subject":"Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-06T16:13:56Z","receivedAt":"2018-11-06T16:14:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Nov 6, 2018 at 9:57 AM Ævar Arnfjörð Bjarmason wrote:\n> Leaving aside the question of whether the pain of switching is worth it,\n> I think it's a worthwihle to consider if we could stop hardcoding one\n> specific human language in commit messages, and instead leave something\n> machine-readable behind.\n>\n> We do that with reverts, and also with merge commits, which could be\n> given a similar treatment where we change e.g. \"Merge branches\n> 'jc/convert', 'jc/bigfile' and 'jc/replacing' into jc/streaming\" (to use\n> git.git's 02071b27f1 as an example) to:\n>\n> Merge-branch-1: jc/convert\n> Merge-branch-2: jc/bigfile\n> Merge-branch-3: jc/replacing\n> Merge-branch-into: jc/streaming\n>\n> Then, when rendering the commit in the UI we could parse that out, and\n> put a \"Merge branches[...]\" message at the top, except this time in the\n> user's own language.\n\nMy first reaction of this was \"but branch name is a local thing and\nnot significant anyway!\". But then if people use one branch as a\nbundle of other branches like 'pu', then the ability to recreate\nbranches (with the right name of course) from those merges may be\nuseful. So... yeah, maybe.\n-- \nDuy\n"},{"id":"362592","messageId":"20181106171637.15562-1-pclouds@gmail.com","threadId":"49753","inReplyTo":"20181104181026.8451-1-pclouds@gmail.com","subject":"[PATCH/RFC] Support --append-trailer in cherry-pick and revert","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-06T17:16:37Z","receivedAt":"2018-11-06T17:16:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"OK here is a less constroversal attempt to add new trailers. Instead\nof changing the default behavior (which could be done incrementally\nlater), this patch simply adds a new option --append-trailer to revert\nand cherry-pick.\n\nBoth will show either\n\n Reverts: [~]\n\nor\n\n Cherry-picked-from: [~]\n\n--append-trailer could be added to more commands (e.g. merge) that\ngenerate commit messages if they have something for machine\nconsumption.\n\nAfter this, perhaps we could have a config key to turn this on by\ndefault (for revert; for cherry-pick it will turn off \"-x\" too). Then\nafter a couple releases, the we got good reception, we'll make it\ndefault?\n\nNo tests, no proper commit message since I think we're still going to\ndiscuss a bit more before settling down.\n\nPS. maybe --append-trailer is too generic? We have --signoff which is\nalso a trailer. But that one carries more weights than just \"machine\ngenerated tags\".\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/git-cherry-pick.txt | 6 +++++\n Documentation/git-revert.txt | 6 +++++\n builtin/revert.c | 7 ++++++\n sequencer.c | 39 +++++++++++++++++++++++++++----\n sequencer.h | 1 +\n 5 files changed, 55 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\nindex d35d771fc8..b5dff29ead 100644\n--- a/Documentation/git-cherry-pick.txt\n+++ b/Documentation/git-cherry-pick.txt\n@@ -102,6 +102,12 @@ effect to your index in a row.\n \tAdd Signed-off-by line at the end of the commit message.\n \tSee the signoff option in linkgit:git-commit[1] for more information.\n \n+--append-trailer::\n+\tWhen recording a commit, append a \"Cherry-picked-from:\" line\n+\twith object name of the cherry-picked commit. If a merge is\n+\tcherry-picked with `-m`, the extended SHA-1 syntax is used\n+\tto indicate the side of the merge to be cherry-picked.\n+\n -S[]::\n --gpg-sign[=]::\n \tGPG-sign commits. The `keyid` argument is optional and\ndiff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt\nindex 837707a8fd..e08010b200 100644\n--- a/Documentation/git-revert.txt\n+++ b/Documentation/git-revert.txt\n@@ -91,6 +91,12 @@ effect to your index in a row.\n \tAdd Signed-off-by line at the end of the commit message.\n \tSee the signoff option in linkgit:git-commit[1] for more information.\n \n+--append-trailer::\n+\tWhen recording a commit, append a \"Cherry-picked-from:\" line\n+\twith object name of the cherry-picked commit. If a merge is\n+\tcherry-picked with `-m`, the extended SHA-1 syntax is used\n+\tto indicate the side of the merge to be cherry-picked.\n+\n --strategy=::\n \tUse the given merge strategy. Should only be used once.\n \tSee the MERGE STRATEGIES section in linkgit:git-merge[1]\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex c93393c89b..3bd8f57b90 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -119,6 +119,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \tif (opts->action == REPLAY_PICK) {\n \t\tstruct option cp_extra[] = {\n \t\t\tOPT_BOOL('x', NULL, &opts->record_origin, N_(\"append commit name\")),\n+\t\t\tOPT_BOOL(0, \"append-trailer\", &opts->append_trailer, N_(\"record cherry picked commit as trailer\")),\n \t\t\tOPT_BOOL(0, \"ff\", &opts->allow_ff, N_(\"allow fast-forward\")),\n \t\t\tOPT_BOOL(0, \"allow-empty\", &opts->allow_empty, N_(\"preserve initially empty commits\")),\n \t\t\tOPT_BOOL(0, \"allow-empty-message\", &opts->allow_empty_message, N_(\"allow commits with empty messages\")),\n@@ -126,6 +127,12 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\tOPT_END(),\n \t\t};\n \t\toptions = parse_options_concat(options, cp_extra);\n+\t} else if (opts->action == REPLAY_REVERT) {\n+\t\tstruct option cp_extra[] = {\n+\t\t\tOPT_BOOL(0, \"append-trailer\", &opts->append_trailer, N_(\"record reverted commit as trailer\")),\n+\t\t\tOPT_END(),\n+\t\t};\n+\t\toptions = parse_options_concat(options, cp_extra);\n \t}\n \n \targc = parse_options(argc, argv, NULL, options, usage_str,\ndiff --git a/sequencer.c b/sequencer.c\nindex 9e1ab3a2a7..e8fa307109 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -911,7 +911,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \tif ((flags & EDIT_MSG))\n \t\targv_array_push(&cmd.args, \"-e\");\n \telse if (!(flags & CLEANUP_MSG) &&\n-\t\t !opts->signoff && !opts->record_origin &&\n+\t\t !opts->signoff && !opts->record_origin && !opts->append_trailer &&\n \t\t git_config_get_value(\"commit.cleanup\", &value))\n \t\targv_array_push(&cmd.args, \"--cleanup=verbatim\");\n \n@@ -1669,7 +1669,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \tchar *author = NULL;\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n \tstruct strbuf msgbuf = STRBUF_INIT;\n-\tint res, unborn = 0, allow;\n+\tint res, unborn = 0, allow, parent_id = -1;\n \n \tif (opts->no_commit) {\n \t\t/*\n@@ -1716,6 +1716,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\treturn error(_(\"commit %s does not have parent %d\"),\n \t\t\t\toid_to_hex(&commit->object.oid), opts->mainline);\n \t\tparent = p->item;\n+\t\tparent_id = cnt;\n \t} else if (0 < opts->mainline)\n \t\treturn error(_(\"mainline was specified but commit %s is not a merge.\"),\n \t\t\toid_to_hex(&commit->object.oid));\n@@ -1768,6 +1769,17 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid));\n \t\t}\n \t\tstrbuf_addstr(&msgbuf, \".\\n\");\n+\n+\t\tif (opts->append_trailer) {\n+\t\t\tstrbuf_addstr(&msgbuf, \"\\n\");\n+\t\t\tif (parent_id != -1)\n+\t\t\t\tstrbuf_addf(&msgbuf, \"Reverts: %s~%d\\n\",\n+\t\t\t\t\t oid_to_hex(&commit->object.oid),\n+\t\t\t\t\t parent_id);\n+\t\t\telse\n+\t\t\t\tstrbuf_addf(&msgbuf, \"Reverts: %s\\n\",\n+\t\t\t\t\t oid_to_hex(&commit->object.oid));\n+\t\t}\n \t} else {\n \t\tconst char *p;\n \n@@ -1780,14 +1792,28 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\tif (find_commit_subject(msg.message, &p))\n \t\t\tstrbuf_addstr(&msgbuf, p);\n \n-\t\tif (opts->record_origin) {\n+\t\tif (opts->record_origin || opts->append_trailer) {\n \t\t\tstrbuf_complete_line(&msgbuf);\n \t\t\tif (!has_conforming_footer(&msgbuf, NULL, 0))\n \t\t\t\tstrbuf_addch(&msgbuf, '\\n');\n+\t\t}\n+\n+\t\tif (opts->record_origin) {\n \t\t\tstrbuf_addstr(&msgbuf, cherry_picked_prefix);\n \t\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n \t\t}\n+\t\tif (opts->append_trailer) {\n+\t\t\tif (opts->record_origin)\n+\t\t\t\tstrbuf_addstr(&msgbuf, \"\\n\");\n+\t\t\tif (parent_id != -1)\n+\t\t\t\tstrbuf_addf(&msgbuf, \"Cherry-picked-from: %s~%d\\n\",\n+\t\t\t\t\t oid_to_hex(&commit->object.oid),\n+\t\t\t\t\t parent_id);\n+\t\t\telse\n+\t\t\t\tstrbuf_addf(&msgbuf, \"Cherry-picked-from: %s\\n\",\n+\t\t\t\t\t oid_to_hex(&commit->object.oid));\n+\t\t}\n \t\tif (!is_fixup(command))\n \t\t\tauthor = get_author(msg.message);\n \t}\n@@ -2227,6 +2253,8 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \t\topts->signoff = git_config_bool_or_int(key, value, &error_flag);\n \telse if (!strcmp(key, \"options.record-origin\"))\n \t\topts->record_origin = git_config_bool_or_int(key, value, &error_flag);\n+\telse if (!strcmp(key, \"options.append-trailer\"))\n+\t\topts->append_trailer = git_config_bool_or_int(key, value, &error_flag);\n \telse if (!strcmp(key, \"options.allow-ff\"))\n \t\topts->allow_ff = git_config_bool_or_int(key, value, &error_flag);\n \telse if (!strcmp(key, \"options.mainline\"))\n@@ -2618,6 +2646,8 @@ static int save_opts(struct replay_opts *opts)\n \t\tres |= git_config_set_in_file_gently(opts_file, \"options.signoff\", \"true\");\n \tif (opts->record_origin)\n \t\tres |= git_config_set_in_file_gently(opts_file, \"options.record-origin\", \"true\");\n+\tif (opts->append_trailer)\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.append-trailer\", \"true\");\n \tif (opts->allow_ff)\n \t\tres |= git_config_set_in_file_gently(opts_file, \"options.allow-ff\", \"true\");\n \tif (opts->mainline) {\n@@ -3432,7 +3462,8 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)\n \tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n \tif (opts->allow_ff)\n \t\tassert(!(opts->signoff || opts->no_commit ||\n-\t\t\t\topts->record_origin || opts->edit));\n+\t\t\t opts->record_origin || opts->append_trailer ||\n+\t\t\t opts->edit));\n \tif (read_and_refresh_cache(opts))\n \t\treturn -1;\n \ndiff --git a/sequencer.h b/sequencer.h\nindex 660cff5050..7d7c1fe6b4 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -31,6 +31,7 @@ struct replay_opts {\n \t/* Boolean options */\n \tint edit;\n \tint record_origin;\n+\tint append_trailer;\n \tint no_commit;\n \tint signoff;\n \tint allow_ff;\n-- \n2.19.1.1005.gac84295441\n\n"},{"id":"362595","messageId":"8736se6qyc.fsf@evledraar.gmail.com","threadId":"49753","inReplyTo":"CACsJy8Cm6EBVDf6pk47dBdbpe+qdJAM97vQXXW-B1VHF10nTmw@mail.gmail.com","subject":"Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-06T17:44:43Z","receivedAt":"2018-11-06T17:44:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 06 2018, Duy Nguyen wrote:\n\n> On Tue, Nov 6, 2018 at 9:57 AM Ævar Arnfjörð Bjarmason wrote:\n>> Leaving aside the question of whether the pain of switching is worth it,\n>> I think it's a worthwihle to consider if we could stop hardcoding one\n>> specific human language in commit messages, and instead leave something\n>> machine-readable behind.\n>>\n>> We do that with reverts, and also with merge commits, which could be\n>> given a similar treatment where we change e.g. \"Merge branches\n>> 'jc/convert', 'jc/bigfile' and 'jc/replacing' into jc/streaming\" (to use\n>> git.git's 02071b27f1 as an example) to:\n>>\n>> Merge-branch-1: jc/convert\n>> Merge-branch-2: jc/bigfile\n>> Merge-branch-3: jc/replacing\n>> Merge-branch-into: jc/streaming\n>>\n>> Then, when rendering the commit in the UI we could parse that out, and\n>> put a \"Merge branches[...]\" message at the top, except this time in the\n>> user's own language.\n>\n> My first reaction of this was \"but branch name is a local thing and\n> not significant anyway!\". But then if people use one branch as a\n> bundle of other branches like 'pu', then the ability to recreate\n> branches (with the right name of course) from those merges may be\n> useful. So... yeah, maybe.\n\nYeah, in theory, in practice no. But all I'm observing is that we're\n*already* encoding this information, so to me at least the logical end\ngame to changes like what you're proposing is something like the above,\notherwise why bother?\n\nBut having thought about it a bit more I wonder if\ngit-interpret-trailers (or some command like it) shouldn't just as a\nspecial-case learn to do more parsing of commit messages we've\nhistorically generated.\n\nE.g. know how to parse out a merge message we've produced and spew out\nsomething like the above Merge-* output. Same for existing \"Revert\"\noutput, if anyone cares about parsing this they'll need to do that\nanyway.\n"},{"id":"362596","messageId":"871s7y6qs9.fsf@evledraar.gmail.com","threadId":"49753","inReplyTo":"20181106171637.15562-1-pclouds@gmail.com","subject":"Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-06T17:48:22Z","receivedAt":"2018-11-06T17:48:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 06 2018, Nguyễn Thái Ngọc Duy wrote:\n\n> OK here is a less constroversal attempt to add new trailers. Instead\n> of changing the default behavior (which could be done incrementally\n> later), this patch simply adds a new option --append-trailer to revert\n> and cherry-pick.\n>\n> Both will show either\n>\n> Reverts: [~]\n>\n> or\n>\n> Cherry-picked-from: [~]\n>\n> --append-trailer could be added to more commands (e.g. merge) that\n> generate commit messages if they have something for machine\n> consumption.\n>\n> After this, perhaps we could have a config key to turn this on by\n> default (for revert; for cherry-pick it will turn off \"-x\" too). Then\n> after a couple releases, the we got good reception, we'll make it\n> default?\n>\n> No tests, no proper commit message since I think we're still going to\n> discuss a bit more before settling down.\n>\n> PS. maybe --append-trailer is too generic? We have --signoff which is\n> also a trailer. But that one carries more weights than just \"machine\n> generated tags\".\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\nThe implementation looks fine to me, but as noted in\nhttps://public-inbox.org/git/8736se6qyc.fsf@evledraar.gmail.com/ I\nwonder what the plausible end-game is. That we'll turn this on by\ndefault in a few years, and then you can only worry about\ngit-interpret-trailers for repos created as of 2020 or something?\n\nOtherwise it seems we'll need to *also* parse out the existing messages\nwe've added.\n"},{"id":"362617","messageId":"20181106221118.GA9975@sigill.intra.peff.net","threadId":"49753","inReplyTo":"871s7y6qs9.fsf@evledraar.gmail.com","subject":"Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-06T22:11:18Z","receivedAt":"2018-11-06T22:11:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 06, 2018 at 06:48:22PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> The implementation looks fine to me, but as noted in\n> https://public-inbox.org/git/8736se6qyc.fsf@evledraar.gmail.com/ I\n> wonder what the plausible end-game is. That we'll turn this on by\n> default in a few years, and then you can only worry about\n> git-interpret-trailers for repos created as of 2020 or something?\n> \n> Otherwise it seems we'll need to *also* parse out the existing messages\n> we've added.\n\nCould we help the reading scripts by normalizing old and new output via\ninterpret-trailers, %(trailers), etc?\n\nI think \"(cherry picked from ...)\" is already considered a trailer by\nthe trailer code. If the caller instructs us to, we could probably\nrewrite it to:\n\n Cherry-picked-from: ...\n\nin the output. Then the end-game is that scripts should just use\ninterpret-trailers, etc, and old and new commits will Just Work.\n\n-Peff\n"},{"id":"362618","messageId":"20181106222912.GA14820@sigill.intra.peff.net","threadId":"49753","inReplyTo":"20181106221118.GA9975@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-06T22:29:12Z","receivedAt":"2018-11-06T22:29:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 06, 2018 at 05:11:18PM -0500, Jeff King wrote:\n\n> On Tue, Nov 06, 2018 at 06:48:22PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> > The implementation looks fine to me, but as noted in\n> > https://public-inbox.org/git/8736se6qyc.fsf@evledraar.gmail.com/ I\n> > wonder what the plausible end-game is. That we'll turn this on by\n> > default in a few years, and then you can only worry about\n> > git-interpret-trailers for repos created as of 2020 or something?\n> > \n> > Otherwise it seems we'll need to *also* parse out the existing messages\n> > we've added.\n> \n> Could we help the reading scripts by normalizing old and new output via\n> interpret-trailers, %(trailers), etc?\n> \n> I think \"(cherry picked from ...)\" is already considered a trailer by\n> the trailer code. If the caller instructs us to, we could probably\n> rewrite it to:\n> \n> Cherry-picked-from: ...\n> \n> in the output. Then the end-game is that scripts should just use\n> interpret-trailers, etc, and old and new commits will Just Work.\n\nI.e., something like this:\n\ndiff --git a/trailer.c b/trailer.c\nindex 0796f326b3..491c7c240e 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -46,9 +46,11 @@ static int configured;\n \n #define TRAILER_ARG_STRING \"$ARG\"\n \n+#define CHERRY_PICK_PREFIX \"(cherry picked from commit \"\n+\n static const char *git_generated_prefixes[] = {\n \t\"Signed-off-by: \",\n-\t\"(cherry picked from commit \",\n+\tCHERRY_PICK_PREFIX,\n \tNULL\n };\n \n@@ -1141,6 +1143,8 @@ static void format_trailer_info(struct strbuf *out,\n \tfor (i = 0; i < info->trailer_nr; i++) {\n \t\tchar *trailer = info->trailers[i];\n \t\tssize_t separator_pos = find_separator(trailer, separators);\n+\t\tconst char *hex;\n+\t\tstruct object_id oid;\n \n \t\tif (separator_pos >= 1) {\n \t\t\tstruct strbuf tok = STRBUF_INIT;\n@@ -1154,6 +1158,12 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tstrbuf_release(&tok);\n \t\t\tstrbuf_release(&val);\n \n+\t\t} else if (/* should check opts->normalize_cherry_pick */1 &&\n+\t\t\t skip_prefix(trailer, CHERRY_PICK_PREFIX, &hex) &&\n+\t\t\t !get_oid_hex(hex, &oid)) {\n+\t\t\tstrbuf_addf(out, \"Cherry-picked-from: %s\\n\",\n+\t\t\t\t oid_to_hex(&oid));\n+\n \t\t} else if (!opts->only_trailers) {\n \t\t\tstrbuf_addstr(out, trailer);\n \t\t}\n\nThat would need to add the normalize_cherry_pick flag to the options\nstruct, and then plumb it through. But it's enough to play around with,\nand:\n\n git log --format='%h%n%(trailers:only)'\n\nworks.\n\n-Peff\n"},{"id":"362620","messageId":"xmqq4lctenjo.fsf@gitster-ct.c.googlers.com","threadId":"49753","inReplyTo":"20181106171637.15562-1-pclouds@gmail.com","subject":"Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-07T00:31:07Z","receivedAt":"2018-11-07T00:31:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> OK here is a less constroversal attempt to add new trailers. Instead\n> of changing the default behavior (which could be done incrementally\n> later), this patch simply adds a new option --append-trailer to revert\n> and cherry-pick.\n\nI almost agree, except that the the textual description in a revert\nand the funny-looking trailer-wannabe in a cherry-pick are two\nfundamentally different things, and adding duplicate to the latter\ndoes not make much sense, while keeping both does make sense for the\nformer.\n\n> PS. maybe --append-trailer is too generic? We have --signoff which is\n> also a trailer. But that one carries more weights than just \"machine\n> generated tags\".\n\n> +\t\tif (opts->append_trailer) {\n> +\t\t\tstrbuf_addstr(&msgbuf, \"\\n\");\n> +\t\t\tif (parent_id != -1)\n> +\t\t\t\tstrbuf_addf(&msgbuf, \"Reverts: %s~%d\\n\",\n> +\t\t\t\t\t oid_to_hex(&commit->object.oid),\n> +\t\t\t\t\t parent_id);\n> +\t\t\telse\n> +\t\t\t\tstrbuf_addf(&msgbuf, \"Reverts: %s\\n\",\n> +\t\t\t\t\t oid_to_hex(&commit->object.oid));\n> +\t\t}\n\nMakes sense, I guess.\n\n> @@ -1780,14 +1792,28 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n> \t\tif (find_commit_subject(msg.message, &p))\n> \t\t\tstrbuf_addstr(&msgbuf, p);\n> \n> -\t\tif (opts->record_origin) {\n> +\t\tif (opts->record_origin || opts->append_trailer) {\n> \t\t\tstrbuf_complete_line(&msgbuf);\n> \t\t\tif (!has_conforming_footer(&msgbuf, NULL, 0))\n> \t\t\t\tstrbuf_addch(&msgbuf, '\\n');\n> +\t\t}\n> +\n> +\t\tif (opts->record_origin) {\n> \t\t\tstrbuf_addstr(&msgbuf, cherry_picked_prefix);\n> \t\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n> \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n> \t\t}\n> +\t\tif (opts->append_trailer) {\n\nThese should be either-or, I would think, as adding exactly the same\npiece of information in two different machine-readable form does not\nmake much sense. The command line parser may even want to make sure\nthat both are not given at the same time.\n\nAlternatively, we can keep using -x as \"we are going to record where\nthe change was cherry-picked from\" and use .record_origin to store\nthat bit, and use the new .append_trailer bit as \"if we are recording\nthe origin, in which format between the old and the new do we do\nso?\" bit.\n\nI think the latter makes more sense, at least to me.\n\n\n> +\t\t\tif (opts->record_origin)\n> +\t\t\t\tstrbuf_addstr(&msgbuf, \"\\n\");\n> +\t\t\tif (parent_id != -1)\n> +\t\t\t\tstrbuf_addf(&msgbuf, \"Cherry-picked-from: %s~%d\\n\",\n> +\t\t\t\t\t oid_to_hex(&commit->object.oid),\n> +\t\t\t\t\t parent_id);\n> +\t\t\telse\n> +\t\t\t\tstrbuf_addf(&msgbuf, \"Cherry-picked-from: %s\\n\",\n> +\t\t\t\t\t oid_to_hex(&commit->object.oid));\n> +\t\t}\n"},{"id":"362621","messageId":"xmqqzhuld8gq.fsf@gitster-ct.c.googlers.com","threadId":"49753","inReplyTo":"20181106221118.GA9975@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-07T00:42:13Z","receivedAt":"2018-11-07T00:42:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Could we help the reading scripts by normalizing old and new output via\n> interpret-trailers, %(trailers), etc?\n>\n> I think \"(cherry picked from ...)\" is already considered a trailer by\n> the trailer code.\n\n;-) \n\nGreat minds think alike, I guess. I think it is a great idea\nto ask interpret-trailers to help us here.\n"},{"id":"362665","messageId":"CACsJy8DTdW-P9zG3DBPArjhZU1VwwBmd7qNibyHxYkyjRrdWmw@mail.gmail.com","threadId":"49753","inReplyTo":"20181106221118.GA9975@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-07T15:30:38Z","receivedAt":"2018-11-07T15:31:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Nov 6, 2018 at 11:11 PM Jeff King wrote:\n>\n> On Tue, Nov 06, 2018 at 06:48:22PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n> > The implementation looks fine to me, but as noted in\n> > https://public-inbox.org/git/8736se6qyc.fsf@evledraar.gmail.com/ I\n> > wonder what the plausible end-game is. That we'll turn this on by\n> > default in a few years, and then you can only worry about\n> > git-interpret-trailers for repos created as of 2020 or something?\n> >\n> > Otherwise it seems we'll need to *also* parse out the existing messages\n> > we've added.\n>\n> Could we help the reading scripts by normalizing old and new output via\n> interpret-trailers, %(trailers), etc?\n>\n> I think \"(cherry picked from ...)\" is already considered a trailer by\n> the trailer code. If the caller instructs us to, we could probably\n> rewrite it to:\n>\n> Cherry-picked-from: ...\n>\n> in the output. Then the end-game is that scripts should just use\n> interpret-trailers, etc, and old and new commits will Just Work.\n\nThere is still one thing to settle. \"revert -m1\" could produce\nsomething like this\n\n This reverts commit , reversing\n changes made to .\n\nMy proposal produces this\n\n Reverts: ^2\n\nAnd I can't really convert the former to latter without accessing\nobject database (probably not a good idea?) to check if SHA2 is the\nsecond parent of SHA1. So either\n\n - I access object database anyway\n - Generate just \"Reverts: \" (i.e. losing info) with interpret-trailers\n - Change Reverts: tag to a different output format, or maybe use two\ntags instead.\n-- \nDuy\n"},{"id":"362672","messageId":"20181107210256.GC30078@sigill.intra.peff.net","threadId":"49753","inReplyTo":"CACsJy8DTdW-P9zG3DBPArjhZU1VwwBmd7qNibyHxYkyjRrdWmw@mail.gmail.com","subject":"Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-07T21:02:56Z","receivedAt":"2018-11-07T21:03:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 07, 2018 at 04:30:38PM +0100, Duy Nguyen wrote:\n\n> > Could we help the reading scripts by normalizing old and new output via\n> > interpret-trailers, %(trailers), etc?\n> >\n> > I think \"(cherry picked from ...)\" is already considered a trailer by\n> > the trailer code. If the caller instructs us to, we could probably\n> > rewrite it to:\n> >\n> > Cherry-picked-from: ...\n> >\n> > in the output. Then the end-game is that scripts should just use\n> > interpret-trailers, etc, and old and new commits will Just Work.\n> \n> There is still one thing to settle. \"revert -m1\" could produce\n> something like this\n> \n> This reverts commit , reversing\n> changes made to .\n> \n> My proposal produces this\n> \n> Reverts: ^2\n> \n> And I can't really convert the former to latter without accessing\n> object database (probably not a good idea?) to check if SHA2 is the\n> second parent of SHA1. So either\n> \n> - I access object database anyway\n> - Generate just \"Reverts: \" (i.e. losing info) with interpret-trailers\n> - Change Reverts: tag to a different output format, or maybe use two\n> tags instead.\n\nIMHO the revert case is way less interesting for automated parsing. In a\nworkflow like Git's, cherry-picks aren't very common, but there _are_\nworkflows where there's a lot of cherry-picking between dev/release\nbranches, and automated analysis is useful there. Whereas for revert,\nit's almost always a human-scale thing. A commit was bad, so you revert\nit. The annotation is useful if you're digging, but it's not generally\ngoing to be a fundamental part of a workflow. And it's not really any\ndifferent than fixing a bug later.\n\nAnd I think that's reflected in the way we just casually stick the\nreverted oid in the human-readable part of the commit message (and the\nlack of any tools to parse it).\n\nSo IMHO it would be OK to treat this less carefully than the cherry-pick\ncase.\n\n-Peff\n"},{"id":"362683","messageId":"xmqqh8gs9zh3.fsf@gitster-ct.c.googlers.com","threadId":"49753","inReplyTo":"CACsJy8DTdW-P9zG3DBPArjhZU1VwwBmd7qNibyHxYkyjRrdWmw@mail.gmail.com","subject":"Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-08T00:36:56Z","receivedAt":"2018-11-08T00:37:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> There is still one thing to settle. \"revert -m1\" could produce\n> something like this\n>\n> This reverts commit , reversing\n> changes made to .\n\nI do not think it is relevant, with or without multiple parents, to\neven attempt to read this message.\n\nThe description is not meant to be machine readable/parseable, but\nis meant to be updated to describe the reason why the reversion was\nmade for human readers. Spending any cycle to attempt interpreting\nit by machines will give a wrong signal to encourage people not to\ntouch it. Instead we should actively encourage people to take that\nas the beginning of their description.\n\nI even suspect that an update to that message to read something like\nthese\n\n\t\"This reverts commit because FILL IN THE REASONS HERE\"\n\n\t\"This reverts commit , reversing changes made to\n\t , because FILL IN THE REASONS HERE\"\n\nwould be a good idea. It of course is orthogonal to the topic of\nintroducing a new footer to record the \"what happened\" (without the\n\"why\") in a machine-readable way.\n\n"},{"id":"362688","messageId":"20181108012901.GB10148@sigill.intra.peff.net","threadId":"49753","inReplyTo":"xmqqh8gs9zh3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-08T01:29:01Z","receivedAt":"2018-11-08T01:29:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 08, 2018 at 09:36:56AM +0900, Junio C Hamano wrote:\n\n> Duy Nguyen writes:\n> \n> > There is still one thing to settle. \"revert -m1\" could produce\n> > something like this\n> >\n> > This reverts commit , reversing\n> > changes made to .\n> \n> I do not think it is relevant, with or without multiple parents, to\n> even attempt to read this message.\n> \n> The description is not meant to be machine readable/parseable, but\n> is meant to be updated to describe the reason why the reversion was\n> made for human readers. Spending any cycle to attempt interpreting\n> it by machines will give a wrong signal to encourage people not to\n> touch it. Instead we should actively encourage people to take that\n> as the beginning of their description.\n> \n> I even suspect that an update to that message to read something like\n> these\n> \n> \t\"This reverts commit because FILL IN THE REASONS HERE\"\n> \n> \t\"This reverts commit , reversing changes made to\n> \t , because FILL IN THE REASONS HERE\"\n\nYeah, that was the point I was trying to make earlier today in this\nthread. I really think of this as a human-readable message.\n\nBut as far as how this impacts the addition of a trailer: once we have a\nmachine-readable \"Reverts:\", people naturally may want to know about\n\"Reverts\" for older commits. Our options are:\n\n 1. say \"sorry, there was no machine-readable version back then\"\n\n 2. try to parse our \"This reverts\" message and normalize it into\n \"Reverts\" for their benefit.\n\nBefore introducing the new footer, it's worth thinking about the\nend-game here. If we do (1), then people may want to parse themselves.\nThey are stuck parsing both the old and new (to handle old and new\ncommits). We could make life a little easier on them if we included\n_both_ the English text and the machine-readable version. And then if\nthey just chose to parse the English, it would work for old and new.\n\nBut I guess that is really just a losing battle, if we consider the\nEnglish to be changeable. And doing it ourselves in (2) is really just\npushing that losing battle onto ourselves.\n\nSo if we are comfortable with saying that this is a new feature to have\nthe machine-readable trailer version, and there isn't a robust way to\nget historical revert information (because there really isn't[1]), then\nI think we can just punt on any kind of trailer-normalization magic.\n\n-Peff\n\n[1] Thinking back on reverts I have done, they are often _not_\n straight-up reverts. For example, I may end up dropping half of a\n commit, but leaving some traces of it in place in order to build up\n the correct solution on top (i.e., fixing whatever problem caused me\n to revert in the first place). I list those as \"this is morally a\n revert of 1234abcd...\", which is definitely not machine readable. ;)\n"},{"id":"362696","messageId":"xmqqh8gs8coe.fsf@gitster-ct.c.googlers.com","threadId":"49753","inReplyTo":"20181108012901.GB10148@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-08T03:34:41Z","receivedAt":"2018-11-08T03:34:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So if we are comfortable with saying that this is a new feature to have\n> the machine-readable trailer version, and there isn't a robust way to\n> get historical revert information (because there really isn't[1]), then\n> I think we can just punt on any kind of trailer-normalization magic.\n\nYes, I do consider that the original suggestion was two-part\n\n - cherry-pick did have machine readable info, but by historical\n accident, it is shaped differently from \"trailers\", so we'd\n transition into the new format.\n\n - revert did not have machine readble info at all, so we are adding\n one, even though it is not that interesting as cherry-pick (for\n the reasons you stated in an earlier message in this thread).\n\nSo my \"honest answer\" is your #1, \"sorry, there was no\nmachine-readable version back then\", for reverts. We do not have\nsuch a problem with cherry-pick luckily ;-)\n\n> [1] Thinking back on reverts I have done, they are often _not_\n> straight-up reverts. For example, I may end up dropping half of a\n> commit, but leaving some traces of it in place in order to build up\n> the correct solution on top (i.e., fixing whatever problem caused me\n> to revert in the first place). I list those as \"this is morally a\n> revert of 1234abcd...\", which is definitely not machine readable. ;)\n\nYup, and it is debatable if it even makes sense to add the machine\nreadable trailer for such a commit. A human-made claim that it is a\n\"moral equivalent of reverting X\" may not look any different from a\n\"textual revert of X\" to a machine, but the actual patch text would\nbe quite different---unless the machine reading it understands\n\"moral equivalence\", letting it blindly take on faith whatever\nhumans say may not be a good idea.\n"}]}