| Message ID | xmqqsfp2b30k.fsf@gitster.g (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | revert: optionally refer to commit in the "reference" format | expand |
Hi Junio, On Sat, 21 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. Good idea, the `reference` text is so much more pleasant to read (and it also makes things somewhat proof against rebasing and transitioning to SHA-256). > 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. At some stage, we may even default to `revert.reference=true`? > 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. > + > + Is that extra empty line intentional? > 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..97abd5932b 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); It is somewhat sad that we have to semi-duplicate [*1*] this format here and do not have something like an enum value we can use instead. > + } 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,19 @@ 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, "Revert \""); > + strbuf_addstr(&msgbuf, msg.subject); > + strbuf_addstr(&msgbuf, "\""); > + } else { > + strbuf_addstr(&msgbuf, "DESCRIBE WHY WE ARE REVERTING HERE"); > + } We often tell newcomers that we prefer the shorter conditional block to come first. Maybe you want to do that here, too? I don't mind the current form, though. > + 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..36f9565b92 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 (vanilla)' ' It might make sense to replace "vanilla" by "default format". > + 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 && I was a bit puzzled about this dance, as I had expected this instead: grep "^This reverts commit $(git rev-parse HEAD^)\\.$" actual.raw but I guess you wanted to make sure that the file contents of `actual` are shown if no match was found? If that was the intention, we may want to add a test helper that I know from other test frameworks, something like `test_contains` that takes a regex and a file and prints out the full contents if no match was found. It's obviously not necessary to add this test helper in this patch series of a single patch ("separation of concerns"), just food for thought. > + 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 > +' If it was up to me, I would combine these three test cases, if only to help the `--run=<single-number>` case (the latter two depend on the side effect of the first one to create a `to-ident` tag). > + > +test_expect_success 'identification of reverted commit (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-298-g81715a4e6d Looks good, I have no objections against merging this as-is. Ciao, Dscho Footnote *1*: https://github.com/git/git/blob/v2.36.1/pretty.c#L103-L104 also has the `%C(auto)` prefix, which we obviously do not need here.
On Sat, May 21 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. This is fantastic, I've wanted to fix this particular nit for a long time. > +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,19 @@ 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, "Revert \""); > + strbuf_addstr(&msgbuf, msg.subject); > + strbuf_addstr(&msgbuf, "\""); > + } else { > + strbuf_addstr(&msgbuf, "DESCRIBE WHY WE ARE REVERTING HERE"); Having seen how people use git in the wild, this *WILL* result in a bunch of history where people don't edit that at all. If we want them to edit something we should come up with a buffer that's empty except for suggested #-commented parts, and suggest that they uncomment/edit some of it. I also use "git revert --no-edit" in my own workflow, e.g. when reverting back and forth in WIP code, having the OID is useful, having this new "reference" would be even better, but having that just in the body and "DESCRIBE WHY..." in the %s would be much less useful. > + } > + 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 { This is very POC, but I wish we had something like the WIP diff at the end of this E-Mail (which is a mess, I was experimenting). I.e. we'd always emit (when this setting is enabled) Revert %h (%s, %ad) Except that the %s and the rest of the line would use the truncation feature of "git log" formats, and would make sure the line isn't longer than so-and-so, e.g. 72 or 50 characters. Then only if we truncated things there, or we have parents to mention would we add a body like: This reverts commit %h (%s, %ad) Followed by "reversing changes..." All of which would be much easier than the below monstrosity I came up with if: 1. You could ask "git log" formats to not whitespace pad the truncation, I couldn't find a way to do that (but it's the first time I use it). 2. We just had some generic "wrap this buffer" function, maybe we do, I didn't look much. > 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..36f9565b92 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 (vanilla)' ' > + 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 && This hides the exit code of the embedded $() git command. Re the suggested changes below: let's just test_cmp the whole thing, subject+body instead? > + test_cmp expect actual > +' > + > +test_expect_success 'identification of reverted commit (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 diff --git a/sequencer.c b/sequencer.c index 4abb77add09..a43444e2339 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2061,18 +2061,14 @@ 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 void refer_to_commit(struct strbuf *sb, struct commit *commit, + const char *const fmt) +{ + struct pretty_print_context ctx = { + .abbrev = DEFAULT_ABBREV, + .date_mode.type = DATE_SHORT, + }; + format_commit_message(commit, fmt, sb, &ctx); } static int do_pick_commit(struct repository *r, @@ -2183,19 +2179,63 @@ static int do_pick_commit(struct repository *r, base_label = msg.label; next = parent; next_label = msg.parent_label; - if (!opts->commit_use_reference) { + if (opts->commit_use_reference) { + struct strbuf sb_b = STRBUF_INIT; + struct strbuf sb_h = STRBUF_INIT; + struct strbuf sb_ad = STRBUF_INIT; + struct strbuf sb_s = STRBUF_INIT; + struct strbuf sb_s_t = STRBUF_INIT; + const size_t wraplen = 72; + char *fmt; + size_t len = wraplen; + int truncated; + + refer_to_commit(&sb_h, commit, "Revert %h ("); + len -= sb_h.len; + refer_to_commit(&sb_ad, commit, "%ad)"); + len -= sb_ad.len; + len -= 2 ; /* The ", " in: Revert %h (%s, %ad) */ + /* vanilla %h (%s, %ad) */ + refer_to_commit(&sb_s, commit, "%s"); + + /* truncated %h (%s, %ad) */ + fmt = xstrfmt("%%<(%d,trunc)%%s", (int)len); + refer_to_commit(&sb_s_t, commit, fmt); + free(fmt); + strbuf_rtrim(&sb_s_t); + + truncated = strcmp(sb_s.buf, sb_s_t.buf); + + strbuf_addbuf(&msgbuf, &sb_h); + strbuf_addbuf(&msgbuf, &sb_s_t); + strbuf_addbuf(&msgbuf, &sb_ad); + strbuf_addstr(&msgbuf, "\n\n"); + + if (truncated) { + const char *pfx = "This reverts commit "; + size_t len = wraplen - strlen(pfx); /* TODO: Consider %h etc. */ + char *fmt; + + fmt = xstrfmt("%s%%h (%%w(%d,0,0)%%s, %%ad)", + pfx, (int)len); + refer_to_commit(&sb_b, commit, fmt); + free(fmt); + strbuf_addbuf(&msgbuf, &sb_b); + } else { + /* TODO: The not-truncated version ... */ + } + } else { strbuf_addstr(&msgbuf, "Revert \""); strbuf_addstr(&msgbuf, msg.subject); strbuf_addstr(&msgbuf, "\""); - } else { - strbuf_addstr(&msgbuf, "DESCRIBE WHY WE ARE REVERTING HERE"); - } - strbuf_addstr(&msgbuf, "\n\nThis reverts commit "); - refer_to_commit(opts, &msgbuf, commit); + strbuf_addstr(&msgbuf, "\n\nThis reverts commit "); + strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid)); - if (commit->parents && commit->parents->next) { - strbuf_addstr(&msgbuf, ", reversing\nchanges made to "); - refer_to_commit(opts, &msgbuf, parent); + if (commit->parents && commit->parents->next) { + /* TODO: Handle the wrapping of this too... */ + strbuf_addstr(&msgbuf, ", reversing\nchanges made to "); + strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid)); + } } strbuf_addstr(&msgbuf, ".\n"); } else {
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> + if (!opts->commit_use_reference) { >> + strbuf_addstr(&msgbuf, "Revert \""); >> + strbuf_addstr(&msgbuf, msg.subject); >> + strbuf_addstr(&msgbuf, "\""); >> + } else { >> + strbuf_addstr(&msgbuf, "DESCRIBE WHY WE ARE REVERTING HERE"); > > Having seen how people use git in the wild, this *WILL* result in a > bunch of history where people don't edit that at all. It was done very much on purpose. A pressure from other project participants against the ugly all caps content-free message will help instill better behaviour. A solo developer also will learn after making reverts with content-free titles (and if they do not find it inconvenient for their development purpose that their history is full of content-free titles shouting in all caps, then more power to them---if they are happy, we are happy, too). If we leave something like # Add one line above and explain not *what* this revert did, # but *why* you decided to revert in 50-70 chars. Did it # break something? Was it premature? This reverts commit 8aa3f0dc (CI: set CC in MAKEFLAGS directly, 2022-04-21) presumably, stripspace will make the "This reverts commit ..." the title of the reverted commit. It would invite people not to edit it out out of laziness. Since the whole point of this change is to encourage people to describe the reason behind the revert on the subject line, such an invitation is counter-productive. Doing the first two lines like so: Revert 8aa3f0dc (CI: set CC in MAKEFLAGS directly, 2022-04-21) # Edit the above line to explain not *what* this revert did, would have pretty much the same effect, I am afraid. So...
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Good idea, the `reference` text is so much more pleasant to read (and it > also makes things somewhat proof against rebasing and transitioning to > SHA-256). Never thought of that myself. It does sound like another good reason to see if this is a good alternative format to use. > At some stage, we may even default to `revert.reference=true`? Perhaps. Wasn't planning to propose that in any near future, either, though. I would prefer to never think about changing the default in the future when considering a new feature. It should be done as two separate steps (except that care must be taken so that the feature could become default if we choose to). >> +--reference:: >> ... >> + default. >> + >> + > > Is that extra empty line intentional? It is very much so. A single blank between each items in the section, a larger blank between two sections, is what I aimed at here. >> + if (!opts->commit_use_reference) { >> + strbuf_addstr(&msgbuf, "Revert \""); >> + strbuf_addstr(&msgbuf, msg.subject); >> + strbuf_addstr(&msgbuf, "\""); >> + } else { >> + strbuf_addstr(&msgbuf, "DESCRIBE WHY WE ARE REVERTING HERE"); >> + } > > We often tell newcomers that we prefer the shorter conditional block to > come first. Maybe you want to do that here, too? I don't mind the current > form, though. Good point, but that is more applicable to new code (and not limited to newcomers). We also tell that your new feature is often not any more important than the existing code, and think twice before prepending the new code to existing code (iow, by default new code should be appended, unless there is a good reason not to). I kept the original behaviour first with optional new action as a follow-up. Besides, 3-line block is not all that larger than a single-line block, and this did not trigger my "that one is larger" sensor. Let me try swapping the order. >> @@ -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 (vanilla)' ' > > It might make sense to replace "vanilla" by "default format". Sounds very sensible; thanks. >> + 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 && > > I was a bit puzzled about this dance, as I had expected this instead: > > grep "^This reverts commit $(git rev-parse HEAD^)\\.$" actual.raw > > but I guess you wanted to make sure that the file contents of `actual` are > shown if no match was found? My thought process was much simpler. "^This reverts" is common in both formats, so grep would extract the commit reference(s) the command would have made to "actual". What we need to make sure with the test_cmp are multifold. We want to make sure that the reference is made in the right format, refers to the right commit, and we show the reference exactly the expected number (i.e. one) of times. So it was simpler to do so in a step separate from the "extraction" step above. We could go looser and use test_contains but I usually try to avoid deliberately to be loose. >> +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 >> +' > > If it was up to me, I would combine these three test cases, if only to > help the `--run=<single-number>` case (the latter two depend on the side > effect of the first one to create a `to-ident` tag). I wonder if our prereq infrastructure is lightweight and scalable enough so that we can easily add a support a pseudo-prerequisite PREVIOUS that lets us say test_expect_success PREVIOUS "identification ..." ' ... ' to mean that this test requires the previous test has not been skipped. Then we can organize them into four (one to create to-ident commit, one uses vanilla, one uses --reference and one uses revert.reference), and make the latter three depend on their previous step. >> +test_expect_success 'identification of reverted commit (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 Thanks for taking a look. --- >8 --- Subject: [PATCH v2] revert: optionally refer to commit in the "reference" format 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. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Range-diff: 1: 26b60b56c0 ! 1: c5b11efe7d revert: optionally refer to commit in the "reference" format @@ sequencer.c: static int do_pick_commit(struct repository *r, - 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) { ++ if (opts->commit_use_reference) { ++ strbuf_addstr(&msgbuf, "DESCRIBE WHY WE ARE REVERTING HERE"); ++ } else { + strbuf_addstr(&msgbuf, "Revert \""); + strbuf_addstr(&msgbuf, msg.subject); + strbuf_addstr(&msgbuf, "\""); -+ } else { -+ strbuf_addstr(&msgbuf, "DESCRIBE WHY WE ARE REVERTING HERE"); + } + strbuf_addstr(&msgbuf, "\n\nThis reverts commit "); + refer_to_commit(opts, &msgbuf, commit); @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'advice from failed revert' ' test_cmp expected actual ' + -+test_expect_success 'identification of reverted commit (vanilla)' ' ++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 && @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'advice from failed revert' ' + test_cmp expect actual +' + -+test_expect_success 'identification of reverted commit (reference)' ' ++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 && @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'advice from failed revert' ' + test_cmp expect actual +' + -+test_expect_success 'identification of reverted commit (reference)' ' ++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 && Documentation/config/revert.txt | 3 +++ Documentation/git-revert.txt | 9 +++++++++ builtin/revert.c | 2 ++ sequencer.c | 32 +++++++++++++++++++++++++++----- sequencer.h | 1 + t/t3501-revert-cherry-pick.sh | 31 +++++++++++++++++++++++++++++++ 6 files changed, 73 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..92d301f0c4 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,19 @@ 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, "DESCRIBE WHY WE ARE REVERTING HERE"); + } 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
On Mon, May 23 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> + if (!opts->commit_use_reference) { >>> + strbuf_addstr(&msgbuf, "Revert \""); >>> + strbuf_addstr(&msgbuf, msg.subject); >>> + strbuf_addstr(&msgbuf, "\""); >>> + } else { >>> + strbuf_addstr(&msgbuf, "DESCRIBE WHY WE ARE REVERTING HERE"); >> >> Having seen how people use git in the wild, this *WILL* result in a >> bunch of history where people don't edit that at all. > > It was done very much on purpose. A pressure from other project > participants against the ugly all caps content-free message will > help instill better behaviour. In some cases that will happen, but from experience of working with a bunch of novice git users some people won't edit these at all, particularly when they're in a rush (e.g. reverting something because a thing broke production). So making the default subject less useful is something that will be seen by others, and thus result in worse UX. AFAICT all of the goals you're suggesting here will be even better if it's commented out, then you will need to edit it, and we'll error out by default if you don't. Why not use that? > A solo developer also will learn > after making reverts with content-free titles (and if they do not > find it inconvenient for their development purpose that their > history is full of content-free titles shouting in all caps, then > more power to them---if they are happy, we are happy, too). The subject you're replacing isn't content-free, it at least includes the OID, this one doesn't, so you won't see it in --oneline. That's less useful. > If we leave something like > > # Add one line above and explain not *what* this revert did, > # but *why* you decided to revert in 50-70 chars. Did it > # break something? Was it premature? > > This reverts commit 8aa3f0dc (CI: set CC in MAKEFLAGS directly, 2022-04-21) > > presumably, stripspace will make the "This reverts commit ..." the > title of the reverted commit. It would invite people not to edit it > out out of laziness. Since the whole point of this change is to > encourage people to describe the reason behind the revert on the > subject line, such an invitation is counter-productive. If that's the concern we could also comment the "This reverts" line. > Doing the first two lines like so: > > Revert 8aa3f0dc (CI: set CC in MAKEFLAGS directly, 2022-04-21) > # Edit the above line to explain not *what* this revert did, > > would have pretty much the same effect, I am afraid. > > So... Ditto, so just present a buffer like: # Revert 8aa3f0dc (CI: set CC in MAKEFLAGS directly, 2022-04-21) # # This reverts commit 8aa3f0dc (CI: set CC in MAKEFLAGS directly, 2022-04-21) Along with some advice that the user can uncomment those template lines & adjust them.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > AFAICT all of the goals you're suggesting here will be even better if > it's commented out, then you will need to edit it, and we'll error out > by default if you don't. Why not use that? Simply because I doubt that would work. Doesn't the stripspace on the log message with unedited comments simply discard the commented out part and results in a title that is taken from the line that happens to end up being the first non-blank-non-commented-out line? > The subject you're replacing isn't content-free, it at least includes > the OID, this one doesn't, so you won't see it in --oneline. That's less > useful. I know. That is the point; those who choose to use --reference want to make sure they want to edit out the line that tell them to edit. > >> If we leave something like >> >> # Add one line above and explain not *what* this revert did, >> # but *why* you decided to revert in 50-70 chars. Did it >> # break something? Was it premature? >> >> This reverts commit 8aa3f0dc (CI: set CC in MAKEFLAGS directly, 2022-04-21) >> >> presumably, stripspace will make the "This reverts commit ..." the >> title of the reverted commit. It would invite people not to edit it >> out out of laziness. Since the whole point of this change is to >> encourage people to describe the reason behind the revert on the >> subject line, such an invitation is counter-productive. > > If that's the concern we could also comment the "This reverts" line. That punishes the normal use case where the title is edited and the body is used to start the message as-is.
Hi Junio On 22/05/2022 05:32, 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. I think this is a good idea which will hopefully improve project histories. > 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")), This option is being added to base_options which applies to cherry-pick as well as revert. There is a "if" statement just below this hunk which adds cherry-pick specific options, I think we want to add this new option in an else block added to that "if" statement. > static int do_pick_commit(struct repository *r, > struct todo_item *item, > struct replay_opts *opts, > @@ -2167,14 +2184,19 @@ 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, "Revert \""); > + strbuf_addstr(&msgbuf, msg.subject); > + strbuf_addstr(&msgbuf, "\""); > + } else { > + strbuf_addstr(&msgbuf, "DESCRIBE WHY WE ARE REVERTING HERE"); In format-patch we add three asterisks to the beginning and end of the dummy subject and body lines to encourage the user to edit them - is that worth doing here as well? Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > In format-patch we add three asterisks to the beginning and end of the > dummy subject and body lines to encourage the user to edit them - is > that worth doing here as well? Yeah, that would make things consistent. I am still mulling over Ævar's idea of preparing the messsage in such a way that, if committed without editing, the title becomes identifiable but too ugly to force users consider rewording (and the "identifiable" part helps the "rebase -i" process), while making it easy to discard the instruction for the user who do want to edit. I think that is the right approach, if we can find one. What has been floated on the list during the discussion is not there yet, but I think the approach is a good one. Thanks.
Hi Junio, On Mon, 23 May 2022, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > >> +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 > >> +' > > > > If it was up to me, I would combine these three test cases, if only to > > help the `--run=<single-number>` case (the latter two depend on the > > side effect of the first one to create a `to-ident` tag). > > I wonder if our prereq infrastructure is lightweight and scalable enough > so that we can easily add a support a pseudo-prerequisite PREVIOUS that > lets us say > > test_expect_success PREVIOUS "identification ..." ' > ... > ' > > to mean that this test requires the previous test has not been > skipped. In theory, this sounds good to me. In practice, however, side effects are awful and make everything harder, from developing code to debugging to helping new contributors. I wish we would do away with them altogether and have something more akin to the before/after constructs known from e.g. TestNG (think `@BeforeTest` and `@BeforeClass`). One option would be to mark `setup` steps completely differently, sort of imitating the prereq infrastructure instead of using `test_expect_success`. Kind of prereqs, but required to pass. This could potentially allow us to randomize the order in which the test cases are run, to identify and fix (unintended) side effects. A complication is that we have nothing in the way of `@AfterClass`, i.e. we do not have a way to, say, run an Apache instance for the lifecycle of a given test script _and tear it down at the end_. Another, rather obvious complication is that we have 17 years of commit history introducing side effects left and right :laughing: Ciao, Dscho
On Mon, May 30 2022, Johannes Schindelin wrote: > Hi Junio, > > On Mon, 23 May 2022, Junio C Hamano wrote: > >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> >> >> +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 >> >> +' >> > >> > If it was up to me, I would combine these three test cases, if only to >> > help the `--run=<single-number>` case (the latter two depend on the >> > side effect of the first one to create a `to-ident` tag). >> >> I wonder if our prereq infrastructure is lightweight and scalable enough >> so that we can easily add a support a pseudo-prerequisite PREVIOUS that >> lets us say >> >> test_expect_success PREVIOUS "identification ..." ' >> ... >> ' >> >> to mean that this test requires the previous test has not been >> skipped. > > In theory, this sounds good to me. > > In practice, however, side effects are awful and make everything harder, > from developing code to debugging to helping new contributors. I wish we > would do away with them altogether and have something more akin to the > before/after constructs known from e.g. TestNG (think `@BeforeTest` and > `@BeforeClass`). > > One option would be to mark `setup` steps completely differently, sort of > imitating the prereq infrastructure instead of using > `test_expect_success`. Kind of prereqs, but required to pass. I've suggested a test_expect_setup before: https://lore.kernel.org/git/8735vrvg39.fsf@evledraar.gmail.com/; basically a test_expect_success that ignores GIT_SKIP_TESTS and --run. I think that even if we could imagine much more complex relationships (up to and including writing these tests as Makefiles instead) it's better to just have the simpler "this is a setup". Then for everything more complex be more eager to split up tests. > This could potentially allow us to randomize the order in which the test > cases are run, to identify and fix (unintended) side effects. Yes, a "chaos monkey" mode similar to --stress would be nice. > A complication is that we have nothing in the way of `@AfterClass`, i.e. > we do not have a way to, say, run an Apache instance for the lifecycle of > a given test script _and tear it down at the end_. I think this is generally a feature in that if you find yourself needing this you should split the test up so that the Apache setup is in only one file. E.g. in the case of some http tests we have a prereq on something for git:// and a different thing (apache) for http:// tests, so that depending on what combination you have we might end up needlessly skipping tests. > Another, rather obvious complication is that we have 17 years of commit > history introducing side effects left and right :laughing:
Am 22.05.22 um 06:32 schrieb Junio C Hamano: > --- > Documentation/config/revert.txt | 3 +++ > Documentation/git-revert.txt | 9 +++++++++ > builtin/revert.c | 2 ++ > sequencer.c | 32 +++++++++++++++++++++++++++----- > sequencer.h | 1 + > t/t3501-revert-cherry-pick.sh | 31 +++++++++++++++++++++++++++++++ > 6 files changed, 73 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. Shouldn't this be "were" instead of "is"? Not fully sure. Anyway: --- >8 --- Subject: [PATCH] revert: config documentation fixes 43966ab315 (revert: optionally refer to commit in the "reference" format, 2022-05-26) added the documentation file config/revert.txt. Actually include it in config.txt. Make is used with a bare infinitive after the object; remove the "to". Signed-off-by: René Scharfe <l.s.r@web.de> --- Documentation/config.txt | 2 ++ Documentation/config/revert.txt | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index e284b042f2..e376d547ce 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -495,6 +495,8 @@ include::config/repack.txt[] include::config/rerere.txt[] +include::config/revert.txt[] + include::config/safe.txt[] include::config/sendemail.txt[] diff --git a/Documentation/config/revert.txt b/Documentation/config/revert.txt index 797bfb6d62..802d6faca2 100644 --- a/Documentation/config/revert.txt +++ b/Documentation/config/revert.txt @@ -1,3 +1,3 @@ revert.reference:: - Setting this variable to true makes `git revert` to behave + Setting this variable to true makes `git revert` behave as if the `--reference` option is given. -- 2.36.1
René Scharfe <l.s.r@web.de> writes: > Actually include it in config.txt. > > Make is used with a bare infinitive after the object; remove the "to". Thanks.
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..97abd5932b 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,19 @@ 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, "Revert \""); + strbuf_addstr(&msgbuf, msg.subject); + strbuf_addstr(&msgbuf, "\""); + } else { + strbuf_addstr(&msgbuf, "DESCRIBE WHY WE ARE REVERTING HERE"); + } + 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..36f9565b92 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 (vanilla)' ' + 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 (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. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/config/revert.txt | 3 +++ Documentation/git-revert.txt | 9 +++++++++ builtin/revert.c | 2 ++ sequencer.c | 32 +++++++++++++++++++++++++++----- sequencer.h | 1 + t/t3501-revert-cherry-pick.sh | 31 +++++++++++++++++++++++++++++++ 6 files changed, 73 insertions(+), 5 deletions(-) create mode 100644 Documentation/config/revert.txt