| Message ID | xmqq8rqn7buk.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 43966ab3156a082067bf9930351e96e9488da735 |
| Headers | show |
| Series | [v3] revert: optionally refer to commit in the "reference" format | expand |
Hi Junio, On Thu, 26 May 2022, Junio C Hamano wrote: > A typical "git revert" commit uses the full title of the original > commit in its title, and starts its body of the message with: > > This reverts commit 8fa7f667cf61386257c00d6e954855cc3215ae91. > > This does not encourage the best practice of describing not just > "what" (i.e. "Revert X" on the title says what we did) but "why" > (i.e. and it does not say why X was undesirable). > > We can instead phrase this first line of the body to be more like > > This reverts commit 8fa7f667 (do this and that, 2022-04-25) > > so that the title does not have to be > > Revert "do this and that" > > We can instead use the title to describe "why" we are reverting the > original commit. > > Introduce the "--reference" option to "git revert", and also the > revert.reference configuration variable, which defaults to false, to > tweak the title and the first line of the draft commit message for > when creating a "revert" commit. > > When this option is in use, the first line of the pre-filled editor > buffer becomes a comment line that tells the user to say _why_. If > the user exits the editor without touching this line by mistake, > what we prepare to become the first line of the body, i.e. "This > reverts commit 8fa7f667 (do this and that, 2022-04-25)", ends up to > be the title of the resulting commit. This behaviour is designed to > help such a user to identify such a revert in "git log --oneline" > easily so that it can be further reworded with "git rebase -i" later. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > > * The only difference since the second one is that the first line of > the log message template is commented out and has asterisks to > draw more attention. The last paragraph (new) in the proposed > log message explains the rationale behind this design. While it may be more purist to try to force the user to provide a description by having a non-comment placeholder there, from my experience with PullRequest/Issue templates convinces me that the comment is the more practical approach. Looking good, Dscho > > Third-time a charm, hopefully. > > Range-diff against v2: > 1: 4152fc2092 ! 1: f4325a503a revert: optionally refer to commit in the "reference" format > @@ Commit message > tweak the title and the first line of the draft commit message for > when creating a "revert" commit. > > + When this option is in use, the first line of the pre-filled editor > + buffer becomes a comment line that tells the user to say _why_. If > + the user exits the editor without touching this line by mistake, > + what we prepare to become the first line of the body, i.e. "This > + reverts commit 8fa7f667 (do this and that, 2022-04-25)", ends up to > + be the title of the resulting commit. This behaviour is designed to > + help such a user to identify such a revert in "git log --oneline" > + easily so that it can be further reworded with "git rebase -i" later. > + > Signed-off-by: Junio C Hamano <gitster@pobox.com> > > ## Documentation/config/revert.txt (new) ## > @@ sequencer.c: static int do_pick_commit(struct repository *r, > - strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit "); > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); > + if (opts->commit_use_reference) { > -+ strbuf_addstr(&msgbuf, "DESCRIBE WHY WE ARE REVERTING HERE"); > ++ strbuf_addstr(&msgbuf, > ++ "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***"); > + } else { > + strbuf_addstr(&msgbuf, "Revert \""); > + strbuf_addstr(&msgbuf, msg.subject); > > Documentation/config/revert.txt | 3 +++ > Documentation/git-revert.txt | 9 +++++++++ > builtin/revert.c | 2 ++ > sequencer.c | 33 ++++++++++++++++++++++++++++----- > sequencer.h | 1 + > t/t3501-revert-cherry-pick.sh | 31 +++++++++++++++++++++++++++++++ > 6 files changed, 74 insertions(+), 5 deletions(-) > create mode 100644 Documentation/config/revert.txt > > diff --git a/Documentation/config/revert.txt b/Documentation/config/revert.txt > new file mode 100644 > index 0000000000..797bfb6d62 > --- /dev/null > +++ b/Documentation/config/revert.txt > @@ -0,0 +1,3 @@ > +revert.reference:: > + Setting this variable to true makes `git revert` to behave > + as if the `--reference` option is given. > diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt > index bb92a4a451..8463fe9cf7 100644 > --- a/Documentation/git-revert.txt > +++ b/Documentation/git-revert.txt > @@ -117,6 +117,15 @@ effect to your index in a row. > Allow the rerere mechanism to update the index with the > result of auto-conflict resolution if possible. > > +--reference:: > + Instead of starting the body of the log message with "This > + reverts <full object name of the commit being reverted>.", > + refer to the commit using "--pretty=reference" format > + (cf. linkgit:git-log[1]). The `revert.reference` > + configuration variable can be used to enable this option by > + default. > + > + > SEQUENCER SUBCOMMANDS > --------------------- > include::sequencer.txt[] > diff --git a/builtin/revert.c b/builtin/revert.c > index 51776abea6..ada51e46b9 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -116,6 +116,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > N_("option for merge strategy"), option_parse_x), > { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"), > N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, > + OPT_BOOL(0, "reference", &opts->commit_use_reference, > + N_("use the 'reference' format to refer to commits")), > OPT_END() > }; > struct option *options = base_options; > diff --git a/sequencer.c b/sequencer.c > index a5f678f452..96fec6ef6d 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -221,6 +221,9 @@ static int git_sequencer_config(const char *k, const char *v, void *cb) > return ret; > } > > + if (!strcmp(k, "revert.reference")) > + opts->commit_use_reference = git_config_bool(k, v); > + > status = git_gpg_config(k, v, NULL); > if (status) > return status; > @@ -2059,6 +2062,20 @@ static int should_edit(struct replay_opts *opts) { > return opts->edit; > } > > +static void refer_to_commit(struct replay_opts *opts, > + struct strbuf *msgbuf, struct commit *commit) > +{ > + if (opts->commit_use_reference) { > + struct pretty_print_context ctx = { > + .abbrev = DEFAULT_ABBREV, > + .date_mode.type = DATE_SHORT, > + }; > + format_commit_message(commit, "%h (%s, %ad)", msgbuf, &ctx); > + } else { > + strbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid)); > + } > +} > + > static int do_pick_commit(struct repository *r, > struct todo_item *item, > struct replay_opts *opts, > @@ -2167,14 +2184,20 @@ static int do_pick_commit(struct repository *r, > base_label = msg.label; > next = parent; > next_label = msg.parent_label; > - strbuf_addstr(&msgbuf, "Revert \""); > - strbuf_addstr(&msgbuf, msg.subject); > - strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit "); > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); > + if (opts->commit_use_reference) { > + strbuf_addstr(&msgbuf, > + "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***"); > + } else { > + strbuf_addstr(&msgbuf, "Revert \""); > + strbuf_addstr(&msgbuf, msg.subject); > + strbuf_addstr(&msgbuf, "\""); > + } > + strbuf_addstr(&msgbuf, "\n\nThis reverts commit "); > + refer_to_commit(opts, &msgbuf, commit); > > if (commit->parents && commit->parents->next) { > strbuf_addstr(&msgbuf, ", reversing\nchanges made to "); > - strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid)); > + refer_to_commit(opts, &msgbuf, parent); > } > strbuf_addstr(&msgbuf, ".\n"); > } else { > diff --git a/sequencer.h b/sequencer.h > index da64473636..698599fe4e 100644 > --- a/sequencer.h > +++ b/sequencer.h > @@ -49,6 +49,7 @@ struct replay_opts { > int reschedule_failed_exec; > int committer_date_is_author_date; > int ignore_date; > + int commit_use_reference; > > int mainline; > > diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh > index 8617efaaf1..a386ae9e88 100755 > --- a/t/t3501-revert-cherry-pick.sh > +++ b/t/t3501-revert-cherry-pick.sh > @@ -159,6 +159,7 @@ test_expect_success 'cherry-pick works with dirty renamed file' ' > ' > > test_expect_success 'advice from failed revert' ' > + test_when_finished "git reset --hard" && > test_commit --no-tag "add dream" dream dream && > dream_oid=$(git rev-parse --short HEAD) && > cat <<-EOF >expected && > @@ -174,4 +175,34 @@ test_expect_success 'advice from failed revert' ' > test_must_fail git revert HEAD^ 2>actual && > test_cmp expected actual > ' > + > +test_expect_success 'identification of reverted commit (default)' ' > + test_commit to-ident && > + test_when_finished "git reset --hard to-ident" && > + git checkout --detach to-ident && > + git revert --no-edit HEAD && > + git cat-file commit HEAD >actual.raw && > + grep "^This reverts " actual.raw >actual && > + echo "This reverts commit $(git rev-parse HEAD^)." >expect && > + test_cmp expect actual > +' > + > +test_expect_success 'identification of reverted commit (--reference)' ' > + git checkout --detach to-ident && > + git revert --reference --no-edit HEAD && > + git cat-file commit HEAD >actual.raw && > + grep "^This reverts " actual.raw >actual && > + echo "This reverts commit $(git show -s --pretty=reference HEAD^)." >expect && > + test_cmp expect actual > +' > + > +test_expect_success 'identification of reverted commit (revert.reference)' ' > + git checkout --detach to-ident && > + git -c revert.reference=true revert --no-edit HEAD && > + git cat-file commit HEAD >actual.raw && > + grep "^This reverts " actual.raw >actual && > + echo "This reverts commit $(git show -s --pretty=reference HEAD^)." >expect && > + test_cmp expect actual > +' > + > test_done > -- > 2.36.1-331-g1b5d92e060 > >
Hi Junio On 27/05/2022 07:01, Junio C Hamano wrote: > A typical "git revert" commit uses the full title of the original > commit in its title, and starts its body of the message with: > > This reverts commit 8fa7f667cf61386257c00d6e954855cc3215ae91. > > This does not encourage the best practice of describing not just > "what" (i.e. "Revert X" on the title says what we did) but "why" > (i.e. and it does not say why X was undesirable). > > We can instead phrase this first line of the body to be more like > > This reverts commit 8fa7f667 (do this and that, 2022-04-25) > > so that the title does not have to be > > Revert "do this and that" > > We can instead use the title to describe "why" we are reverting the > original commit. > > Introduce the "--reference" option to "git revert", and also the > revert.reference configuration variable, which defaults to false, to > tweak the title and the first line of the draft commit message for > when creating a "revert" commit. > > When this option is in use, the first line of the pre-filled editor > buffer becomes a comment line that tells the user to say _why_. If > the user exits the editor without touching this line by mistake, > what we prepare to become the first line of the body, i.e. "This > reverts commit 8fa7f667 (do this and that, 2022-04-25)", ends up to > be the title of the resulting commit. This behaviour is designed to > help such a user to identify such a revert in "git log --oneline" > easily so that it can be further reworded with "git rebase -i" later. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > > * The only difference since the second one is that the first line of > the log message template is commented out and has asterisks to > draw more attention. The last paragraph (new) in the proposed > log message explains the rationale behind this design. > > Third-time a charm, hopefully. I think the changes to the template message are good. We're still adding "--reference" as a valid option to cherry-pick though which I don't think is a good idea (though in the future we may want to allow "cherry-pick -x --reference") Best Wishes Phillip > Range-diff against v2: > 1: 4152fc2092 ! 1: f4325a503a revert: optionally refer to commit in the "reference" format > @@ Commit message > tweak the title and the first line of the draft commit message for > when creating a "revert" commit. > > + When this option is in use, the first line of the pre-filled editor > + buffer becomes a comment line that tells the user to say _why_. If > + the user exits the editor without touching this line by mistake, > + what we prepare to become the first line of the body, i.e. "This > + reverts commit 8fa7f667 (do this and that, 2022-04-25)", ends up to > + be the title of the resulting commit. This behaviour is designed to > + help such a user to identify such a revert in "git log --oneline" > + easily so that it can be further reworded with "git rebase -i" later. > + > Signed-off-by: Junio C Hamano <gitster@pobox.com> > > ## Documentation/config/revert.txt (new) ## > @@ sequencer.c: static int do_pick_commit(struct repository *r, > - strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit "); > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); > + if (opts->commit_use_reference) { > -+ strbuf_addstr(&msgbuf, "DESCRIBE WHY WE ARE REVERTING HERE"); > ++ strbuf_addstr(&msgbuf, > ++ "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***"); > + } else { > + strbuf_addstr(&msgbuf, "Revert \""); > + strbuf_addstr(&msgbuf, msg.subject); > > Documentation/config/revert.txt | 3 +++ > Documentation/git-revert.txt | 9 +++++++++ > builtin/revert.c | 2 ++ > sequencer.c | 33 ++++++++++++++++++++++++++++----- > sequencer.h | 1 + > t/t3501-revert-cherry-pick.sh | 31 +++++++++++++++++++++++++++++++ > 6 files changed, 74 insertions(+), 5 deletions(-) > create mode 100644 Documentation/config/revert.txt > > diff --git a/Documentation/config/revert.txt b/Documentation/config/revert.txt > new file mode 100644 > index 0000000000..797bfb6d62 > --- /dev/null > +++ b/Documentation/config/revert.txt > @@ -0,0 +1,3 @@ > +revert.reference:: > + Setting this variable to true makes `git revert` to behave > + as if the `--reference` option is given. > diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt > index bb92a4a451..8463fe9cf7 100644 > --- a/Documentation/git-revert.txt > +++ b/Documentation/git-revert.txt > @@ -117,6 +117,15 @@ effect to your index in a row. > Allow the rerere mechanism to update the index with the > result of auto-conflict resolution if possible. > > +--reference:: > + Instead of starting the body of the log message with "This > + reverts <full object name of the commit being reverted>.", > + refer to the commit using "--pretty=reference" format > + (cf. linkgit:git-log[1]). The `revert.reference` > + configuration variable can be used to enable this option by > + default. > + > + > SEQUENCER SUBCOMMANDS > --------------------- > include::sequencer.txt[] > diff --git a/builtin/revert.c b/builtin/revert.c > index 51776abea6..ada51e46b9 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -116,6 +116,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > N_("option for merge strategy"), option_parse_x), > { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"), > N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, > + OPT_BOOL(0, "reference", &opts->commit_use_reference, > + N_("use the 'reference' format to refer to commits")), > OPT_END() > }; > struct option *options = base_options; > diff --git a/sequencer.c b/sequencer.c > index a5f678f452..96fec6ef6d 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -221,6 +221,9 @@ static int git_sequencer_config(const char *k, const char *v, void *cb) > return ret; > } > > + if (!strcmp(k, "revert.reference")) > + opts->commit_use_reference = git_config_bool(k, v); > + > status = git_gpg_config(k, v, NULL); > if (status) > return status; > @@ -2059,6 +2062,20 @@ static int should_edit(struct replay_opts *opts) { > return opts->edit; > } > > +static void refer_to_commit(struct replay_opts *opts, > + struct strbuf *msgbuf, struct commit *commit) > +{ > + if (opts->commit_use_reference) { > + struct pretty_print_context ctx = { > + .abbrev = DEFAULT_ABBREV, > + .date_mode.type = DATE_SHORT, > + }; > + format_commit_message(commit, "%h (%s, %ad)", msgbuf, &ctx); > + } else { > + strbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid)); > + } > +} > + > static int do_pick_commit(struct repository *r, > struct todo_item *item, > struct replay_opts *opts, > @@ -2167,14 +2184,20 @@ static int do_pick_commit(struct repository *r, > base_label = msg.label; > next = parent; > next_label = msg.parent_label; > - strbuf_addstr(&msgbuf, "Revert \""); > - strbuf_addstr(&msgbuf, msg.subject); > - strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit "); > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); > + if (opts->commit_use_reference) { > + strbuf_addstr(&msgbuf, > + "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***"); > + } else { > + strbuf_addstr(&msgbuf, "Revert \""); > + strbuf_addstr(&msgbuf, msg.subject); > + strbuf_addstr(&msgbuf, "\""); > + } > + strbuf_addstr(&msgbuf, "\n\nThis reverts commit "); > + refer_to_commit(opts, &msgbuf, commit); > > if (commit->parents && commit->parents->next) { > strbuf_addstr(&msgbuf, ", reversing\nchanges made to "); > - strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid)); > + refer_to_commit(opts, &msgbuf, parent); > } > strbuf_addstr(&msgbuf, ".\n"); > } else { > diff --git a/sequencer.h b/sequencer.h > index da64473636..698599fe4e 100644 > --- a/sequencer.h > +++ b/sequencer.h > @@ -49,6 +49,7 @@ struct replay_opts { > int reschedule_failed_exec; > int committer_date_is_author_date; > int ignore_date; > + int commit_use_reference; > > int mainline; > > diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh > index 8617efaaf1..a386ae9e88 100755 > --- a/t/t3501-revert-cherry-pick.sh > +++ b/t/t3501-revert-cherry-pick.sh > @@ -159,6 +159,7 @@ test_expect_success 'cherry-pick works with dirty renamed file' ' > ' > > test_expect_success 'advice from failed revert' ' > + test_when_finished "git reset --hard" && > test_commit --no-tag "add dream" dream dream && > dream_oid=$(git rev-parse --short HEAD) && > cat <<-EOF >expected && > @@ -174,4 +175,34 @@ test_expect_success 'advice from failed revert' ' > test_must_fail git revert HEAD^ 2>actual && > test_cmp expected actual > ' > + > +test_expect_success 'identification of reverted commit (default)' ' > + test_commit to-ident && > + test_when_finished "git reset --hard to-ident" && > + git checkout --detach to-ident && > + git revert --no-edit HEAD && > + git cat-file commit HEAD >actual.raw && > + grep "^This reverts " actual.raw >actual && > + echo "This reverts commit $(git rev-parse HEAD^)." >expect && > + test_cmp expect actual > +' > + > +test_expect_success 'identification of reverted commit (--reference)' ' > + git checkout --detach to-ident && > + git revert --reference --no-edit HEAD && > + git cat-file commit HEAD >actual.raw && > + grep "^This reverts " actual.raw >actual && > + echo "This reverts commit $(git show -s --pretty=reference HEAD^)." >expect && > + test_cmp expect actual > +' > + > +test_expect_success 'identification of reverted commit (revert.reference)' ' > + git checkout --detach to-ident && > + git -c revert.reference=true revert --no-edit HEAD && > + git cat-file commit HEAD >actual.raw && > + grep "^This reverts " actual.raw >actual && > + echo "This reverts commit $(git show -s --pretty=reference HEAD^)." >expect && > + test_cmp expect actual > +' > + > test_done
Phillip Wood <phillip.wood123@gmail.com> writes: > I think the changes to the template message are good. We're still > adding "--reference" as a valid option to cherry-pick though which I > don't think is a good idea (though in the future we may want to allow > "cherry-pick -x --reference") I love when people notice mistakes that the original author and other people missed, many eyes making all bugs shallow. I am inclined to apply the following on top. How does it look? Thanks. ----- >8 --------- >8 --------- >8 --------- >8 ----- Subject: [PATCH] revert: --reference should apply only to 'revert', not 'cherry-pick' As 'revert' and 'cherry-pick' share a lot of code, it is easy to modify the behaviour of one command and inadvertently affect the other. An earlier change to teach the '--reference' option and the 'revert.reference' configuration variable to the former was not careful enough and 'cherry-pick --reference' wasn't rejected as an error. It is possible to think 'cherry-pick -x' might benefit from the '--reference' option, but it is fundamentally different from 'revert' in at least two ways to make it questionable: - 'revert' names a commit that is ancestor of the resulting commit, so an abbreviated object name with human readable title is sufficient to identify the named commit uniquely without using the full object name. On the other hand, 'cherry-pick' usually [*] picks a commit that is not an ancestor. It might be even picking a private commit that never becomes part of the public history. - The whole commit message of 'cherry-pick' is a copy of the original commit, and there is nothing gained to repeat only the title part on 'cherry-picked from' message. [*] well, you could revert and then you can pick the original that was reverted to get back to where you were, but then you can revert the revert to do the same thing. Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/revert.c | 9 +++++++-- sequencer.c | 2 +- t/t3501-revert-cherry-pick.sh | 6 ++++++ 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index ada51e46b9..f84c253f4c 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -116,8 +116,6 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) N_("option for merge strategy"), option_parse_x), { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"), N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, - OPT_BOOL(0, "reference", &opts->commit_use_reference, - N_("use the 'reference' format to refer to commits")), OPT_END() }; struct option *options = base_options; @@ -132,6 +130,13 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) OPT_END(), }; options = parse_options_concat(options, cp_extra); + } else if (opts->action == REPLAY_REVERT) { + struct option cp_extra[] = { + OPT_BOOL(0, "reference", &opts->commit_use_reference, + N_("use the 'reference' format to refer to commits")), + OPT_END(), + }; + options = parse_options_concat(options, cp_extra); } argc = parse_options(argc, argv, NULL, options, usage_str, diff --git a/sequencer.c b/sequencer.c index 96fec6ef6d..4b66a1f79c 100644 --- a/sequencer.c +++ b/sequencer.c @@ -221,7 +221,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb) return ret; } - if (!strcmp(k, "revert.reference")) + if (opts->action == REPLAY_REVERT && !strcmp(k, "revert.reference")) opts->commit_use_reference = git_config_bool(k, v); status = git_gpg_config(k, v, NULL); diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh index a386ae9e88..fb4466599b 100755 --- a/t/t3501-revert-cherry-pick.sh +++ b/t/t3501-revert-cherry-pick.sh @@ -205,4 +205,10 @@ test_expect_success 'identification of reverted commit (revert.reference)' ' test_cmp expect actual ' +test_expect_success 'cherry-pick is unaware of --reference (for now)' ' + test_when_finished "git reset --hard" && + test_must_fail git cherry-pick --reference HEAD 2>actual && + grep "^usage: git cherry-pick" actual +' + test_done
Hi Junio On 01/06/2022 05:45, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> I think the changes to the template message are good. We're still >> adding "--reference" as a valid option to cherry-pick though which I >> don't think is a good idea (though in the future we may want to allow >> "cherry-pick -x --reference") > > I love when people notice mistakes that the original author and > other people missed, many eyes making all bugs shallow. > > I am inclined to apply the following on top. How does it look? It looks good to me. Best Wishes Phillip > Thanks. > > ----- >8 --------- >8 --------- >8 --------- >8 ----- > Subject: [PATCH] revert: --reference should apply only to 'revert', not 'cherry-pick' > > As 'revert' and 'cherry-pick' share a lot of code, it is easy to > modify the behaviour of one command and inadvertently affect the > other. An earlier change to teach the '--reference' option and the > 'revert.reference' configuration variable to the former was not > careful enough and 'cherry-pick --reference' wasn't rejected as an > error. > > It is possible to think 'cherry-pick -x' might benefit from the > '--reference' option, but it is fundamentally different from > 'revert' in at least two ways to make it questionable: > > - 'revert' names a commit that is ancestor of the resulting commit, > so an abbreviated object name with human readable title is > sufficient to identify the named commit uniquely without using > the full object name. On the other hand, 'cherry-pick' > usually [*] picks a commit that is not an ancestor. It might be > even picking a private commit that never becomes part of the > public history. > > - The whole commit message of 'cherry-pick' is a copy of the > original commit, and there is nothing gained to repeat only the > title part on 'cherry-picked from' message. > > [*] well, you could revert and then you can pick the original that > was reverted to get back to where you were, but then you can > revert the revert to do the same thing. > > Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > builtin/revert.c | 9 +++++++-- > sequencer.c | 2 +- > t/t3501-revert-cherry-pick.sh | 6 ++++++ > 3 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/builtin/revert.c b/builtin/revert.c > index ada51e46b9..f84c253f4c 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -116,8 +116,6 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > N_("option for merge strategy"), option_parse_x), > { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"), > N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, > - OPT_BOOL(0, "reference", &opts->commit_use_reference, > - N_("use the 'reference' format to refer to commits")), > OPT_END() > }; > struct option *options = base_options; > @@ -132,6 +130,13 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > OPT_END(), > }; > options = parse_options_concat(options, cp_extra); > + } else if (opts->action == REPLAY_REVERT) { > + struct option cp_extra[] = { > + OPT_BOOL(0, "reference", &opts->commit_use_reference, > + N_("use the 'reference' format to refer to commits")), > + OPT_END(), > + }; > + options = parse_options_concat(options, cp_extra); > } > > argc = parse_options(argc, argv, NULL, options, usage_str, > diff --git a/sequencer.c b/sequencer.c > index 96fec6ef6d..4b66a1f79c 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -221,7 +221,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb) > return ret; > } > > - if (!strcmp(k, "revert.reference")) > + if (opts->action == REPLAY_REVERT && !strcmp(k, "revert.reference")) > opts->commit_use_reference = git_config_bool(k, v); > > status = git_gpg_config(k, v, NULL); > diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh > index a386ae9e88..fb4466599b 100755 > --- a/t/t3501-revert-cherry-pick.sh > +++ b/t/t3501-revert-cherry-pick.sh > @@ -205,4 +205,10 @@ test_expect_success 'identification of reverted commit (revert.reference)' ' > test_cmp expect actual > ' > > +test_expect_success 'cherry-pick is unaware of --reference (for now)' ' > + test_when_finished "git reset --hard" && > + test_must_fail git cherry-pick --reference HEAD 2>actual && > + grep "^usage: git cherry-pick" actual > +' > + > test_done
On Thu, May 26 2022, Junio C Hamano wrote: > When this option is in use, the first line of the pre-filled editor > buffer becomes a comment line that tells the user to say _why_. If > the user exits the editor without touching this line by mistake, > what we prepare to become the first line of the body, i.e. "This > reverts commit 8fa7f667 (do this and that, 2022-04-25)", ends up to > be the title of the resulting commit. This behaviour is designed to > help such a user to identify such a revert in "git log --oneline" > easily so that it can be further reworded with "git rebase -i" later. This is a good trade-off, and means that the --no-edit case is also preserved. However... > @@ -2167,14 +2184,20 @@ static int do_pick_commit(struct repository *r, > base_label = msg.label; > next = parent; > next_label = msg.parent_label; > - strbuf_addstr(&msgbuf, "Revert \""); > - strbuf_addstr(&msgbuf, msg.subject); > - strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit "); > - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); > + if (opts->commit_use_reference) { > + strbuf_addstr(&msgbuf, > + "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***"); > + } else { > + strbuf_addstr(&msgbuf, "Revert \""); > + strbuf_addstr(&msgbuf, msg.subject); > + strbuf_addstr(&msgbuf, "\""); > + } > + strbuf_addstr(&msgbuf, "\n\nThis reverts commit "); > + refer_to_commit(opts, &msgbuf, commit); > > if (commit->parents && commit->parents->next) { > strbuf_addstr(&msgbuf, ", reversing\nchanges made to "); > - strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid)); > + refer_to_commit(opts, &msgbuf, parent); > } ...the way this is implemented means that we end up with a much more verbose subject line in the case of reverts, which doesn't seem to be intended (or at least not called out in the commit message). I think a good solution to that would be to e.g. emit: # *** SAY WHY WE ARE REVERTING ON THE TITLE LINE *** Reverts commit <git reference> This revert of a merge reverts changes made to <git reference 2>. Instead of what you have, which is: # *** SAY WHY WE ARE REVERTING ON THE TITLE LINE *** This reverts commit <git reference>, reversing changes made to <git reference 2>. It's sharing a bit less code between the two, but I think the message is suffering for it now. I.e. the "Revert <reference>" change to "This reverts commit <reference>" doesn't per-se seem intentional, but just a side-effect of the optional "reference" revert's default "subject" line piggy-backing on the non-"reference" body. > +test_expect_success 'identification of reverted commit (default)' ' > + test_commit to-ident && > + test_when_finished "git reset --hard to-ident" && > + git checkout --detach to-ident && > + git revert --no-edit HEAD && > + git cat-file commit HEAD >actual.raw && > + grep "^This reverts " actual.raw >actual && > + echo "This reverts commit $(git rev-parse HEAD^)." >expect && This pattern hides git exit codes & segfaults (I don't remember if I mentioned that in a previous round, I think so...). > +test_expect_success 'identification of reverted commit (--reference)' ' > + git checkout --detach to-ident && > + git revert --reference --no-edit HEAD && > + git cat-file commit HEAD >actual.raw && > + grep "^This reverts " actual.raw >actual && > + echo "This reverts commit $(git show -s --pretty=reference HEAD^)." >expect && > + test_cmp expect actual > +' > + > +test_expect_success 'identification of reverted commit (revert.reference)' ' > + git checkout --detach to-ident && > + git -c revert.reference=true revert --no-edit HEAD && > + git cat-file commit HEAD >actual.raw && > + grep "^This reverts " actual.raw >actual && > + echo "This reverts commit $(git show -s --pretty=reference HEAD^)." >expect && > + test_cmp expect actual Also (probably mentioned) I'd find this much easier to read/review if it was using test_cmp, now you need to carefully parse the code to see what the outputs are like exactly, but if we compared the full output...
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I think a good solution to that would be to e.g. emit: > > # *** SAY WHY WE ARE REVERTING ON THE TITLE LINE *** > > Reverts commit <git reference> > > This revert of a merge reverts changes made to <git reference 2>. > > Instead of what you have, which is: > > # *** SAY WHY WE ARE REVERTING ON THE TITLE LINE *** > > This reverts commit <git reference>, reversing > changes made to <git reference 2>. > > It's sharing a bit less code between the two, but I think the message is > suffering for it now. We shouldn't be making it inconvenient for our primary intended audience. The real first line is designed to be usable without editing for them. Those who forgets to write the title and ends up with "This reverts ..." as the title can still identify such a commit for the purpose of "rebase -i" and "commit --fixup", and that is good enough for them. But your version will force the intended audience to remove the "Reverts ..." line, that strikes the balance at the wrong place. >> +test_expect_success 'identification of reverted commit (revert.reference)' ' >> + git checkout --detach to-ident && >> + git -c revert.reference=true revert --no-edit HEAD && >> + git cat-file commit HEAD >actual.raw && >> + grep "^This reverts " actual.raw >actual && >> + echo "This reverts commit $(git show -s --pretty=reference HEAD^)." >expect && >> + test_cmp expect actual > > Also (probably mentioned) I'd find this much easier to read/review if it > was using test_cmp, now you need to carefully parse the code to see what > the outputs are like exactly, but if we compared the full output... We are using test_cmp. 'actual' has what we care about (i.e. what does the line that begin with "This reverts " say?) and compares it with what we expect to see in 'expect'.
diff --git a/Documentation/config/revert.txt b/Documentation/config/revert.txt new file mode 100644 index 0000000000..797bfb6d62 --- /dev/null +++ b/Documentation/config/revert.txt @@ -0,0 +1,3 @@ +revert.reference:: + Setting this variable to true makes `git revert` to behave + as if the `--reference` option is given. diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt index bb92a4a451..8463fe9cf7 100644 --- a/Documentation/git-revert.txt +++ b/Documentation/git-revert.txt @@ -117,6 +117,15 @@ effect to your index in a row. Allow the rerere mechanism to update the index with the result of auto-conflict resolution if possible. +--reference:: + Instead of starting the body of the log message with "This + reverts <full object name of the commit being reverted>.", + refer to the commit using "--pretty=reference" format + (cf. linkgit:git-log[1]). The `revert.reference` + configuration variable can be used to enable this option by + default. + + SEQUENCER SUBCOMMANDS --------------------- include::sequencer.txt[] diff --git a/builtin/revert.c b/builtin/revert.c index 51776abea6..ada51e46b9 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -116,6 +116,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) N_("option for merge strategy"), option_parse_x), { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"), N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, + OPT_BOOL(0, "reference", &opts->commit_use_reference, + N_("use the 'reference' format to refer to commits")), OPT_END() }; struct option *options = base_options; diff --git a/sequencer.c b/sequencer.c index a5f678f452..96fec6ef6d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -221,6 +221,9 @@ static int git_sequencer_config(const char *k, const char *v, void *cb) return ret; } + if (!strcmp(k, "revert.reference")) + opts->commit_use_reference = git_config_bool(k, v); + status = git_gpg_config(k, v, NULL); if (status) return status; @@ -2059,6 +2062,20 @@ static int should_edit(struct replay_opts *opts) { return opts->edit; } +static void refer_to_commit(struct replay_opts *opts, + struct strbuf *msgbuf, struct commit *commit) +{ + if (opts->commit_use_reference) { + struct pretty_print_context ctx = { + .abbrev = DEFAULT_ABBREV, + .date_mode.type = DATE_SHORT, + }; + format_commit_message(commit, "%h (%s, %ad)", msgbuf, &ctx); + } else { + strbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid)); + } +} + static int do_pick_commit(struct repository *r, struct todo_item *item, struct replay_opts *opts, @@ -2167,14 +2184,20 @@ static int do_pick_commit(struct repository *r, base_label = msg.label; next = parent; next_label = msg.parent_label; - strbuf_addstr(&msgbuf, "Revert \""); - strbuf_addstr(&msgbuf, msg.subject); - strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit "); - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); + if (opts->commit_use_reference) { + strbuf_addstr(&msgbuf, + "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***"); + } else { + strbuf_addstr(&msgbuf, "Revert \""); + strbuf_addstr(&msgbuf, msg.subject); + strbuf_addstr(&msgbuf, "\""); + } + strbuf_addstr(&msgbuf, "\n\nThis reverts commit "); + refer_to_commit(opts, &msgbuf, commit); if (commit->parents && commit->parents->next) { strbuf_addstr(&msgbuf, ", reversing\nchanges made to "); - strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid)); + refer_to_commit(opts, &msgbuf, parent); } strbuf_addstr(&msgbuf, ".\n"); } else { diff --git a/sequencer.h b/sequencer.h index da64473636..698599fe4e 100644 --- a/sequencer.h +++ b/sequencer.h @@ -49,6 +49,7 @@ struct replay_opts { int reschedule_failed_exec; int committer_date_is_author_date; int ignore_date; + int commit_use_reference; int mainline; diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh index 8617efaaf1..a386ae9e88 100755 --- a/t/t3501-revert-cherry-pick.sh +++ b/t/t3501-revert-cherry-pick.sh @@ -159,6 +159,7 @@ test_expect_success 'cherry-pick works with dirty renamed file' ' ' test_expect_success 'advice from failed revert' ' + test_when_finished "git reset --hard" && test_commit --no-tag "add dream" dream dream && dream_oid=$(git rev-parse --short HEAD) && cat <<-EOF >expected && @@ -174,4 +175,34 @@ test_expect_success 'advice from failed revert' ' test_must_fail git revert HEAD^ 2>actual && test_cmp expected actual ' + +test_expect_success 'identification of reverted commit (default)' ' + test_commit to-ident && + test_when_finished "git reset --hard to-ident" && + git checkout --detach to-ident && + git revert --no-edit HEAD && + git cat-file commit HEAD >actual.raw && + grep "^This reverts " actual.raw >actual && + echo "This reverts commit $(git rev-parse HEAD^)." >expect && + test_cmp expect actual +' + +test_expect_success 'identification of reverted commit (--reference)' ' + git checkout --detach to-ident && + git revert --reference --no-edit HEAD && + git cat-file commit HEAD >actual.raw && + grep "^This reverts " actual.raw >actual && + echo "This reverts commit $(git show -s --pretty=reference HEAD^)." >expect && + test_cmp expect actual +' + +test_expect_success 'identification of reverted commit (revert.reference)' ' + git checkout --detach to-ident && + git -c revert.reference=true revert --no-edit HEAD && + git cat-file commit HEAD >actual.raw && + grep "^This reverts " actual.raw >actual && + echo "This reverts commit $(git show -s --pretty=reference HEAD^)." >expect && + test_cmp expect actual +' + test_done
A typical "git revert" commit uses the full title of the original commit in its title, and starts its body of the message with: This reverts commit 8fa7f667cf61386257c00d6e954855cc3215ae91. This does not encourage the best practice of describing not just "what" (i.e. "Revert X" on the title says what we did) but "why" (i.e. and it does not say why X was undesirable). We can instead phrase this first line of the body to be more like This reverts commit 8fa7f667 (do this and that, 2022-04-25) so that the title does not have to be Revert "do this and that" We can instead use the title to describe "why" we are reverting the original commit. Introduce the "--reference" option to "git revert", and also the revert.reference configuration variable, which defaults to false, to tweak the title and the first line of the draft commit message for when creating a "revert" commit. When this option is in use, the first line of the pre-filled editor buffer becomes a comment line that tells the user to say _why_. If the user exits the editor without touching this line by mistake, what we prepare to become the first line of the body, i.e. "This reverts commit 8fa7f667 (do this and that, 2022-04-25)", ends up to be the title of the resulting commit. This behaviour is designed to help such a user to identify such a revert in "git log --oneline" easily so that it can be further reworded with "git rebase -i" later. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * The only difference since the second one is that the first line of the log message template is commented out and has asterisks to draw more attention. The last paragraph (new) in the proposed log message explains the rationale behind this design. Third-time a charm, hopefully. Range-diff against v2: 1: 4152fc2092 ! 1: f4325a503a revert: optionally refer to commit in the "reference" format @@ Commit message tweak the title and the first line of the draft commit message for when creating a "revert" commit. + When this option is in use, the first line of the pre-filled editor + buffer becomes a comment line that tells the user to say _why_. If + the user exits the editor without touching this line by mistake, + what we prepare to become the first line of the body, i.e. "This + reverts commit 8fa7f667 (do this and that, 2022-04-25)", ends up to + be the title of the resulting commit. This behaviour is designed to + help such a user to identify such a revert in "git log --oneline" + easily so that it can be further reworded with "git rebase -i" later. + Signed-off-by: Junio C Hamano <gitster@pobox.com> ## Documentation/config/revert.txt (new) ## @@ sequencer.c: static int do_pick_commit(struct repository *r, - strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit "); - strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); + if (opts->commit_use_reference) { -+ strbuf_addstr(&msgbuf, "DESCRIBE WHY WE ARE REVERTING HERE"); ++ strbuf_addstr(&msgbuf, ++ "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***"); + } else { + strbuf_addstr(&msgbuf, "Revert \""); + strbuf_addstr(&msgbuf, msg.subject); Documentation/config/revert.txt | 3 +++ Documentation/git-revert.txt | 9 +++++++++ builtin/revert.c | 2 ++ sequencer.c | 33 ++++++++++++++++++++++++++++----- sequencer.h | 1 + t/t3501-revert-cherry-pick.sh | 31 +++++++++++++++++++++++++++++++ 6 files changed, 74 insertions(+), 5 deletions(-) create mode 100644 Documentation/config/revert.txt