{"thread":{"id":"57910","subject":"[PATCH] revert: optionally refer to commit in the \"reference\" format","startedAt":"2022-05-22T04:33:17Z","lastAt":"2022-06-27T15:38:26Z","messageCount":20,"participants":["Junio C Hamano","Johannes Schindelin","Ævar Arnfjörð Bjarmason","Phillip Wood","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"455728","messageId":"xmqqsfp2b30k.fsf@gitster.g","threadId":"57910","inReplyTo":null,"subject":"[PATCH] revert: optionally refer to commit in the \"reference\" format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-22T04:32:59Z","receivedAt":"2022-05-22T04:33:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A typical \"git revert\" commit uses the full title of the original\ncommit in its title, and starts its body of the message with:\n\n This reverts commit 8fa7f667cf61386257c00d6e954855cc3215ae91.\n\nThis does not encourage the best practice of describing not just\n\"what\" (i.e. \"Revert X\" on the title says what we did) but \"why\"\n(i.e. and it does not say why X was undesirable).\n\nWe can instead phrase this first line of the body to be more like\n\n This reverts commit 8fa7f667 (do this and that, 2022-04-25)\n\nso that the title does not have to be\n\n Revert \"do this and that\"\n\nWe can instead use the title to describe \"why\" we are reverting the\noriginal commit.\n\nIntroduce the \"--reference\" option to \"git revert\", and also the\nrevert.reference configuration variable, which defaults to false, to\ntweak the title and the first line of the draft commit message for\nwhen creating a \"revert\" commit.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/config/revert.txt | 3 +++\n Documentation/git-revert.txt | 9 +++++++++\n builtin/revert.c | 2 ++\n sequencer.c | 32 +++++++++++++++++++++++++++-----\n sequencer.h | 1 +\n t/t3501-revert-cherry-pick.sh | 31 +++++++++++++++++++++++++++++++\n 6 files changed, 73 insertions(+), 5 deletions(-)\n create mode 100644 Documentation/config/revert.txt\n\ndiff --git a/Documentation/config/revert.txt b/Documentation/config/revert.txt\nnew file mode 100644\nindex 0000000000..797bfb6d62\n--- /dev/null\n+++ b/Documentation/config/revert.txt\n@@ -0,0 +1,3 @@\n+revert.reference::\n+\tSetting this variable to true makes `git revert` to behave\n+\tas if the `--reference` option is given.\ndiff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt\nindex bb92a4a451..8463fe9cf7 100644\n--- a/Documentation/git-revert.txt\n+++ b/Documentation/git-revert.txt\n@@ -117,6 +117,15 @@ effect to your index in a row.\n \tAllow the rerere mechanism to update the index with the\n \tresult of auto-conflict resolution if possible.\n \n+--reference::\n+\tInstead of starting the body of the log message with \"This\n+\treverts .\",\n+\trefer to the commit using \"--pretty=reference\" format\n+\t(cf. linkgit:git-log[1]). The `revert.reference`\n+\tconfiguration variable can be used to enable this option by\n+\tdefault.\n+\n+\n SEQUENCER SUBCOMMANDS\n ---------------------\n include::sequencer.txt[]\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 51776abea6..ada51e46b9 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -116,6 +116,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\tN_(\"option for merge strategy\"), option_parse_x),\n \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n \t\t N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n+\t\tOPT_BOOL(0, \"reference\", &opts->commit_use_reference,\n+\t\t\t N_(\"use the 'reference' format to refer to commits\")),\n \t\tOPT_END()\n \t};\n \tstruct option *options = base_options;\ndiff --git a/sequencer.c b/sequencer.c\nindex a5f678f452..97abd5932b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -221,6 +221,9 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n \t\treturn ret;\n \t}\n \n+\tif (!strcmp(k, \"revert.reference\"))\n+\t\topts->commit_use_reference = git_config_bool(k, v);\n+\n \tstatus = git_gpg_config(k, v, NULL);\n \tif (status)\n \t\treturn status;\n@@ -2059,6 +2062,20 @@ static int should_edit(struct replay_opts *opts) {\n \treturn opts->edit;\n }\n \n+static void refer_to_commit(struct replay_opts *opts,\n+\t\t\t struct strbuf *msgbuf, struct commit *commit)\n+{\n+\tif (opts->commit_use_reference) {\n+\t\tstruct pretty_print_context ctx = {\n+\t\t\t.abbrev = DEFAULT_ABBREV,\n+\t\t\t.date_mode.type = DATE_SHORT,\n+\t\t};\n+\t\tformat_commit_message(commit, \"%h (%s, %ad)\", msgbuf, &ctx);\n+\t} else {\n+\t\tstrbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));\n+\t}\n+}\n+\n static int do_pick_commit(struct repository *r,\n \t\t\t struct todo_item *item,\n \t\t\t struct replay_opts *opts,\n@@ -2167,14 +2184,19 @@ static int do_pick_commit(struct repository *r,\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+\t\tif (!opts->commit_use_reference) {\n+\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n+\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n+\t\t\tstrbuf_addstr(&msgbuf, \"\\\"\");\n+\t\t} else {\n+\t\t\tstrbuf_addstr(&msgbuf, \"DESCRIBE WHY WE ARE REVERTING HERE\");\n+\t\t}\n+\t\tstrbuf_addstr(&msgbuf, \"\\n\\nThis reverts commit \");\n+\t\trefer_to_commit(opts, &msgbuf, commit);\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\trefer_to_commit(opts, &msgbuf, parent);\n \t\t}\n \t\tstrbuf_addstr(&msgbuf, \".\\n\");\n \t} else {\ndiff --git a/sequencer.h b/sequencer.h\nindex da64473636..698599fe4e 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -49,6 +49,7 @@ struct replay_opts {\n \tint reschedule_failed_exec;\n \tint committer_date_is_author_date;\n \tint ignore_date;\n+\tint commit_use_reference;\n \n \tint mainline;\n \ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex 8617efaaf1..36f9565b92 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -159,6 +159,7 @@ test_expect_success 'cherry-pick works with dirty renamed file' '\n '\n \n test_expect_success 'advice from failed revert' '\n+\ttest_when_finished \"git reset --hard\" &&\n \ttest_commit --no-tag \"add dream\" dream dream &&\n \tdream_oid=$(git rev-parse --short HEAD) &&\n \tcat <<-EOF >expected &&\n@@ -174,4 +175,34 @@ test_expect_success 'advice from failed revert' '\n \ttest_must_fail git revert HEAD^ 2>actual &&\n \ttest_cmp expected actual\n '\n+\n+test_expect_success 'identification of reverted commit (vanilla)' '\n+\ttest_commit to-ident &&\n+\ttest_when_finished \"git reset --hard to-ident\" &&\n+\tgit checkout --detach to-ident &&\n+\tgit revert --no-edit HEAD &&\n+\tgit cat-file commit HEAD >actual.raw &&\n+\tgrep \"^This reverts \" actual.raw >actual &&\n+\techo \"This reverts commit $(git rev-parse HEAD^).\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'identification of reverted commit (reference)' '\n+\tgit checkout --detach to-ident &&\n+\tgit revert --reference --no-edit HEAD &&\n+\tgit cat-file commit HEAD >actual.raw &&\n+\tgrep \"^This reverts \" actual.raw >actual &&\n+\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'identification of reverted commit (reference)' '\n+\tgit checkout --detach to-ident &&\n+\tgit -c revert.reference=true revert --no-edit HEAD &&\n+\tgit cat-file commit HEAD >actual.raw &&\n+\tgrep \"^This reverts \" actual.raw >actual &&\n+\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.36.1-298-g81715a4e6d\n\n"},{"id":"455769","messageId":"nycvar.QRO.7.76.6.2205231507350.352@tvgsbejvaqbjf.bet","threadId":"57910","inReplyTo":"xmqqsfp2b30k.fsf@gitster.g","subject":"Re: [PATCH] revert: optionally refer to commit in the \"reference\" format","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-05-23T13:25:18Z","receivedAt":"2022-05-23T13:25:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sat, 21 May 2022, Junio C Hamano wrote:\n\n> A typical \"git revert\" commit uses the full title of the original\n> commit in its title, and starts its body of the message with:\n>\n> This reverts commit 8fa7f667cf61386257c00d6e954855cc3215ae91.\n>\n> This does not encourage the best practice of describing not just\n> \"what\" (i.e. \"Revert X\" on the title says what we did) but \"why\"\n> (i.e. and it does not say why X was undesirable).\n>\n> We can instead phrase this first line of the body to be more like\n>\n> This reverts commit 8fa7f667 (do this and that, 2022-04-25)\n>\n> so that the title does not have to be\n>\n> Revert \"do this and that\"\n>\n> We can instead use the title to describe \"why\" we are reverting the\n> original commit.\n\nGood idea, the `reference` text is so much more pleasant to read (and it\nalso makes things somewhat proof against rebasing and transitioning to\nSHA-256).\n\n> Introduce the \"--reference\" option to \"git revert\", and also the\n> revert.reference configuration variable, which defaults to false, to\n> tweak the title and the first line of the draft commit message for\n> when creating a \"revert\" commit.\n\nAt some stage, we may even default to `revert.reference=true`?\n\n> diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt\n> index bb92a4a451..8463fe9cf7 100644\n> --- a/Documentation/git-revert.txt\n> +++ b/Documentation/git-revert.txt\n> @@ -117,6 +117,15 @@ effect to your index in a row.\n> \tAllow the rerere mechanism to update the index with the\n> \tresult of auto-conflict resolution if possible.\n>\n> +--reference::\n> +\tInstead of starting the body of the log message with \"This\n> +\treverts .\",\n> +\trefer to the commit using \"--pretty=reference\" format\n> +\t(cf. linkgit:git-log[1]). The `revert.reference`\n> +\tconfiguration variable can be used to enable this option by\n> +\tdefault.\n> +\n> +\n\nIs that extra empty line intentional?\n\n> SEQUENCER SUBCOMMANDS\n> ---------------------\n> include::sequencer.txt[]\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 51776abea6..ada51e46b9 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -116,6 +116,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> \t\t\tN_(\"option for merge strategy\"), option_parse_x),\n> \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n> \t\t N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n> +\t\tOPT_BOOL(0, \"reference\", &opts->commit_use_reference,\n> +\t\t\t N_(\"use the 'reference' format to refer to commits\")),\n> \t\tOPT_END()\n> \t};\n> \tstruct option *options = base_options;\n> diff --git a/sequencer.c b/sequencer.c\n> index a5f678f452..97abd5932b 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -221,6 +221,9 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n> \t\treturn ret;\n> \t}\n>\n> +\tif (!strcmp(k, \"revert.reference\"))\n> +\t\topts->commit_use_reference = git_config_bool(k, v);\n> +\n> \tstatus = git_gpg_config(k, v, NULL);\n> \tif (status)\n> \t\treturn status;\n> @@ -2059,6 +2062,20 @@ static int should_edit(struct replay_opts *opts) {\n> \treturn opts->edit;\n> }\n>\n> +static void refer_to_commit(struct replay_opts *opts,\n> +\t\t\t struct strbuf *msgbuf, struct commit *commit)\n> +{\n> +\tif (opts->commit_use_reference) {\n> +\t\tstruct pretty_print_context ctx = {\n> +\t\t\t.abbrev = DEFAULT_ABBREV,\n> +\t\t\t.date_mode.type = DATE_SHORT,\n> +\t\t};\n> +\t\tformat_commit_message(commit, \"%h (%s, %ad)\", msgbuf, &ctx);\n\nIt is somewhat sad that we have to semi-duplicate [*1*] this format here\nand do not have something like an enum value we can use instead.\n\n> +\t} else {\n> +\t\tstrbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));\n> +\t}\n> +}\n> +\n> static int do_pick_commit(struct repository *r,\n> \t\t\t struct todo_item *item,\n> \t\t\t struct replay_opts *opts,\n> @@ -2167,14 +2184,19 @@ static int do_pick_commit(struct repository *r,\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> +\t\tif (!opts->commit_use_reference) {\n> +\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n> +\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n> +\t\t\tstrbuf_addstr(&msgbuf, \"\\\"\");\n> +\t\t} else {\n> +\t\t\tstrbuf_addstr(&msgbuf, \"DESCRIBE WHY WE ARE REVERTING HERE\");\n> +\t\t}\n\nWe often tell newcomers that we prefer the shorter conditional block to\ncome first. Maybe you want to do that here, too? I don't mind the current\nform, though.\n\n> +\t\tstrbuf_addstr(&msgbuf, \"\\n\\nThis reverts commit \");\n> +\t\trefer_to_commit(opts, &msgbuf, commit);\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\trefer_to_commit(opts, &msgbuf, parent);\n> \t\t}\n> \t\tstrbuf_addstr(&msgbuf, \".\\n\");\n> \t} else {\n> diff --git a/sequencer.h b/sequencer.h\n> index da64473636..698599fe4e 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -49,6 +49,7 @@ struct replay_opts {\n> \tint reschedule_failed_exec;\n> \tint committer_date_is_author_date;\n> \tint ignore_date;\n> +\tint commit_use_reference;\n>\n> \tint mainline;\n>\n> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\n> index 8617efaaf1..36f9565b92 100755\n> --- a/t/t3501-revert-cherry-pick.sh\n> +++ b/t/t3501-revert-cherry-pick.sh\n> @@ -159,6 +159,7 @@ test_expect_success 'cherry-pick works with dirty renamed file' '\n> '\n>\n> test_expect_success 'advice from failed revert' '\n> +\ttest_when_finished \"git reset --hard\" &&\n> \ttest_commit --no-tag \"add dream\" dream dream &&\n> \tdream_oid=$(git rev-parse --short HEAD) &&\n> \tcat <<-EOF >expected &&\n> @@ -174,4 +175,34 @@ test_expect_success 'advice from failed revert' '\n> \ttest_must_fail git revert HEAD^ 2>actual &&\n> \ttest_cmp expected actual\n> '\n> +\n> +test_expect_success 'identification of reverted commit (vanilla)' '\n\nIt might make sense to replace \"vanilla\" by \"default format\".\n\n> +\ttest_commit to-ident &&\n> +\ttest_when_finished \"git reset --hard to-ident\" &&\n> +\tgit checkout --detach to-ident &&\n> +\tgit revert --no-edit HEAD &&\n> +\tgit cat-file commit HEAD >actual.raw &&\n> +\tgrep \"^This reverts \" actual.raw >actual &&\n> +\techo \"This reverts commit $(git rev-parse HEAD^).\" >expect &&\n\nI was a bit puzzled about this dance, as I had expected this instead:\n\n\tgrep \"^This reverts commit $(git rev-parse HEAD^)\\\\.$\" actual.raw\n\nbut I guess you wanted to make sure that the file contents of `actual` are\nshown if no match was found?\n\nIf that was the intention, we may want to add a test helper that I know\nfrom other test frameworks, something like `test_contains` that takes a\nregex and a file and prints out the full contents if no match was found.\n\nIt's obviously not necessary to add this test helper in this patch series\nof a single patch (\"separation of concerns\"), just food for thought.\n\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'identification of reverted commit (reference)' '\n> +\tgit checkout --detach to-ident &&\n> +\tgit revert --reference --no-edit HEAD &&\n> +\tgit cat-file commit HEAD >actual.raw &&\n> +\tgrep \"^This reverts \" actual.raw >actual &&\n> +\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n\nIf it was up to me, I would combine these three test cases, if only to\nhelp the `--run=` case (the latter two depend on the side\neffect of the first one to create a `to-ident` tag).\n\n> +\n> +test_expect_success 'identification of reverted commit (reference)' '\n> +\tgit checkout --detach to-ident &&\n> +\tgit -c revert.reference=true revert --no-edit HEAD &&\n> +\tgit cat-file commit HEAD >actual.raw &&\n> +\tgrep \"^This reverts \" actual.raw >actual &&\n> +\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n> --\n> 2.36.1-298-g81715a4e6d\n\nLooks good, I have no objections against merging this as-is.\n\nCiao,\nDscho\n\nFootnote *1*: https://github.com/git/git/blob/v2.36.1/pretty.c#L103-L104\nalso has the `%C(auto)` prefix, which we obviously do not need here.\n"},{"id":"455782","messageId":"220523.86wnecxqol.gmgdl@evledraar.gmail.com","threadId":"57910","inReplyTo":"xmqqsfp2b30k.fsf@gitster.g","subject":"Re: [PATCH] revert: optionally refer to commit in the \"reference\" format","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-23T13:45:26Z","receivedAt":"2022-05-23T14:35:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, May 21 2022, Junio C Hamano wrote:\n\n> A typical \"git revert\" commit uses the full title of the original\n> commit in its title, and starts its body of the message with:\n>\n> This reverts commit 8fa7f667cf61386257c00d6e954855cc3215ae91.\n>\n> This does not encourage the best practice of describing not just\n> \"what\" (i.e. \"Revert X\" on the title says what we did) but \"why\"\n> (i.e. and it does not say why X was undesirable).\n>\n> We can instead phrase this first line of the body to be more like\n>\n> This reverts commit 8fa7f667 (do this and that, 2022-04-25)\n>\n> so that the title does not have to be\n>\n> Revert \"do this and that\"\n>\n> We can instead use the title to describe \"why\" we are reverting the\n> original commit.\n>\n> Introduce the \"--reference\" option to \"git revert\", and also the\n> revert.reference configuration variable, which defaults to false, to\n> tweak the title and the first line of the draft commit message for\n> when creating a \"revert\" commit.\n\nThis is fantastic, I've wanted to fix this particular nit for a long\ntime.\n\n> +static void refer_to_commit(struct replay_opts *opts,\n> +\t\t\t struct strbuf *msgbuf, struct commit *commit)\n> +{\n> +\tif (opts->commit_use_reference) {\n> +\t\tstruct pretty_print_context ctx = {\n> +\t\t\t.abbrev = DEFAULT_ABBREV,\n> +\t\t\t.date_mode.type = DATE_SHORT,\n> +\t\t};\n> +\t\tformat_commit_message(commit, \"%h (%s, %ad)\", msgbuf, &ctx);\n> +\t} else {\n> +\t\tstrbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));\n> +\t}\n> +}\n> +\n> static int do_pick_commit(struct repository *r,\n> \t\t\t struct todo_item *item,\n> \t\t\t struct replay_opts *opts,\n> @@ -2167,14 +2184,19 @@ static int do_pick_commit(struct repository *r,\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> +\t\tif (!opts->commit_use_reference) {\n> +\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n> +\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n> +\t\t\tstrbuf_addstr(&msgbuf, \"\\\"\");\n> +\t\t} else {\n> +\t\t\tstrbuf_addstr(&msgbuf, \"DESCRIBE WHY WE ARE REVERTING HERE\");\n\nHaving seen how people use git in the wild, this *WILL* result in a\nbunch of history where people don't edit that at all.\n\nIf we want them to edit something we should come up with a buffer that's\nempty except for suggested #-commented parts, and suggest that they\nuncomment/edit some of it.\n\nI also use \"git revert --no-edit\" in my own workflow, e.g. when\nreverting back and forth in WIP code, having the OID is useful, having\nthis new \"reference\" would be even better, but having that just in the\nbody and \"DESCRIBE WHY...\" in the %s would be much less useful.\n\n> +\t\t}\n> +\t\tstrbuf_addstr(&msgbuf, \"\\n\\nThis reverts commit \");\n> +\t\trefer_to_commit(opts, &msgbuf, commit);\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\trefer_to_commit(opts, &msgbuf, parent);\n> \t\t}\n> \t\tstrbuf_addstr(&msgbuf, \".\\n\");\n> \t} else {\n\nThis is very POC, but I wish we had something like the WIP diff at the\nend of this E-Mail (which is a mess, I was experimenting). I.e. we'd\nalways emit (when this setting is enabled)\n\n Revert %h (%s, %ad)\n\nExcept that the %s and the rest of the line would use the truncation\nfeature of \"git log\" formats, and would make sure the line isn't longer\nthan so-and-so, e.g. 72 or 50 characters.\n\nThen only if we truncated things there, or we have parents to mention\nwould we add a body like:\n\n This reverts commit %h (%s, %ad)\n\nFollowed by \"reversing changes...\"\n\nAll of which would be much easier than the below monstrosity I came up\nwith if:\n\n 1. You could ask \"git log\" formats to not whitespace pad the\n truncation, I couldn't find a way to do that (but it's the first\n time I use it).\n\n 2. We just had some generic \"wrap this buffer\" function, maybe we do, I\n didn't look much.\n\n> diff --git a/sequencer.h b/sequencer.h\n> index da64473636..698599fe4e 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -49,6 +49,7 @@ struct replay_opts {\n> \tint reschedule_failed_exec;\n> \tint committer_date_is_author_date;\n> \tint ignore_date;\n> +\tint commit_use_reference;\n> \n> \tint mainline;\n> \n> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\n> index 8617efaaf1..36f9565b92 100755\n> --- a/t/t3501-revert-cherry-pick.sh\n> +++ b/t/t3501-revert-cherry-pick.sh\n> @@ -159,6 +159,7 @@ test_expect_success 'cherry-pick works with dirty renamed file' '\n> '\n> \n> test_expect_success 'advice from failed revert' '\n> +\ttest_when_finished \"git reset --hard\" &&\n> \ttest_commit --no-tag \"add dream\" dream dream &&\n> \tdream_oid=$(git rev-parse --short HEAD) &&\n> \tcat <<-EOF >expected &&\n> @@ -174,4 +175,34 @@ test_expect_success 'advice from failed revert' '\n> \ttest_must_fail git revert HEAD^ 2>actual &&\n> \ttest_cmp expected actual\n> '\n> +\n> +test_expect_success 'identification of reverted commit (vanilla)' '\n> +\ttest_commit to-ident &&\n> +\ttest_when_finished \"git reset --hard to-ident\" &&\n> +\tgit checkout --detach to-ident &&\n> +\tgit revert --no-edit HEAD &&\n> +\tgit cat-file commit HEAD >actual.raw &&\n> +\tgrep \"^This reverts \" actual.raw >actual &&\n> +\techo \"This reverts commit $(git rev-parse HEAD^).\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'identification of reverted commit (reference)' '\n> +\tgit checkout --detach to-ident &&\n> +\tgit revert --reference --no-edit HEAD &&\n> +\tgit cat-file commit HEAD >actual.raw &&\n> +\tgrep \"^This reverts \" actual.raw >actual &&\n> +\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n\nThis hides the exit code of the embedded $() git command.\n\nRe the suggested changes below: let's just test_cmp the whole thing,\nsubject+body instead?\n\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'identification of reverted commit (reference)' '\n> +\tgit checkout --detach to-ident &&\n> +\tgit -c revert.reference=true revert --no-edit HEAD &&\n> +\tgit cat-file commit HEAD >actual.raw &&\n> +\tgrep \"^This reverts \" actual.raw >actual &&\n> +\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 4abb77add09..a43444e2339 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2061,18 +2061,14 @@ static int should_edit(struct replay_opts *opts) {\n \treturn opts->edit;\n }\n \n-static void refer_to_commit(struct replay_opts *opts,\n-\t\t\t struct strbuf *msgbuf, struct commit *commit)\n-{\n-\tif (opts->commit_use_reference) {\n-\t\tstruct pretty_print_context ctx = {\n-\t\t\t.abbrev = DEFAULT_ABBREV,\n-\t\t\t.date_mode.type = DATE_SHORT,\n-\t\t};\n-\t\tformat_commit_message(commit, \"%h (%s, %ad)\", msgbuf, &ctx);\n-\t} else {\n-\t\tstrbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));\n-\t}\n+static void refer_to_commit(struct strbuf *sb, struct commit *commit,\n+\t\t\t const char *const fmt)\n+{\n+\tstruct pretty_print_context ctx = {\n+\t\t.abbrev = DEFAULT_ABBREV,\n+\t\t.date_mode.type = DATE_SHORT,\n+\t};\n+\tformat_commit_message(commit, fmt, sb, &ctx);\n }\n \n static int do_pick_commit(struct repository *r,\n@@ -2183,19 +2179,63 @@ static int do_pick_commit(struct repository *r,\n \t\tbase_label = msg.label;\n \t\tnext = parent;\n \t\tnext_label = msg.parent_label;\n-\t\tif (!opts->commit_use_reference) {\n+\t\tif (opts->commit_use_reference) {\n+\t\t\tstruct strbuf sb_b = STRBUF_INIT;\n+\t\t\tstruct strbuf sb_h = STRBUF_INIT;\n+\t\t\tstruct strbuf sb_ad = STRBUF_INIT;\n+\t\t\tstruct strbuf sb_s = STRBUF_INIT;\n+\t\t\tstruct strbuf sb_s_t = STRBUF_INIT;\n+\t\t\tconst size_t wraplen = 72;\n+\t\t\tchar *fmt;\n+\t\t\tsize_t len = wraplen;\n+\t\t\tint truncated;\n+\n+\t\t\trefer_to_commit(&sb_h, commit, \"Revert %h (\");\n+\t\t\tlen -= sb_h.len;\n+\t\t\trefer_to_commit(&sb_ad, commit, \"%ad)\");\n+\t\t\tlen -= sb_ad.len;\n+\t\t\tlen -= 2 ; /* The \", \" in: Revert %h (%s, %ad) */\n+\t\t\t/* vanilla %h (%s, %ad) */\n+\t\t\trefer_to_commit(&sb_s, commit, \"%s\");\n+\n+\t\t\t/* truncated %h (%s, %ad) */\n+\t\t\tfmt = xstrfmt(\"%%<(%d,trunc)%%s\", (int)len);\n+\t\t\trefer_to_commit(&sb_s_t, commit, fmt);\n+\t\t\tfree(fmt);\n+\t\t\tstrbuf_rtrim(&sb_s_t);\n+\n+\t\t\ttruncated = strcmp(sb_s.buf, sb_s_t.buf);\n+\n+\t\t\tstrbuf_addbuf(&msgbuf, &sb_h);\n+\t\t\tstrbuf_addbuf(&msgbuf, &sb_s_t);\n+\t\t\tstrbuf_addbuf(&msgbuf, &sb_ad);\n+\t\t\tstrbuf_addstr(&msgbuf, \"\\n\\n\");\n+\n+\t\t\tif (truncated) {\n+\t\t\t\tconst char *pfx = \"This reverts commit \";\n+\t\t\t\tsize_t len = wraplen - strlen(pfx); /* TODO: Consider %h etc. */\n+\t\t\t\tchar *fmt;\n+\t\t\t\t\n+\t\t\t\tfmt = xstrfmt(\"%s%%h (%%w(%d,0,0)%%s, %%ad)\",\n+\t\t\t\t\t pfx, (int)len);\n+\t\t\t\trefer_to_commit(&sb_b, commit, fmt);\n+\t\t\t\tfree(fmt);\n+\t\t\t\tstrbuf_addbuf(&msgbuf, &sb_b);\n+\t\t\t} else {\n+\t\t\t\t/* TODO: The not-truncated version ... */\n+\t\t\t}\n+\t\t} else {\n \t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n \t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n \t\t\tstrbuf_addstr(&msgbuf, \"\\\"\");\n-\t\t} else {\n-\t\t\tstrbuf_addstr(&msgbuf, \"DESCRIBE WHY WE ARE REVERTING HERE\");\n-\t\t}\n-\t\tstrbuf_addstr(&msgbuf, \"\\n\\nThis reverts commit \");\n-\t\trefer_to_commit(opts, &msgbuf, commit);\n+\t\t\tstrbuf_addstr(&msgbuf, \"\\n\\nThis reverts commit \");\n+\t\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\trefer_to_commit(opts, &msgbuf, parent);\n+\t\t\tif (commit->parents && commit->parents->next) {\n+\t\t\t\t/* TODO: Handle the wrapping of this too... */\n+\t\t\t\tstrbuf_addstr(&msgbuf, \", reversing\\nchanges made to \");\n+\t\t\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid));\n+\t\t\t}\n \t\t}\n \t\tstrbuf_addstr(&msgbuf, \".\\n\");\n \t} else {\n"},{"id":"455856","messageId":"xmqqy1yrnado.fsf@gitster.g","threadId":"57910","inReplyTo":"220523.86wnecxqol.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] revert: optionally refer to commit in the \"reference\" format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-23T22:37:39Z","receivedAt":"2022-05-23T22:37:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> +\t\tif (!opts->commit_use_reference) {\n>> +\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n>> +\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n>> +\t\t\tstrbuf_addstr(&msgbuf, \"\\\"\");\n>> +\t\t} else {\n>> +\t\t\tstrbuf_addstr(&msgbuf, \"DESCRIBE WHY WE ARE REVERTING HERE\");\n>\n> Having seen how people use git in the wild, this *WILL* result in a\n> bunch of history where people don't edit that at all.\n\nIt was done very much on purpose. A pressure from other project\nparticipants against the ugly all caps content-free message will\nhelp instill better behaviour. A solo developer also will learn\nafter making reverts with content-free titles (and if they do not\nfind it inconvenient for their development purpose that their\nhistory is full of content-free titles shouting in all caps, then\nmore power to them---if they are happy, we are happy, too).\n\nIf we leave something like\n\n\t# Add one line above and explain not *what* this revert did,\n\t# but *why* you decided to revert in 50-70 chars. Did it\n\t# break something? Was it premature?\n\n\tThis reverts commit 8aa3f0dc (CI: set CC in MAKEFLAGS directly, 2022-04-21)\n\npresumably, stripspace will make the \"This reverts commit ...\" the\ntitle of the reverted commit. It would invite people not to edit it\nout out of laziness. Since the whole point of this change is to\nencourage people to describe the reason behind the revert on the\nsubject line, such an invitation is counter-productive.\n\nDoing the first two lines like so:\n\n\tRevert 8aa3f0dc (CI: set CC in MAKEFLAGS directly, 2022-04-21)\n\t# Edit the above line to explain not *what* this revert did,\n\nwould have pretty much the same effect, I am afraid.\n\nSo...\n"},{"id":"455857","messageId":"xmqq35gzn9vk.fsf@gitster.g","threadId":"57910","inReplyTo":"nycvar.QRO.7.76.6.2205231507350.352@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] revert: optionally refer to commit in the \"reference\" format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-23T22:48:31Z","receivedAt":"2022-05-23T22:48:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Good idea, the `reference` text is so much more pleasant to read (and it\n> also makes things somewhat proof against rebasing and transitioning to\n> SHA-256).\n\nNever thought of that myself. It does sound like another good\nreason to see if this is a good alternative format to use.\n\n> At some stage, we may even default to `revert.reference=true`?\n\nPerhaps. Wasn't planning to propose that in any near future,\neither, though. I would prefer to never think about changing the\ndefault in the future when considering a new feature. It should be\ndone as two separate steps (except that care must be taken so that\nthe feature could become default if we choose to).\n\n>> +--reference::\n>> ...\n>> +\tdefault.\n>> +\n>> +\n>\n> Is that extra empty line intentional?\n\nIt is very much so. A single blank between each items in the\nsection, a larger blank between two sections, is what I aimed at\nhere.\n\n>> +\t\tif (!opts->commit_use_reference) {\n>> +\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n>> +\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n>> +\t\t\tstrbuf_addstr(&msgbuf, \"\\\"\");\n>> +\t\t} else {\n>> +\t\t\tstrbuf_addstr(&msgbuf, \"DESCRIBE WHY WE ARE REVERTING HERE\");\n>> +\t\t}\n>\n> We often tell newcomers that we prefer the shorter conditional block to\n> come first. Maybe you want to do that here, too? I don't mind the current\n> form, though.\n\nGood point, but that is more applicable to new code (and not limited\nto newcomers). We also tell that your new feature is often not any\nmore important than the existing code, and think twice before\nprepending the new code to existing code (iow, by default new code\nshould be appended, unless there is a good reason not to).\n\nI kept the original behaviour first with optional new action as a\nfollow-up. Besides, 3-line block is not all that larger than a\nsingle-line block, and this did not trigger my \"that one is larger\"\nsensor.\n\nLet me try swapping the order.\n\n>> @@ -174,4 +175,34 @@ test_expect_success 'advice from failed revert' '\n>> \ttest_must_fail git revert HEAD^ 2>actual &&\n>> \ttest_cmp expected actual\n>> '\n>> +\n>> +test_expect_success 'identification of reverted commit (vanilla)' '\n>\n> It might make sense to replace \"vanilla\" by \"default format\".\n\nSounds very sensible; thanks.\n\n>> +\ttest_commit to-ident &&\n>> +\ttest_when_finished \"git reset --hard to-ident\" &&\n>> +\tgit checkout --detach to-ident &&\n>> +\tgit revert --no-edit HEAD &&\n>> +\tgit cat-file commit HEAD >actual.raw &&\n>> +\tgrep \"^This reverts \" actual.raw >actual &&\n>> +\techo \"This reverts commit $(git rev-parse HEAD^).\" >expect &&\n>\n> I was a bit puzzled about this dance, as I had expected this instead:\n>\n> \tgrep \"^This reverts commit $(git rev-parse HEAD^)\\\\.$\" actual.raw\n>\n> but I guess you wanted to make sure that the file contents of `actual` are\n> shown if no match was found?\n\nMy thought process was much simpler.\n\n\"^This reverts\" is common in both formats, so grep would extract the\ncommit reference(s) the command would have made to \"actual\".\n\nWhat we need to make sure with the test_cmp are multifold. We want\nto make sure that the reference is made in the right format, refers\nto the right commit, and we show the reference exactly the expected\nnumber (i.e. one) of times. So it was simpler to do so in a step\nseparate from the \"extraction\" step above. We could go looser and\nuse test_contains but I usually try to avoid deliberately to be\nloose.\n\n>> +test_expect_success 'identification of reverted commit (reference)' '\n>> +\tgit checkout --detach to-ident &&\n>> +\tgit revert --reference --no-edit HEAD &&\n>> +\tgit cat-file commit HEAD >actual.raw &&\n>> +\tgrep \"^This reverts \" actual.raw >actual &&\n>> +\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n>> +\ttest_cmp expect actual\n>> +'\n>\n> If it was up to me, I would combine these three test cases, if only to\n> help the `--run=` case (the latter two depend on the side\n> effect of the first one to create a `to-ident` tag).\n\nI wonder if our prereq infrastructure is lightweight and scalable\nenough so that we can easily add a support a pseudo-prerequisite\nPREVIOUS that lets us say\n\n\ttest_expect_success PREVIOUS \"identification ...\" '\n\t\t...\n\t'\n\nto mean that this test requires the previous test has not been\nskipped.\n\nThen we can organize them into four (one to create to-ident commit,\none uses vanilla, one uses --reference and one uses revert.reference),\nand make the latter three depend on their previous step.\n\n>> +test_expect_success 'identification of reverted commit (reference)' '\n>> +\tgit checkout --detach to-ident &&\n>> +\tgit -c revert.reference=true revert --no-edit HEAD &&\n>> +\tgit cat-file commit HEAD >actual.raw &&\n>> +\tgrep \"^This reverts \" actual.raw >actual &&\n>> +\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n>> +\ttest_cmp expect actual\n>> +'\n>> +\n>> test_done\n\nThanks for taking a look.\n\n--- >8 ---\nSubject: [PATCH v2] revert: optionally refer to commit in the \"reference\" format\n\nA typical \"git revert\" commit uses the full title of the original\ncommit in its title, and starts its body of the message with:\n\n This reverts commit 8fa7f667cf61386257c00d6e954855cc3215ae91.\n\nThis does not encourage the best practice of describing not just\n\"what\" (i.e. \"Revert X\" on the title says what we did) but \"why\"\n(i.e. and it does not say why X was undesirable).\n\nWe can instead phrase this first line of the body to be more like\n\n This reverts commit 8fa7f667 (do this and that, 2022-04-25)\n\nso that the title does not have to be\n\n Revert \"do this and that\"\n\nWe can instead use the title to describe \"why\" we are reverting the\noriginal commit.\n\nIntroduce the \"--reference\" option to \"git revert\", and also the\nrevert.reference configuration variable, which defaults to false, to\ntweak the title and the first line of the draft commit message for\nwhen creating a \"revert\" commit.\n\nSigned-off-by: Junio C Hamano \n---\nRange-diff:\n1: 26b60b56c0 ! 1: c5b11efe7d revert: optionally refer to commit in the \"reference\" format\n @@ sequencer.c: static int do_pick_commit(struct repository *r,\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 -+\t\tif (!opts->commit_use_reference) {\n ++\t\tif (opts->commit_use_reference) {\n ++\t\t\tstrbuf_addstr(&msgbuf, \"DESCRIBE WHY WE ARE REVERTING HERE\");\n ++\t\t} else {\n +\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n +\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n +\t\t\tstrbuf_addstr(&msgbuf, \"\\\"\");\n -+\t\t} else {\n -+\t\t\tstrbuf_addstr(&msgbuf, \"DESCRIBE WHY WE ARE REVERTING HERE\");\n +\t\t}\n +\t\tstrbuf_addstr(&msgbuf, \"\\n\\nThis reverts commit \");\n +\t\trefer_to_commit(opts, &msgbuf, commit);\n @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'advice from failed revert' '\n \ttest_cmp expected actual\n '\n +\n -+test_expect_success 'identification of reverted commit (vanilla)' '\n ++test_expect_success 'identification of reverted commit (default)' '\n +\ttest_commit to-ident &&\n +\ttest_when_finished \"git reset --hard to-ident\" &&\n +\tgit checkout --detach to-ident &&\n @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'advice from failed revert' '\n +\ttest_cmp expect actual\n +'\n +\n -+test_expect_success 'identification of reverted commit (reference)' '\n ++test_expect_success 'identification of reverted commit (--reference)' '\n +\tgit checkout --detach to-ident &&\n +\tgit revert --reference --no-edit HEAD &&\n +\tgit cat-file commit HEAD >actual.raw &&\n @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'advice from failed revert' '\n +\ttest_cmp expect actual\n +'\n +\n -+test_expect_success 'identification of reverted commit (reference)' '\n ++test_expect_success 'identification of reverted commit (revert.reference)' '\n +\tgit checkout --detach to-ident &&\n +\tgit -c revert.reference=true revert --no-edit HEAD &&\n +\tgit cat-file commit HEAD >actual.raw &&\n\n Documentation/config/revert.txt | 3 +++\n Documentation/git-revert.txt | 9 +++++++++\n builtin/revert.c | 2 ++\n sequencer.c | 32 +++++++++++++++++++++++++++-----\n sequencer.h | 1 +\n t/t3501-revert-cherry-pick.sh | 31 +++++++++++++++++++++++++++++++\n 6 files changed, 73 insertions(+), 5 deletions(-)\n create mode 100644 Documentation/config/revert.txt\n\ndiff --git a/Documentation/config/revert.txt b/Documentation/config/revert.txt\nnew file mode 100644\nindex 0000000000..797bfb6d62\n--- /dev/null\n+++ b/Documentation/config/revert.txt\n@@ -0,0 +1,3 @@\n+revert.reference::\n+\tSetting this variable to true makes `git revert` to behave\n+\tas if the `--reference` option is given.\ndiff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt\nindex bb92a4a451..8463fe9cf7 100644\n--- a/Documentation/git-revert.txt\n+++ b/Documentation/git-revert.txt\n@@ -117,6 +117,15 @@ effect to your index in a row.\n \tAllow the rerere mechanism to update the index with the\n \tresult of auto-conflict resolution if possible.\n \n+--reference::\n+\tInstead of starting the body of the log message with \"This\n+\treverts .\",\n+\trefer to the commit using \"--pretty=reference\" format\n+\t(cf. linkgit:git-log[1]). The `revert.reference`\n+\tconfiguration variable can be used to enable this option by\n+\tdefault.\n+\n+\n SEQUENCER SUBCOMMANDS\n ---------------------\n include::sequencer.txt[]\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 51776abea6..ada51e46b9 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -116,6 +116,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\tN_(\"option for merge strategy\"), option_parse_x),\n \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n \t\t N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n+\t\tOPT_BOOL(0, \"reference\", &opts->commit_use_reference,\n+\t\t\t N_(\"use the 'reference' format to refer to commits\")),\n \t\tOPT_END()\n \t};\n \tstruct option *options = base_options;\ndiff --git a/sequencer.c b/sequencer.c\nindex a5f678f452..92d301f0c4 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -221,6 +221,9 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n \t\treturn ret;\n \t}\n \n+\tif (!strcmp(k, \"revert.reference\"))\n+\t\topts->commit_use_reference = git_config_bool(k, v);\n+\n \tstatus = git_gpg_config(k, v, NULL);\n \tif (status)\n \t\treturn status;\n@@ -2059,6 +2062,20 @@ static int should_edit(struct replay_opts *opts) {\n \treturn opts->edit;\n }\n \n+static void refer_to_commit(struct replay_opts *opts,\n+\t\t\t struct strbuf *msgbuf, struct commit *commit)\n+{\n+\tif (opts->commit_use_reference) {\n+\t\tstruct pretty_print_context ctx = {\n+\t\t\t.abbrev = DEFAULT_ABBREV,\n+\t\t\t.date_mode.type = DATE_SHORT,\n+\t\t};\n+\t\tformat_commit_message(commit, \"%h (%s, %ad)\", msgbuf, &ctx);\n+\t} else {\n+\t\tstrbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));\n+\t}\n+}\n+\n static int do_pick_commit(struct repository *r,\n \t\t\t struct todo_item *item,\n \t\t\t struct replay_opts *opts,\n@@ -2167,14 +2184,19 @@ static int do_pick_commit(struct repository *r,\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+\t\tif (opts->commit_use_reference) {\n+\t\t\tstrbuf_addstr(&msgbuf, \"DESCRIBE WHY WE ARE REVERTING HERE\");\n+\t\t} else {\n+\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n+\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n+\t\t\tstrbuf_addstr(&msgbuf, \"\\\"\");\n+\t\t}\n+\t\tstrbuf_addstr(&msgbuf, \"\\n\\nThis reverts commit \");\n+\t\trefer_to_commit(opts, &msgbuf, commit);\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\trefer_to_commit(opts, &msgbuf, parent);\n \t\t}\n \t\tstrbuf_addstr(&msgbuf, \".\\n\");\n \t} else {\ndiff --git a/sequencer.h b/sequencer.h\nindex da64473636..698599fe4e 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -49,6 +49,7 @@ struct replay_opts {\n \tint reschedule_failed_exec;\n \tint committer_date_is_author_date;\n \tint ignore_date;\n+\tint commit_use_reference;\n \n \tint mainline;\n \ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex 8617efaaf1..a386ae9e88 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -159,6 +159,7 @@ test_expect_success 'cherry-pick works with dirty renamed file' '\n '\n \n test_expect_success 'advice from failed revert' '\n+\ttest_when_finished \"git reset --hard\" &&\n \ttest_commit --no-tag \"add dream\" dream dream &&\n \tdream_oid=$(git rev-parse --short HEAD) &&\n \tcat <<-EOF >expected &&\n@@ -174,4 +175,34 @@ test_expect_success 'advice from failed revert' '\n \ttest_must_fail git revert HEAD^ 2>actual &&\n \ttest_cmp expected actual\n '\n+\n+test_expect_success 'identification of reverted commit (default)' '\n+\ttest_commit to-ident &&\n+\ttest_when_finished \"git reset --hard to-ident\" &&\n+\tgit checkout --detach to-ident &&\n+\tgit revert --no-edit HEAD &&\n+\tgit cat-file commit HEAD >actual.raw &&\n+\tgrep \"^This reverts \" actual.raw >actual &&\n+\techo \"This reverts commit $(git rev-parse HEAD^).\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'identification of reverted commit (--reference)' '\n+\tgit checkout --detach to-ident &&\n+\tgit revert --reference --no-edit HEAD &&\n+\tgit cat-file commit HEAD >actual.raw &&\n+\tgrep \"^This reverts \" actual.raw >actual &&\n+\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'identification of reverted commit (revert.reference)' '\n+\tgit checkout --detach to-ident &&\n+\tgit -c revert.reference=true revert --no-edit HEAD &&\n+\tgit cat-file commit HEAD >actual.raw &&\n+\tgrep \"^This reverts \" actual.raw >actual &&\n+\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.36.1-312-ge077cd1662\n\n"},{"id":"455877","messageId":"220524.867d6bxs40.gmgdl@evledraar.gmail.com","threadId":"57910","inReplyTo":"xmqqy1yrnado.fsf@gitster.g","subject":"Re: [PATCH] revert: optionally refer to commit in the \"reference\" format","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-24T08:12:15Z","receivedAt":"2022-05-24T08:16:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, May 23 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>>> +\t\tif (!opts->commit_use_reference) {\n>>> +\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n>>> +\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n>>> +\t\t\tstrbuf_addstr(&msgbuf, \"\\\"\");\n>>> +\t\t} else {\n>>> +\t\t\tstrbuf_addstr(&msgbuf, \"DESCRIBE WHY WE ARE REVERTING HERE\");\n>>\n>> Having seen how people use git in the wild, this *WILL* result in a\n>> bunch of history where people don't edit that at all.\n>\n> It was done very much on purpose. A pressure from other project\n> participants against the ugly all caps content-free message will\n> help instill better behaviour.\n\nIn some cases that will happen, but from experience of working with a\nbunch of novice git users some people won't edit these at all,\nparticularly when they're in a rush (e.g. reverting something because a\nthing broke production).\n\nSo making the default subject less useful is something that will be seen\nby others, and thus result in worse UX.\n\nAFAICT all of the goals you're suggesting here will be even better if\nit's commented out, then you will need to edit it, and we'll error out\nby default if you don't. Why not use that?\n\n> A solo developer also will learn\n> after making reverts with content-free titles (and if they do not\n> find it inconvenient for their development purpose that their\n> history is full of content-free titles shouting in all caps, then\n> more power to them---if they are happy, we are happy, too).\n\nThe subject you're replacing isn't content-free, it at least includes\nthe OID, this one doesn't, so you won't see it in --oneline. That's less\nuseful.\n\n> If we leave something like\n>\n> \t# Add one line above and explain not *what* this revert did,\n> \t# but *why* you decided to revert in 50-70 chars. Did it\n> \t# break something? Was it premature?\n>\n> \tThis reverts commit 8aa3f0dc (CI: set CC in MAKEFLAGS directly, 2022-04-21)\n>\n> presumably, stripspace will make the \"This reverts commit ...\" the\n> title of the reverted commit. It would invite people not to edit it\n> out out of laziness. Since the whole point of this change is to\n> encourage people to describe the reason behind the revert on the\n> subject line, such an invitation is counter-productive.\n\nIf that's the concern we could also comment the \"This reverts\" line.\n\n> Doing the first two lines like so:\n>\n> \tRevert 8aa3f0dc (CI: set CC in MAKEFLAGS directly, 2022-04-21)\n> \t# Edit the above line to explain not *what* this revert did,\n>\n> would have pretty much the same effect, I am afraid.\n>\n> So...\n\nDitto, so just present a buffer like:\n\n # Revert 8aa3f0dc (CI: set CC in MAKEFLAGS directly, 2022-04-21)\n #\n # This reverts commit 8aa3f0dc (CI: set CC in MAKEFLAGS directly, 2022-04-21)\n\nAlong with some advice that the user can uncomment those template lines\n& adjust them.\n"},{"id":"455930","messageId":"xmqq1qwiiw3x.fsf@gitster.g","threadId":"57910","inReplyTo":"220524.867d6bxs40.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] revert: optionally refer to commit in the \"reference\" format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-24T19:11:46Z","receivedAt":"2022-05-24T19:11:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> AFAICT all of the goals you're suggesting here will be even better if\n> it's commented out, then you will need to edit it, and we'll error out\n> by default if you don't. Why not use that?\n\nSimply because I doubt that would work. Doesn't the stripspace on\nthe log message with unedited comments simply discard the commented\nout part and results in a title that is taken from the line that\nhappens to end up being the first non-blank-non-commented-out line?\n\n> The subject you're replacing isn't content-free, it at least includes\n> the OID, this one doesn't, so you won't see it in --oneline. That's less\n> useful.\n\nI know. That is the point; those who choose to use --reference want\nto make sure they want to edit out the line that tell them to edit.\n\n>\n>> If we leave something like\n>>\n>> \t# Add one line above and explain not *what* this revert did,\n>> \t# but *why* you decided to revert in 50-70 chars. Did it\n>> \t# break something? Was it premature?\n>>\n>> \tThis reverts commit 8aa3f0dc (CI: set CC in MAKEFLAGS directly, 2022-04-21)\n>>\n>> presumably, stripspace will make the \"This reverts commit ...\" the\n>> title of the reverted commit. It would invite people not to edit it\n>> out out of laziness. Since the whole point of this change is to\n>> encourage people to describe the reason behind the revert on the\n>> subject line, such an invitation is counter-productive.\n>\n> If that's the concern we could also comment the \"This reverts\" line.\n\nThat punishes the normal use case where the title is edited and the\nbody is used to start the message as-is.\n\n"},{"id":"456167","messageId":"4c8ea23d-1a1d-efd4-6c11-7c08f8728d59@gmail.com","threadId":"57910","inReplyTo":"xmqqsfp2b30k.fsf@gitster.g","subject":"Re: [PATCH] revert: optionally refer to commit in the \"reference\" format","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-05-26T11:17:33Z","receivedAt":"2022-05-26T11:17:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 22/05/2022 05:32, Junio C Hamano wrote:\n> A typical \"git revert\" commit uses the full title of the original\n> commit in its title, and starts its body of the message with:\n> \n> This reverts commit 8fa7f667cf61386257c00d6e954855cc3215ae91.\n> \n> This does not encourage the best practice of describing not just\n> \"what\" (i.e. \"Revert X\" on the title says what we did) but \"why\"\n> (i.e. and it does not say why X was undesirable).\n> \n> We can instead phrase this first line of the body to be more like\n> \n> This reverts commit 8fa7f667 (do this and that, 2022-04-25)\n> \n> so that the title does not have to be\n> \n> Revert \"do this and that\"\n> \n> We can instead use the title to describe \"why\" we are reverting the\n> original commit.\n> \n> Introduce the \"--reference\" option to \"git revert\", and also the\n> revert.reference configuration variable, which defaults to false, to\n> tweak the title and the first line of the draft commit message for\n> when creating a \"revert\" commit.\n\nI think this is a good idea which will hopefully improve project histories.\n\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 51776abea6..ada51e46b9 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -116,6 +116,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> \t\t\tN_(\"option for merge strategy\"), option_parse_x),\n> \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n> \t\t N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n> +\t\tOPT_BOOL(0, \"reference\", &opts->commit_use_reference,\n> +\t\t\t N_(\"use the 'reference' format to refer to commits\")),\n\nThis option is being added to base_options which applies to cherry-pick \nas well as revert. There is a \"if\" statement just below this hunk which \nadds cherry-pick specific options, I think we want to add this new \noption in an else block added to that \"if\" statement.\n\n> static int do_pick_commit(struct repository *r,\n> \t\t\t struct todo_item *item,\n> \t\t\t struct replay_opts *opts,\n> @@ -2167,14 +2184,19 @@ static int do_pick_commit(struct repository *r,\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> +\t\tif (!opts->commit_use_reference) {\n> +\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n> +\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n> +\t\t\tstrbuf_addstr(&msgbuf, \"\\\"\");\n> +\t\t} else {\n> +\t\t\tstrbuf_addstr(&msgbuf, \"DESCRIBE WHY WE ARE REVERTING HERE\");\n\nIn format-patch we add three asterisks to the beginning and end of the \ndummy subject and body lines to encourage the user to edit them - is \nthat worth doing here as well?\n\nBest Wishes\n\nPhillip\n"},{"id":"456197","messageId":"xmqqzgj4yzha.fsf@gitster.g","threadId":"57910","inReplyTo":"4c8ea23d-1a1d-efd4-6c11-7c08f8728d59@gmail.com","subject":"Re: [PATCH] revert: optionally refer to commit in the \"reference\" format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-26T17:29:05Z","receivedAt":"2022-05-26T17:29:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> In format-patch we add three asterisks to the beginning and end of the\n> dummy subject and body lines to encourage the user to edit them - is \n> that worth doing here as well?\n\nYeah, that would make things consistent.\n\nI am still mulling over Ævar's idea of preparing the messsage in\nsuch a way that, if committed without editing, the title becomes\nidentifiable but too ugly to force users consider rewording (and the\n\"identifiable\" part helps the \"rebase -i\" process), while making it\neasy to discard the instruction for the user who do want to edit. I\nthink that is the right approach, if we can find one. What has been\nfloated on the list during the discussion is not there yet, but I\nthink the approach is a good one.\n\nThanks.\n\n"},{"id":"456267","messageId":"xmqq8rqn7buk.fsf_-_@gitster.g","threadId":"57910","inReplyTo":"xmqq35gzn9vk.fsf@gitster.g","subject":"[PATCH v3] revert: optionally refer to commit in the \"reference\" format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-27T06:01:39Z","receivedAt":"2022-05-27T06:01:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A typical \"git revert\" commit uses the full title of the original\ncommit in its title, and starts its body of the message with:\n\n This reverts commit 8fa7f667cf61386257c00d6e954855cc3215ae91.\n\nThis does not encourage the best practice of describing not just\n\"what\" (i.e. \"Revert X\" on the title says what we did) but \"why\"\n(i.e. and it does not say why X was undesirable).\n\nWe can instead phrase this first line of the body to be more like\n\n This reverts commit 8fa7f667 (do this and that, 2022-04-25)\n\nso that the title does not have to be\n\n Revert \"do this and that\"\n\nWe can instead use the title to describe \"why\" we are reverting the\noriginal commit.\n\nIntroduce the \"--reference\" option to \"git revert\", and also the\nrevert.reference configuration variable, which defaults to false, to\ntweak the title and the first line of the draft commit message for\nwhen creating a \"revert\" commit.\n\nWhen this option is in use, the first line of the pre-filled editor\nbuffer becomes a comment line that tells the user to say _why_. If\nthe user exits the editor without touching this line by mistake,\nwhat we prepare to become the first line of the body, i.e. \"This\nreverts commit 8fa7f667 (do this and that, 2022-04-25)\", ends up to\nbe the title of the resulting commit. This behaviour is designed to\nhelp such a user to identify such a revert in \"git log --oneline\"\neasily so that it can be further reworded with \"git rebase -i\" later.\n\nSigned-off-by: Junio C Hamano \n---\n\n * The only difference since the second one is that the first line of\n the log message template is commented out and has asterisks to\n draw more attention. The last paragraph (new) in the proposed\n log message explains the rationale behind this design.\n\n Third-time a charm, hopefully.\n\nRange-diff against v2:\n1: 4152fc2092 ! 1: f4325a503a revert: optionally refer to commit in the \"reference\" format\n @@ Commit message\n tweak the title and the first line of the draft commit message for\n when creating a \"revert\" commit.\n \n + When this option is in use, the first line of the pre-filled editor\n + buffer becomes a comment line that tells the user to say _why_. If\n + the user exits the editor without touching this line by mistake,\n + what we prepare to become the first line of the body, i.e. \"This\n + reverts commit 8fa7f667 (do this and that, 2022-04-25)\", ends up to\n + be the title of the resulting commit. This behaviour is designed to\n + help such a user to identify such a revert in \"git log --oneline\"\n + easily so that it can be further reworded with \"git rebase -i\" later.\n +\n Signed-off-by: Junio C Hamano \n \n ## Documentation/config/revert.txt (new) ##\n @@ sequencer.c: static int do_pick_commit(struct repository *r,\n -\t\tstrbuf_addstr(&msgbuf, \"\\\"\\n\\nThis reverts commit \");\n -\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n +\t\tif (opts->commit_use_reference) {\n -+\t\t\tstrbuf_addstr(&msgbuf, \"DESCRIBE WHY WE ARE REVERTING HERE\");\n ++\t\t\tstrbuf_addstr(&msgbuf,\n ++\t\t\t\t\"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n +\t\t} else {\n +\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n +\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n\n Documentation/config/revert.txt | 3 +++\n Documentation/git-revert.txt | 9 +++++++++\n builtin/revert.c | 2 ++\n sequencer.c | 33 ++++++++++++++++++++++++++++-----\n sequencer.h | 1 +\n t/t3501-revert-cherry-pick.sh | 31 +++++++++++++++++++++++++++++++\n 6 files changed, 74 insertions(+), 5 deletions(-)\n create mode 100644 Documentation/config/revert.txt\n\ndiff --git a/Documentation/config/revert.txt b/Documentation/config/revert.txt\nnew file mode 100644\nindex 0000000000..797bfb6d62\n--- /dev/null\n+++ b/Documentation/config/revert.txt\n@@ -0,0 +1,3 @@\n+revert.reference::\n+\tSetting this variable to true makes `git revert` to behave\n+\tas if the `--reference` option is given.\ndiff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt\nindex bb92a4a451..8463fe9cf7 100644\n--- a/Documentation/git-revert.txt\n+++ b/Documentation/git-revert.txt\n@@ -117,6 +117,15 @@ effect to your index in a row.\n \tAllow the rerere mechanism to update the index with the\n \tresult of auto-conflict resolution if possible.\n \n+--reference::\n+\tInstead of starting the body of the log message with \"This\n+\treverts .\",\n+\trefer to the commit using \"--pretty=reference\" format\n+\t(cf. linkgit:git-log[1]). The `revert.reference`\n+\tconfiguration variable can be used to enable this option by\n+\tdefault.\n+\n+\n SEQUENCER SUBCOMMANDS\n ---------------------\n include::sequencer.txt[]\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 51776abea6..ada51e46b9 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -116,6 +116,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\tN_(\"option for merge strategy\"), option_parse_x),\n \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n \t\t N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n+\t\tOPT_BOOL(0, \"reference\", &opts->commit_use_reference,\n+\t\t\t N_(\"use the 'reference' format to refer to commits\")),\n \t\tOPT_END()\n \t};\n \tstruct option *options = base_options;\ndiff --git a/sequencer.c b/sequencer.c\nindex a5f678f452..96fec6ef6d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -221,6 +221,9 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n \t\treturn ret;\n \t}\n \n+\tif (!strcmp(k, \"revert.reference\"))\n+\t\topts->commit_use_reference = git_config_bool(k, v);\n+\n \tstatus = git_gpg_config(k, v, NULL);\n \tif (status)\n \t\treturn status;\n@@ -2059,6 +2062,20 @@ static int should_edit(struct replay_opts *opts) {\n \treturn opts->edit;\n }\n \n+static void refer_to_commit(struct replay_opts *opts,\n+\t\t\t struct strbuf *msgbuf, struct commit *commit)\n+{\n+\tif (opts->commit_use_reference) {\n+\t\tstruct pretty_print_context ctx = {\n+\t\t\t.abbrev = DEFAULT_ABBREV,\n+\t\t\t.date_mode.type = DATE_SHORT,\n+\t\t};\n+\t\tformat_commit_message(commit, \"%h (%s, %ad)\", msgbuf, &ctx);\n+\t} else {\n+\t\tstrbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));\n+\t}\n+}\n+\n static int do_pick_commit(struct repository *r,\n \t\t\t struct todo_item *item,\n \t\t\t struct replay_opts *opts,\n@@ -2167,14 +2184,20 @@ static int do_pick_commit(struct repository *r,\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+\t\tif (opts->commit_use_reference) {\n+\t\t\tstrbuf_addstr(&msgbuf,\n+\t\t\t\t\"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n+\t\t} else {\n+\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n+\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n+\t\t\tstrbuf_addstr(&msgbuf, \"\\\"\");\n+\t\t}\n+\t\tstrbuf_addstr(&msgbuf, \"\\n\\nThis reverts commit \");\n+\t\trefer_to_commit(opts, &msgbuf, commit);\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\trefer_to_commit(opts, &msgbuf, parent);\n \t\t}\n \t\tstrbuf_addstr(&msgbuf, \".\\n\");\n \t} else {\ndiff --git a/sequencer.h b/sequencer.h\nindex da64473636..698599fe4e 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -49,6 +49,7 @@ struct replay_opts {\n \tint reschedule_failed_exec;\n \tint committer_date_is_author_date;\n \tint ignore_date;\n+\tint commit_use_reference;\n \n \tint mainline;\n \ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex 8617efaaf1..a386ae9e88 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -159,6 +159,7 @@ test_expect_success 'cherry-pick works with dirty renamed file' '\n '\n \n test_expect_success 'advice from failed revert' '\n+\ttest_when_finished \"git reset --hard\" &&\n \ttest_commit --no-tag \"add dream\" dream dream &&\n \tdream_oid=$(git rev-parse --short HEAD) &&\n \tcat <<-EOF >expected &&\n@@ -174,4 +175,34 @@ test_expect_success 'advice from failed revert' '\n \ttest_must_fail git revert HEAD^ 2>actual &&\n \ttest_cmp expected actual\n '\n+\n+test_expect_success 'identification of reverted commit (default)' '\n+\ttest_commit to-ident &&\n+\ttest_when_finished \"git reset --hard to-ident\" &&\n+\tgit checkout --detach to-ident &&\n+\tgit revert --no-edit HEAD &&\n+\tgit cat-file commit HEAD >actual.raw &&\n+\tgrep \"^This reverts \" actual.raw >actual &&\n+\techo \"This reverts commit $(git rev-parse HEAD^).\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'identification of reverted commit (--reference)' '\n+\tgit checkout --detach to-ident &&\n+\tgit revert --reference --no-edit HEAD &&\n+\tgit cat-file commit HEAD >actual.raw &&\n+\tgrep \"^This reverts \" actual.raw >actual &&\n+\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'identification of reverted commit (revert.reference)' '\n+\tgit checkout --detach to-ident &&\n+\tgit -c revert.reference=true revert --no-edit HEAD &&\n+\tgit cat-file commit HEAD >actual.raw &&\n+\tgrep \"^This reverts \" actual.raw >actual &&\n+\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.36.1-331-g1b5d92e060\n\n"},{"id":"456372","messageId":"nycvar.QRO.7.76.6.2205301837280.349@tvgsbejvaqbjf.bet","threadId":"57910","inReplyTo":"xmqq8rqn7buk.fsf_-_@gitster.g","subject":"Re: [PATCH v3] revert: optionally refer to commit in the \"reference\" format","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-05-30T16:40:31Z","receivedAt":"2022-05-30T16:40:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 26 May 2022, Junio C Hamano wrote:\n\n> A typical \"git revert\" commit uses the full title of the original\n> commit in its title, and starts its body of the message with:\n>\n> This reverts commit 8fa7f667cf61386257c00d6e954855cc3215ae91.\n>\n> This does not encourage the best practice of describing not just\n> \"what\" (i.e. \"Revert X\" on the title says what we did) but \"why\"\n> (i.e. and it does not say why X was undesirable).\n>\n> We can instead phrase this first line of the body to be more like\n>\n> This reverts commit 8fa7f667 (do this and that, 2022-04-25)\n>\n> so that the title does not have to be\n>\n> Revert \"do this and that\"\n>\n> We can instead use the title to describe \"why\" we are reverting the\n> original commit.\n>\n> Introduce the \"--reference\" option to \"git revert\", and also the\n> revert.reference configuration variable, which defaults to false, to\n> tweak the title and the first line of the draft commit message for\n> when creating a \"revert\" commit.\n>\n> When this option is in use, the first line of the pre-filled editor\n> buffer becomes a comment line that tells the user to say _why_. If\n> the user exits the editor without touching this line by mistake,\n> what we prepare to become the first line of the body, i.e. \"This\n> reverts commit 8fa7f667 (do this and that, 2022-04-25)\", ends up to\n> be the title of the resulting commit. This behaviour is designed to\n> help such a user to identify such a revert in \"git log --oneline\"\n> easily so that it can be further reworded with \"git rebase -i\" later.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>\n> * The only difference since the second one is that the first line of\n> the log message template is commented out and has asterisks to\n> draw more attention. The last paragraph (new) in the proposed\n> log message explains the rationale behind this design.\n\nWhile it may be more purist to try to force the user to provide a\ndescription by having a non-comment placeholder there, from my experience\nwith PullRequest/Issue templates convinces me that the comment is the more\npractical approach.\n\nLooking good,\nDscho\n\n>\n> Third-time a charm, hopefully.\n>\n> Range-diff against v2:\n> 1: 4152fc2092 ! 1: f4325a503a revert: optionally refer to commit in the \"reference\" format\n> @@ Commit message\n> tweak the title and the first line of the draft commit message for\n> when creating a \"revert\" commit.\n>\n> + When this option is in use, the first line of the pre-filled editor\n> + buffer becomes a comment line that tells the user to say _why_. If\n> + the user exits the editor without touching this line by mistake,\n> + what we prepare to become the first line of the body, i.e. \"This\n> + reverts commit 8fa7f667 (do this and that, 2022-04-25)\", ends up to\n> + be the title of the resulting commit. This behaviour is designed to\n> + help such a user to identify such a revert in \"git log --oneline\"\n> + easily so that it can be further reworded with \"git rebase -i\" later.\n> +\n> Signed-off-by: Junio C Hamano \n>\n> ## Documentation/config/revert.txt (new) ##\n> @@ sequencer.c: static int do_pick_commit(struct repository *r,\n> -\t\tstrbuf_addstr(&msgbuf, \"\\\"\\n\\nThis reverts commit \");\n> -\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n> +\t\tif (opts->commit_use_reference) {\n> -+\t\t\tstrbuf_addstr(&msgbuf, \"DESCRIBE WHY WE ARE REVERTING HERE\");\n> ++\t\t\tstrbuf_addstr(&msgbuf,\n> ++\t\t\t\t\"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n> +\t\t} else {\n> +\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n> +\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n>\n> Documentation/config/revert.txt | 3 +++\n> Documentation/git-revert.txt | 9 +++++++++\n> builtin/revert.c | 2 ++\n> sequencer.c | 33 ++++++++++++++++++++++++++++-----\n> sequencer.h | 1 +\n> t/t3501-revert-cherry-pick.sh | 31 +++++++++++++++++++++++++++++++\n> 6 files changed, 74 insertions(+), 5 deletions(-)\n> create mode 100644 Documentation/config/revert.txt\n>\n> diff --git a/Documentation/config/revert.txt b/Documentation/config/revert.txt\n> new file mode 100644\n> index 0000000000..797bfb6d62\n> --- /dev/null\n> +++ b/Documentation/config/revert.txt\n> @@ -0,0 +1,3 @@\n> +revert.reference::\n> +\tSetting this variable to true makes `git revert` to behave\n> +\tas if the `--reference` option is given.\n> diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt\n> index bb92a4a451..8463fe9cf7 100644\n> --- a/Documentation/git-revert.txt\n> +++ b/Documentation/git-revert.txt\n> @@ -117,6 +117,15 @@ effect to your index in a row.\n> \tAllow the rerere mechanism to update the index with the\n> \tresult of auto-conflict resolution if possible.\n>\n> +--reference::\n> +\tInstead of starting the body of the log message with \"This\n> +\treverts .\",\n> +\trefer to the commit using \"--pretty=reference\" format\n> +\t(cf. linkgit:git-log[1]). The `revert.reference`\n> +\tconfiguration variable can be used to enable this option by\n> +\tdefault.\n> +\n> +\n> SEQUENCER SUBCOMMANDS\n> ---------------------\n> include::sequencer.txt[]\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 51776abea6..ada51e46b9 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -116,6 +116,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> \t\t\tN_(\"option for merge strategy\"), option_parse_x),\n> \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n> \t\t N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n> +\t\tOPT_BOOL(0, \"reference\", &opts->commit_use_reference,\n> +\t\t\t N_(\"use the 'reference' format to refer to commits\")),\n> \t\tOPT_END()\n> \t};\n> \tstruct option *options = base_options;\n> diff --git a/sequencer.c b/sequencer.c\n> index a5f678f452..96fec6ef6d 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -221,6 +221,9 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n> \t\treturn ret;\n> \t}\n>\n> +\tif (!strcmp(k, \"revert.reference\"))\n> +\t\topts->commit_use_reference = git_config_bool(k, v);\n> +\n> \tstatus = git_gpg_config(k, v, NULL);\n> \tif (status)\n> \t\treturn status;\n> @@ -2059,6 +2062,20 @@ static int should_edit(struct replay_opts *opts) {\n> \treturn opts->edit;\n> }\n>\n> +static void refer_to_commit(struct replay_opts *opts,\n> +\t\t\t struct strbuf *msgbuf, struct commit *commit)\n> +{\n> +\tif (opts->commit_use_reference) {\n> +\t\tstruct pretty_print_context ctx = {\n> +\t\t\t.abbrev = DEFAULT_ABBREV,\n> +\t\t\t.date_mode.type = DATE_SHORT,\n> +\t\t};\n> +\t\tformat_commit_message(commit, \"%h (%s, %ad)\", msgbuf, &ctx);\n> +\t} else {\n> +\t\tstrbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));\n> +\t}\n> +}\n> +\n> static int do_pick_commit(struct repository *r,\n> \t\t\t struct todo_item *item,\n> \t\t\t struct replay_opts *opts,\n> @@ -2167,14 +2184,20 @@ static int do_pick_commit(struct repository *r,\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> +\t\tif (opts->commit_use_reference) {\n> +\t\t\tstrbuf_addstr(&msgbuf,\n> +\t\t\t\t\"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n> +\t\t} else {\n> +\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n> +\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n> +\t\t\tstrbuf_addstr(&msgbuf, \"\\\"\");\n> +\t\t}\n> +\t\tstrbuf_addstr(&msgbuf, \"\\n\\nThis reverts commit \");\n> +\t\trefer_to_commit(opts, &msgbuf, commit);\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\trefer_to_commit(opts, &msgbuf, parent);\n> \t\t}\n> \t\tstrbuf_addstr(&msgbuf, \".\\n\");\n> \t} else {\n> diff --git a/sequencer.h b/sequencer.h\n> index da64473636..698599fe4e 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -49,6 +49,7 @@ struct replay_opts {\n> \tint reschedule_failed_exec;\n> \tint committer_date_is_author_date;\n> \tint ignore_date;\n> +\tint commit_use_reference;\n>\n> \tint mainline;\n>\n> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\n> index 8617efaaf1..a386ae9e88 100755\n> --- a/t/t3501-revert-cherry-pick.sh\n> +++ b/t/t3501-revert-cherry-pick.sh\n> @@ -159,6 +159,7 @@ test_expect_success 'cherry-pick works with dirty renamed file' '\n> '\n>\n> test_expect_success 'advice from failed revert' '\n> +\ttest_when_finished \"git reset --hard\" &&\n> \ttest_commit --no-tag \"add dream\" dream dream &&\n> \tdream_oid=$(git rev-parse --short HEAD) &&\n> \tcat <<-EOF >expected &&\n> @@ -174,4 +175,34 @@ test_expect_success 'advice from failed revert' '\n> \ttest_must_fail git revert HEAD^ 2>actual &&\n> \ttest_cmp expected actual\n> '\n> +\n> +test_expect_success 'identification of reverted commit (default)' '\n> +\ttest_commit to-ident &&\n> +\ttest_when_finished \"git reset --hard to-ident\" &&\n> +\tgit checkout --detach to-ident &&\n> +\tgit revert --no-edit HEAD &&\n> +\tgit cat-file commit HEAD >actual.raw &&\n> +\tgrep \"^This reverts \" actual.raw >actual &&\n> +\techo \"This reverts commit $(git rev-parse HEAD^).\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'identification of reverted commit (--reference)' '\n> +\tgit checkout --detach to-ident &&\n> +\tgit revert --reference --no-edit HEAD &&\n> +\tgit cat-file commit HEAD >actual.raw &&\n> +\tgrep \"^This reverts \" actual.raw >actual &&\n> +\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'identification of reverted commit (revert.reference)' '\n> +\tgit checkout --detach to-ident &&\n> +\tgit -c revert.reference=true revert --no-edit HEAD &&\n> +\tgit cat-file commit HEAD >actual.raw &&\n> +\tgrep \"^This reverts \" actual.raw >actual &&\n> +\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n> --\n> 2.36.1-331-g1b5d92e060\n>\n>\n"},{"id":"456373","messageId":"nycvar.QRO.7.76.6.2205301840410.349@tvgsbejvaqbjf.bet","threadId":"57910","inReplyTo":"xmqq35gzn9vk.fsf@gitster.g","subject":"Side effects in Git's test suite, was Re: [PATCH] revert: optionally refer to commit in the \"reference\" format","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-05-30T16:50:58Z","receivedAt":"2022-05-30T16:51:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 23 May 2022, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> >> +test_expect_success 'identification of reverted commit (reference)' '\n> >> +\tgit checkout --detach to-ident &&\n> >> +\tgit revert --reference --no-edit HEAD &&\n> >> +\tgit cat-file commit HEAD >actual.raw &&\n> >> +\tgrep \"^This reverts \" actual.raw >actual &&\n> >> +\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n> >> +\ttest_cmp expect actual\n> >> +'\n> >\n> > If it was up to me, I would combine these three test cases, if only to\n> > help the `--run=` case (the latter two depend on the\n> > side effect of the first one to create a `to-ident` tag).\n>\n> I wonder if our prereq infrastructure is lightweight and scalable enough\n> so that we can easily add a support a pseudo-prerequisite PREVIOUS that\n> lets us say\n>\n> \ttest_expect_success PREVIOUS \"identification ...\" '\n> \t\t...\n> \t'\n>\n> to mean that this test requires the previous test has not been\n> skipped.\n\nIn theory, this sounds good to me.\n\nIn practice, however, side effects are awful and make everything harder,\nfrom developing code to debugging to helping new contributors. I wish we\nwould do away with them altogether and have something more akin to the\nbefore/after constructs known from e.g. TestNG (think `@BeforeTest` and\n`@BeforeClass`).\n\nOne option would be to mark `setup` steps completely differently, sort of\nimitating the prereq infrastructure instead of using\n`test_expect_success`. Kind of prereqs, but required to pass.\n\nThis could potentially allow us to randomize the order in which the test\ncases are run, to identify and fix (unintended) side effects.\n\nA complication is that we have nothing in the way of `@AfterClass`, i.e.\nwe do not have a way to, say, run an Apache instance for the lifecycle of\na given test script _and tear it down at the end_.\n\nAnother, rather obvious complication is that we have 17 years of commit\nhistory introducing side effects left and right :laughing:\n\nCiao,\nDscho\n"},{"id":"456384","messageId":"220531.86y1yi15xt.gmgdl@evledraar.gmail.com","threadId":"57910","inReplyTo":"nycvar.QRO.7.76.6.2205301840410.349@tvgsbejvaqbjf.bet","subject":"Re: Side effects in Git's test suite, was Re: [PATCH] revert: optionally refer to commit in the \"reference\" format","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-31T08:03:42Z","receivedAt":"2022-05-31T08:07:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, May 30 2022, Johannes Schindelin wrote:\n\n> Hi Junio,\n>\n> On Mon, 23 May 2022, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>>\n>> >> +test_expect_success 'identification of reverted commit (reference)' '\n>> >> +\tgit checkout --detach to-ident &&\n>> >> +\tgit revert --reference --no-edit HEAD &&\n>> >> +\tgit cat-file commit HEAD >actual.raw &&\n>> >> +\tgrep \"^This reverts \" actual.raw >actual &&\n>> >> +\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n>> >> +\ttest_cmp expect actual\n>> >> +'\n>> >\n>> > If it was up to me, I would combine these three test cases, if only to\n>> > help the `--run=` case (the latter two depend on the\n>> > side effect of the first one to create a `to-ident` tag).\n>>\n>> I wonder if our prereq infrastructure is lightweight and scalable enough\n>> so that we can easily add a support a pseudo-prerequisite PREVIOUS that\n>> lets us say\n>>\n>> \ttest_expect_success PREVIOUS \"identification ...\" '\n>> \t\t...\n>> \t'\n>>\n>> to mean that this test requires the previous test has not been\n>> skipped.\n>\n> In theory, this sounds good to me.\n>\n> In practice, however, side effects are awful and make everything harder,\n> from developing code to debugging to helping new contributors. I wish we\n> would do away with them altogether and have something more akin to the\n> before/after constructs known from e.g. TestNG (think `@BeforeTest` and\n> `@BeforeClass`).\n>\n> One option would be to mark `setup` steps completely differently, sort of\n> imitating the prereq infrastructure instead of using\n> `test_expect_success`. Kind of prereqs, but required to pass.\n\nI've suggested a test_expect_setup before:\nhttps://lore.kernel.org/git/8735vrvg39.fsf@evledraar.gmail.com/;\nbasically a test_expect_success that ignores GIT_SKIP_TESTS and --run.\n\nI think that even if we could imagine much more complex relationships\n(up to and including writing these tests as Makefiles instead) it's\nbetter to just have the simpler \"this is a setup\".\n\nThen for everything more complex be more eager to split up tests.\n\n> This could potentially allow us to randomize the order in which the test\n> cases are run, to identify and fix (unintended) side effects.\n\nYes, a \"chaos monkey\" mode similar to --stress would be nice.\n\n> A complication is that we have nothing in the way of `@AfterClass`, i.e.\n> we do not have a way to, say, run an Apache instance for the lifecycle of\n> a given test script _and tear it down at the end_.\n\nI think this is generally a feature in that if you find yourself needing\nthis you should split the test up so that the Apache setup is in only\none file.\n\nE.g. in the case of some http tests we have a prereq on something for\ngit:// and a different thing (apache) for http:// tests, so that\ndepending on what combination you have we might end up needlessly\nskipping tests.\n\n> Another, rather obvious complication is that we have 17 years of commit\n> history introducing side effects left and right :laughing:\n\n"},{"id":"456388","messageId":"479d97af-eef1-ce86-19f7-afcc0e6ecf30@gmail.com","threadId":"57910","inReplyTo":"xmqq8rqn7buk.fsf_-_@gitster.g","subject":"Re: [PATCH v3] revert: optionally refer to commit in the \"reference\" format","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-05-31T14:00:47Z","receivedAt":"2022-05-31T14:00:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 27/05/2022 07:01, Junio C Hamano wrote:\n> A typical \"git revert\" commit uses the full title of the original\n> commit in its title, and starts its body of the message with:\n> \n> This reverts commit 8fa7f667cf61386257c00d6e954855cc3215ae91.\n> \n> This does not encourage the best practice of describing not just\n> \"what\" (i.e. \"Revert X\" on the title says what we did) but \"why\"\n> (i.e. and it does not say why X was undesirable).\n> \n> We can instead phrase this first line of the body to be more like\n> \n> This reverts commit 8fa7f667 (do this and that, 2022-04-25)\n> \n> so that the title does not have to be\n> \n> Revert \"do this and that\"\n> \n> We can instead use the title to describe \"why\" we are reverting the\n> original commit.\n> \n> Introduce the \"--reference\" option to \"git revert\", and also the\n> revert.reference configuration variable, which defaults to false, to\n> tweak the title and the first line of the draft commit message for\n> when creating a \"revert\" commit.\n> \n> When this option is in use, the first line of the pre-filled editor\n> buffer becomes a comment line that tells the user to say _why_. If\n> the user exits the editor without touching this line by mistake,\n> what we prepare to become the first line of the body, i.e. \"This\n> reverts commit 8fa7f667 (do this and that, 2022-04-25)\", ends up to\n> be the title of the resulting commit. This behaviour is designed to\n> help such a user to identify such a revert in \"git log --oneline\"\n> easily so that it can be further reworded with \"git rebase -i\" later.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> \n> * The only difference since the second one is that the first line of\n> the log message template is commented out and has asterisks to\n> draw more attention. The last paragraph (new) in the proposed\n> log message explains the rationale behind this design.\n> \n> Third-time a charm, hopefully.\n\nI think the changes to the template message are good. We're still adding \n\"--reference\" as a valid option to cherry-pick though which I don't \nthink is a good idea (though in the future we may want to allow \n\"cherry-pick -x --reference\")\n\nBest Wishes\n\nPhillip\n\n> Range-diff against v2:\n> 1: 4152fc2092 ! 1: f4325a503a revert: optionally refer to commit in the \"reference\" format\n> @@ Commit message\n> tweak the title and the first line of the draft commit message for\n> when creating a \"revert\" commit.\n> \n> + When this option is in use, the first line of the pre-filled editor\n> + buffer becomes a comment line that tells the user to say _why_. If\n> + the user exits the editor without touching this line by mistake,\n> + what we prepare to become the first line of the body, i.e. \"This\n> + reverts commit 8fa7f667 (do this and that, 2022-04-25)\", ends up to\n> + be the title of the resulting commit. This behaviour is designed to\n> + help such a user to identify such a revert in \"git log --oneline\"\n> + easily so that it can be further reworded with \"git rebase -i\" later.\n> +\n> Signed-off-by: Junio C Hamano \n> \n> ## Documentation/config/revert.txt (new) ##\n> @@ sequencer.c: static int do_pick_commit(struct repository *r,\n> -\t\tstrbuf_addstr(&msgbuf, \"\\\"\\n\\nThis reverts commit \");\n> -\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n> +\t\tif (opts->commit_use_reference) {\n> -+\t\t\tstrbuf_addstr(&msgbuf, \"DESCRIBE WHY WE ARE REVERTING HERE\");\n> ++\t\t\tstrbuf_addstr(&msgbuf,\n> ++\t\t\t\t\"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n> +\t\t} else {\n> +\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n> +\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n> \n> Documentation/config/revert.txt | 3 +++\n> Documentation/git-revert.txt | 9 +++++++++\n> builtin/revert.c | 2 ++\n> sequencer.c | 33 ++++++++++++++++++++++++++++-----\n> sequencer.h | 1 +\n> t/t3501-revert-cherry-pick.sh | 31 +++++++++++++++++++++++++++++++\n> 6 files changed, 74 insertions(+), 5 deletions(-)\n> create mode 100644 Documentation/config/revert.txt\n> \n> diff --git a/Documentation/config/revert.txt b/Documentation/config/revert.txt\n> new file mode 100644\n> index 0000000000..797bfb6d62\n> --- /dev/null\n> +++ b/Documentation/config/revert.txt\n> @@ -0,0 +1,3 @@\n> +revert.reference::\n> +\tSetting this variable to true makes `git revert` to behave\n> +\tas if the `--reference` option is given.\n> diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt\n> index bb92a4a451..8463fe9cf7 100644\n> --- a/Documentation/git-revert.txt\n> +++ b/Documentation/git-revert.txt\n> @@ -117,6 +117,15 @@ effect to your index in a row.\n> \tAllow the rerere mechanism to update the index with the\n> \tresult of auto-conflict resolution if possible.\n> \n> +--reference::\n> +\tInstead of starting the body of the log message with \"This\n> +\treverts .\",\n> +\trefer to the commit using \"--pretty=reference\" format\n> +\t(cf. linkgit:git-log[1]). The `revert.reference`\n> +\tconfiguration variable can be used to enable this option by\n> +\tdefault.\n> +\n> +\n> SEQUENCER SUBCOMMANDS\n> ---------------------\n> include::sequencer.txt[]\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 51776abea6..ada51e46b9 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -116,6 +116,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> \t\t\tN_(\"option for merge strategy\"), option_parse_x),\n> \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n> \t\t N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n> +\t\tOPT_BOOL(0, \"reference\", &opts->commit_use_reference,\n> +\t\t\t N_(\"use the 'reference' format to refer to commits\")),\n> \t\tOPT_END()\n> \t};\n> \tstruct option *options = base_options;\n> diff --git a/sequencer.c b/sequencer.c\n> index a5f678f452..96fec6ef6d 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -221,6 +221,9 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n> \t\treturn ret;\n> \t}\n> \n> +\tif (!strcmp(k, \"revert.reference\"))\n> +\t\topts->commit_use_reference = git_config_bool(k, v);\n> +\n> \tstatus = git_gpg_config(k, v, NULL);\n> \tif (status)\n> \t\treturn status;\n> @@ -2059,6 +2062,20 @@ static int should_edit(struct replay_opts *opts) {\n> \treturn opts->edit;\n> }\n> \n> +static void refer_to_commit(struct replay_opts *opts,\n> +\t\t\t struct strbuf *msgbuf, struct commit *commit)\n> +{\n> +\tif (opts->commit_use_reference) {\n> +\t\tstruct pretty_print_context ctx = {\n> +\t\t\t.abbrev = DEFAULT_ABBREV,\n> +\t\t\t.date_mode.type = DATE_SHORT,\n> +\t\t};\n> +\t\tformat_commit_message(commit, \"%h (%s, %ad)\", msgbuf, &ctx);\n> +\t} else {\n> +\t\tstrbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));\n> +\t}\n> +}\n> +\n> static int do_pick_commit(struct repository *r,\n> \t\t\t struct todo_item *item,\n> \t\t\t struct replay_opts *opts,\n> @@ -2167,14 +2184,20 @@ static int do_pick_commit(struct repository *r,\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> +\t\tif (opts->commit_use_reference) {\n> +\t\t\tstrbuf_addstr(&msgbuf,\n> +\t\t\t\t\"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n> +\t\t} else {\n> +\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n> +\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n> +\t\t\tstrbuf_addstr(&msgbuf, \"\\\"\");\n> +\t\t}\n> +\t\tstrbuf_addstr(&msgbuf, \"\\n\\nThis reverts commit \");\n> +\t\trefer_to_commit(opts, &msgbuf, commit);\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\trefer_to_commit(opts, &msgbuf, parent);\n> \t\t}\n> \t\tstrbuf_addstr(&msgbuf, \".\\n\");\n> \t} else {\n> diff --git a/sequencer.h b/sequencer.h\n> index da64473636..698599fe4e 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -49,6 +49,7 @@ struct replay_opts {\n> \tint reschedule_failed_exec;\n> \tint committer_date_is_author_date;\n> \tint ignore_date;\n> +\tint commit_use_reference;\n> \n> \tint mainline;\n> \n> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\n> index 8617efaaf1..a386ae9e88 100755\n> --- a/t/t3501-revert-cherry-pick.sh\n> +++ b/t/t3501-revert-cherry-pick.sh\n> @@ -159,6 +159,7 @@ test_expect_success 'cherry-pick works with dirty renamed file' '\n> '\n> \n> test_expect_success 'advice from failed revert' '\n> +\ttest_when_finished \"git reset --hard\" &&\n> \ttest_commit --no-tag \"add dream\" dream dream &&\n> \tdream_oid=$(git rev-parse --short HEAD) &&\n> \tcat <<-EOF >expected &&\n> @@ -174,4 +175,34 @@ test_expect_success 'advice from failed revert' '\n> \ttest_must_fail git revert HEAD^ 2>actual &&\n> \ttest_cmp expected actual\n> '\n> +\n> +test_expect_success 'identification of reverted commit (default)' '\n> +\ttest_commit to-ident &&\n> +\ttest_when_finished \"git reset --hard to-ident\" &&\n> +\tgit checkout --detach to-ident &&\n> +\tgit revert --no-edit HEAD &&\n> +\tgit cat-file commit HEAD >actual.raw &&\n> +\tgrep \"^This reverts \" actual.raw >actual &&\n> +\techo \"This reverts commit $(git rev-parse HEAD^).\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'identification of reverted commit (--reference)' '\n> +\tgit checkout --detach to-ident &&\n> +\tgit revert --reference --no-edit HEAD &&\n> +\tgit cat-file commit HEAD >actual.raw &&\n> +\tgrep \"^This reverts \" actual.raw >actual &&\n> +\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'identification of reverted commit (revert.reference)' '\n> +\tgit checkout --detach to-ident &&\n> +\tgit -c revert.reference=true revert --no-edit HEAD &&\n> +\tgit cat-file commit HEAD >actual.raw &&\n> +\tgrep \"^This reverts \" actual.raw >actual &&\n> +\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"456433","messageId":"xmqqfskpuh2w.fsf@gitster.g","threadId":"57910","inReplyTo":"479d97af-eef1-ce86-19f7-afcc0e6ecf30@gmail.com","subject":"Re: [PATCH v3] revert: optionally refer to commit in the \"reference\" format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-01T04:45:59Z","receivedAt":"2022-06-01T04:46:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> I think the changes to the template message are good. We're still\n> adding \"--reference\" as a valid option to cherry-pick though which I\n> don't think is a good idea (though in the future we may want to allow \n> \"cherry-pick -x --reference\")\n\nI love when people notice mistakes that the original author and\nother people missed, many eyes making all bugs shallow.\n\nI am inclined to apply the following on top. How does it look?\n\nThanks.\n\n----- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: [PATCH] revert: --reference should apply only to 'revert', not 'cherry-pick'\n\nAs 'revert' and 'cherry-pick' share a lot of code, it is easy to\nmodify the behaviour of one command and inadvertently affect the\nother. An earlier change to teach the '--reference' option and the\n'revert.reference' configuration variable to the former was not\ncareful enough and 'cherry-pick --reference' wasn't rejected as an\nerror.\n\nIt is possible to think 'cherry-pick -x' might benefit from the\n'--reference' option, but it is fundamentally different from\n'revert' in at least two ways to make it questionable:\n\n - 'revert' names a commit that is ancestor of the resulting commit,\n so an abbreviated object name with human readable title is\n sufficient to identify the named commit uniquely without using\n the full object name. On the other hand, 'cherry-pick'\n usually [*] picks a commit that is not an ancestor. It might be\n even picking a private commit that never becomes part of the\n public history.\n\n - The whole commit message of 'cherry-pick' is a copy of the\n original commit, and there is nothing gained to repeat only the\n title part on 'cherry-picked from' message.\n\n[*] well, you could revert and then you can pick the original that\n was reverted to get back to where you were, but then you can\n revert the revert to do the same thing.\n\nHelped-by: Phillip Wood \nSigned-off-by: Junio C Hamano \n---\n builtin/revert.c | 9 +++++++--\n sequencer.c | 2 +-\n t/t3501-revert-cherry-pick.sh | 6 ++++++\n 3 files changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex ada51e46b9..f84c253f4c 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -116,8 +116,6 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\tN_(\"option for merge strategy\"), option_parse_x),\n \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n \t\t N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n-\t\tOPT_BOOL(0, \"reference\", &opts->commit_use_reference,\n-\t\t\t N_(\"use the 'reference' format to refer to commits\")),\n \t\tOPT_END()\n \t};\n \tstruct option *options = base_options;\n@@ -132,6 +130,13 @@ 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, \"reference\", &opts->commit_use_reference,\n+\t\t\t\t N_(\"use the 'reference' format to refer to commits\")),\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 96fec6ef6d..4b66a1f79c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -221,7 +221,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n \t\treturn ret;\n \t}\n \n-\tif (!strcmp(k, \"revert.reference\"))\n+\tif (opts->action == REPLAY_REVERT && !strcmp(k, \"revert.reference\"))\n \t\topts->commit_use_reference = git_config_bool(k, v);\n \n \tstatus = git_gpg_config(k, v, NULL);\ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex a386ae9e88..fb4466599b 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -205,4 +205,10 @@ test_expect_success 'identification of reverted commit (revert.reference)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick is unaware of --reference (for now)' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\ttest_must_fail git cherry-pick --reference HEAD 2>actual &&\n+\tgrep \"^usage: git cherry-pick\" actual\n+'\n+\n test_done\n-- \n2.36.1-393-g8f55ede6c2\n\n"},{"id":"456446","messageId":"c0635170-5a52-c426-8231-d7291a79fa2c@gmail.com","threadId":"57910","inReplyTo":"xmqqfskpuh2w.fsf@gitster.g","subject":"Re: [PATCH v3] revert: optionally refer to commit in the \"reference\" format","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-06-01T15:03:01Z","receivedAt":"2022-06-01T15:03:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 01/06/2022 05:45, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> I think the changes to the template message are good. We're still\n>> adding \"--reference\" as a valid option to cherry-pick though which I\n>> don't think is a good idea (though in the future we may want to allow\n>> \"cherry-pick -x --reference\")\n> \n> I love when people notice mistakes that the original author and\n> other people missed, many eyes making all bugs shallow.\n> \n> I am inclined to apply the following on top. How does it look?\n\nIt looks good to me.\n\nBest Wishes\n\nPhillip\n\n> Thanks.\n> \n> ----- >8 --------- >8 --------- >8 --------- >8 -----\n> Subject: [PATCH] revert: --reference should apply only to 'revert', not 'cherry-pick'\n> \n> As 'revert' and 'cherry-pick' share a lot of code, it is easy to\n> modify the behaviour of one command and inadvertently affect the\n> other. An earlier change to teach the '--reference' option and the\n> 'revert.reference' configuration variable to the former was not\n> careful enough and 'cherry-pick --reference' wasn't rejected as an\n> error.\n> \n> It is possible to think 'cherry-pick -x' might benefit from the\n> '--reference' option, but it is fundamentally different from\n> 'revert' in at least two ways to make it questionable:\n> \n> - 'revert' names a commit that is ancestor of the resulting commit,\n> so an abbreviated object name with human readable title is\n> sufficient to identify the named commit uniquely without using\n> the full object name. On the other hand, 'cherry-pick'\n> usually [*] picks a commit that is not an ancestor. It might be\n> even picking a private commit that never becomes part of the\n> public history.\n> \n> - The whole commit message of 'cherry-pick' is a copy of the\n> original commit, and there is nothing gained to repeat only the\n> title part on 'cherry-picked from' message.\n> \n> [*] well, you could revert and then you can pick the original that\n> was reverted to get back to where you were, but then you can\n> revert the revert to do the same thing.\n> \n> Helped-by: Phillip Wood \n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/revert.c | 9 +++++++--\n> sequencer.c | 2 +-\n> t/t3501-revert-cherry-pick.sh | 6 ++++++\n> 3 files changed, 14 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index ada51e46b9..f84c253f4c 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -116,8 +116,6 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> \t\t\tN_(\"option for merge strategy\"), option_parse_x),\n> \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n> \t\t N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n> -\t\tOPT_BOOL(0, \"reference\", &opts->commit_use_reference,\n> -\t\t\t N_(\"use the 'reference' format to refer to commits\")),\n> \t\tOPT_END()\n> \t};\n> \tstruct option *options = base_options;\n> @@ -132,6 +130,13 @@ 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, \"reference\", &opts->commit_use_reference,\n> +\t\t\t\t N_(\"use the 'reference' format to refer to commits\")),\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,\n> diff --git a/sequencer.c b/sequencer.c\n> index 96fec6ef6d..4b66a1f79c 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -221,7 +221,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n> \t\treturn ret;\n> \t}\n> \n> -\tif (!strcmp(k, \"revert.reference\"))\n> +\tif (opts->action == REPLAY_REVERT && !strcmp(k, \"revert.reference\"))\n> \t\topts->commit_use_reference = git_config_bool(k, v);\n> \n> \tstatus = git_gpg_config(k, v, NULL);\n> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\n> index a386ae9e88..fb4466599b 100755\n> --- a/t/t3501-revert-cherry-pick.sh\n> +++ b/t/t3501-revert-cherry-pick.sh\n> @@ -205,4 +205,10 @@ test_expect_success 'identification of reverted commit (revert.reference)' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'cherry-pick is unaware of --reference (for now)' '\n> +\ttest_when_finished \"git reset --hard\" &&\n> +\ttest_must_fail git cherry-pick --reference HEAD 2>actual &&\n> +\tgrep \"^usage: git cherry-pick\" actual\n> +'\n> +\n> test_done\n"},{"id":"456447","messageId":"220601.86zgiwz9uk.gmgdl@evledraar.gmail.com","threadId":"57910","inReplyTo":"xmqq8rqn7buk.fsf_-_@gitster.g","subject":"Re: [PATCH v3] revert: optionally refer to commit in the \"reference\" format","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-01T15:14:41Z","receivedAt":"2022-06-01T15:23:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, May 26 2022, Junio C Hamano wrote:\n\n> When this option is in use, the first line of the pre-filled editor\n> buffer becomes a comment line that tells the user to say _why_. If\n> the user exits the editor without touching this line by mistake,\n> what we prepare to become the first line of the body, i.e. \"This\n> reverts commit 8fa7f667 (do this and that, 2022-04-25)\", ends up to\n> be the title of the resulting commit. This behaviour is designed to\n> help such a user to identify such a revert in \"git log --oneline\"\n> easily so that it can be further reworded with \"git rebase -i\" later.\n\nThis is a good trade-off, and means that the --no-edit case is also\npreserved. However...\n\n> @@ -2167,14 +2184,20 @@ static int do_pick_commit(struct repository *r,\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> +\t\tif (opts->commit_use_reference) {\n> +\t\t\tstrbuf_addstr(&msgbuf,\n> +\t\t\t\t\"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n> +\t\t} else {\n> +\t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n> +\t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n> +\t\t\tstrbuf_addstr(&msgbuf, \"\\\"\");\n> +\t\t}\n> +\t\tstrbuf_addstr(&msgbuf, \"\\n\\nThis reverts commit \");\n> +\t\trefer_to_commit(opts, &msgbuf, commit);\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\trefer_to_commit(opts, &msgbuf, parent);\n> \t\t}\n\n...the way this is implemented means that we end up with a much more\nverbose subject line in the case of reverts, which doesn't seem to be\nintended (or at least not called out in the commit message).\n\nI think a good solution to that would be to e.g. emit:\n\n # *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\n\n Reverts commit \n\n This revert of a merge reverts changes made to .\n\nInstead of what you have, which is:\n\n # *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\n\n This reverts commit , reversing\n changes made to .\n\nIt's sharing a bit less code between the two, but I think the message is\nsuffering for it now.\n\nI.e. the \"Revert \" change to \"This reverts commit\n\" doesn't per-se seem intentional, but just a side-effect of\nthe optional \"reference\" revert's default \"subject\" line piggy-backing\non the non-\"reference\" body.\n\n> +test_expect_success 'identification of reverted commit (default)' '\n> +\ttest_commit to-ident &&\n> +\ttest_when_finished \"git reset --hard to-ident\" &&\n> +\tgit checkout --detach to-ident &&\n> +\tgit revert --no-edit HEAD &&\n> +\tgit cat-file commit HEAD >actual.raw &&\n> +\tgrep \"^This reverts \" actual.raw >actual &&\n> +\techo \"This reverts commit $(git rev-parse HEAD^).\" >expect &&\n\nThis pattern hides git exit codes & segfaults (I don't remember if I\nmentioned that in a previous round, I think so...).\n\n> +test_expect_success 'identification of reverted commit (--reference)' '\n> +\tgit checkout --detach to-ident &&\n> +\tgit revert --reference --no-edit HEAD &&\n> +\tgit cat-file commit HEAD >actual.raw &&\n> +\tgrep \"^This reverts \" actual.raw >actual &&\n> +\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'identification of reverted commit (revert.reference)' '\n> +\tgit checkout --detach to-ident &&\n> +\tgit -c revert.reference=true revert --no-edit HEAD &&\n> +\tgit cat-file commit HEAD >actual.raw &&\n> +\tgrep \"^This reverts \" actual.raw >actual &&\n> +\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n> +\ttest_cmp expect actual\n\nAlso (probably mentioned) I'd find this much easier to read/review if it\nwas using test_cmp, now you need to carefully parse the code to see what\nthe outputs are like exactly, but if we compared the full output...\n"},{"id":"456472","messageId":"xmqqmtewoxu9.fsf@gitster.g","threadId":"57910","inReplyTo":"220601.86zgiwz9uk.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3] revert: optionally refer to commit in the \"reference\" format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-01T21:52:46Z","receivedAt":"2022-06-01T21:52:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I think a good solution to that would be to e.g. emit:\n>\n> # *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\n>\n> Reverts commit \n>\n> This revert of a merge reverts changes made to .\n>\n> Instead of what you have, which is:\n>\n> # *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\n>\n> This reverts commit , reversing\n> changes made to .\n>\n> It's sharing a bit less code between the two, but I think the message is\n> suffering for it now.\n\nWe shouldn't be making it inconvenient for our primary intended\naudience.\n\nThe real first line is designed to be usable without editing for\nthem. Those who forgets to write the title and ends up with \"This\nreverts ...\" as the title can still identify such a commit for the\npurpose of \"rebase -i\" and \"commit --fixup\", and that is good enough\nfor them.\n\nBut your version will force the intended audience to remove the\n\"Reverts ...\" line, that strikes the balance at the wrong place.\n\n>> +test_expect_success 'identification of reverted commit (revert.reference)' '\n>> +\tgit checkout --detach to-ident &&\n>> +\tgit -c revert.reference=true revert --no-edit HEAD &&\n>> +\tgit cat-file commit HEAD >actual.raw &&\n>> +\tgrep \"^This reverts \" actual.raw >actual &&\n>> +\techo \"This reverts commit $(git show -s --pretty=reference HEAD^).\" >expect &&\n>> +\ttest_cmp expect actual\n>\n> Also (probably mentioned) I'd find this much easier to read/review if it\n> was using test_cmp, now you need to carefully parse the code to see what\n> the outputs are like exactly, but if we compared the full output...\n\nWe are using test_cmp. 'actual' has what we care about (i.e. what\ndoes the line that begin with \"This reverts \" say?) and compares it\nwith what we expect to see in 'expect'.\n"},{"id":"457873","messageId":"6727daf1-f077-7319-187e-ab4e55de3b2d@web.de","threadId":"57910","inReplyTo":"xmqqsfp2b30k.fsf@gitster.g","subject":"Re: [PATCH] revert: optionally refer to commit in the \"reference\" format","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-26T09:29:35Z","receivedAt":"2022-06-26T09:29:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 22.05.22 um 06:32 schrieb Junio C Hamano:\n> ---\n> Documentation/config/revert.txt | 3 +++\n> Documentation/git-revert.txt | 9 +++++++++\n> builtin/revert.c | 2 ++\n> sequencer.c | 32 +++++++++++++++++++++++++++-----\n> sequencer.h | 1 +\n> t/t3501-revert-cherry-pick.sh | 31 +++++++++++++++++++++++++++++++\n> 6 files changed, 73 insertions(+), 5 deletions(-)\n> create mode 100644 Documentation/config/revert.txt\n>\n> diff --git a/Documentation/config/revert.txt b/Documentation/config/revert.txt\n> new file mode 100644\n> index 0000000000..797bfb6d62\n> --- /dev/null\n> +++ b/Documentation/config/revert.txt\n> @@ -0,0 +1,3 @@\n> +revert.reference::\n> +\tSetting this variable to true makes `git revert` to behave\n> +\tas if the `--reference` option is given.\n\nShouldn't this be \"were\" instead of \"is\"? Not fully sure. Anyway:\n\n--- >8 ---\nSubject: [PATCH] revert: config documentation fixes\n\n43966ab315 (revert: optionally refer to commit in the \"reference\"\nformat, 2022-05-26) added the documentation file config/revert.txt.\nActually include it in config.txt.\n\nMake is used with a bare infinitive after the object; remove the \"to\".\n\nSigned-off-by: René Scharfe \n---\n Documentation/config.txt | 2 ++\n Documentation/config/revert.txt | 2 +-\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex e284b042f2..e376d547ce 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -495,6 +495,8 @@ include::config/repack.txt[]\n\n include::config/rerere.txt[]\n\n+include::config/revert.txt[]\n+\n include::config/safe.txt[]\n\n include::config/sendemail.txt[]\ndiff --git a/Documentation/config/revert.txt b/Documentation/config/revert.txt\nindex 797bfb6d62..802d6faca2 100644\n--- a/Documentation/config/revert.txt\n+++ b/Documentation/config/revert.txt\n@@ -1,3 +1,3 @@\n revert.reference::\n-\tSetting this variable to true makes `git revert` to behave\n+\tSetting this variable to true makes `git revert` behave\n \tas if the `--reference` option is given.\n--\n2.36.1\n"},{"id":"457910","messageId":"xmqqletiqfq0.fsf@gitster.g","threadId":"57910","inReplyTo":"6727daf1-f077-7319-187e-ab4e55de3b2d@web.de","subject":"Re: [PATCH] revert: optionally refer to commit in the \"reference\" format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-27T15:38:15Z","receivedAt":"2022-06-27T15:38:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Actually include it in config.txt.\n>\n> Make is used with a bare infinitive after the object; remove the \"to\".\n\nThanks.\n"}]}