From: Junio C Hamano <gitster@pobox.com>
To: Linus Arver <linusa@google.com>
Cc: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>, git@vger.kernel.org
Subject: Re* [PATCH v3 2/2] doc: revert: add discussion
Date: Fri, 11 Aug 2023 10:44:50 -0700 [thread overview]
Message-ID: <xmqq5y5ltqwd.fsf_-_@gitster.g> (raw)
In-Reply-To: <xmqqsf8ptsqf.fsf@gitster.g> (Junio C. Hamano's message of "Fri, 11 Aug 2023 10:05:12 -0700")
Junio C Hamano <gitster@pobox.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>>
>>> while thinking about what to write, i came up with an idea for another
>>> improvement: with (implicit) --edit, the template message would end up
>>> being:
>>>
>>> This reverts commit <sha1>,
>>> because <PUT REASON HERE>.
>>
>> This sounds great to me.
>
> Oh, absolutely. I rarely do a revert myself (other than reverting a
> premature merge out of 'next'), but giving a better instruction in
> the commit log editor buffer as template is a very good idea.
It might be just the matter of doing something like the attached
patch on top of Oswald's, reusing the existing code to instruct the
user to describe the reversion.
------- >8 ------------- >8 ------------- >8 ------------- >8 -------
Subject: [PATCH 3/2] revert: force explaining overly complex revert chain
Once we revert reverts of revert and reach "Reapply "Reapply "..."",
it becomes too unweirdly to read a reversion of such a comit.
We instruct the user to explain why the reversion is done in their
own words when using the revert.reference mode, and the instruction
applies equally for such an overly complex revert chain. The
rationale for such a sequence of events should be recorded to help
future developers.
Building on top of the recent Oswald's work to turn "revert revert"
into "reapply", let's turn the reference mode automatically on in
such a case.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* I left the reference to the second parent to honor the command
line option and configuration variable even when this new
mechanism kicks in and this is very much deliberate. As a commit
that is a revert (or reapply) should be single parent (because a
revert of a merge is a single parent commit), the choice does not
make any difference in practice.
sequencer.c | 16 +++++++++++-----
t/t3501-revert-cherry-pick.sh | 11 ++++++++++-
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git c/sequencer.c w/sequencer.c
index 7dc13fdcca..43bb558518 100644
--- c/sequencer.c
+++ w/sequencer.c
@@ -2130,10 +2130,10 @@ static int should_edit(struct replay_opts *opts) {
return opts->edit;
}
-static void refer_to_commit(struct replay_opts *opts,
+static void refer_to_commit(int use_reference,
struct strbuf *msgbuf, struct commit *commit)
{
- if (opts->commit_use_reference) {
+ if (use_reference) {
struct pretty_print_context ctx = {
.abbrev = DEFAULT_ABBREV,
.date_mode.type = DATE_SHORT,
@@ -2250,12 +2250,18 @@ static int do_pick_commit(struct repository *r,
if (command == TODO_REVERT) {
const char *orig_subject;
+ int use_reference = opts->commit_use_reference;
base = commit;
base_label = msg.label;
next = parent;
next_label = msg.parent_label;
- if (opts->commit_use_reference) {
+
+ if (starts_with(msg.subject, "Reapply \"Reapply \""))
+ /* fifth time is too many - force reference format*/
+ use_reference = 1;
+
+ if (use_reference) {
strbuf_addstr(&msgbuf,
"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
@@ -2273,11 +2279,11 @@ static int do_pick_commit(struct repository *r,
strbuf_addstr(&msgbuf, "\"");
}
strbuf_addstr(&msgbuf, "\n\nThis reverts commit ");
- refer_to_commit(opts, &msgbuf, commit);
+ refer_to_commit(use_reference, &msgbuf, commit);
if (commit->parents && commit->parents->next) {
strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
- refer_to_commit(opts, &msgbuf, parent);
+ refer_to_commit(opts->commit_use_reference, &msgbuf, parent);
}
strbuf_addstr(&msgbuf, ".\n");
} else {
diff --git c/t/t3501-revert-cherry-pick.sh w/t/t3501-revert-cherry-pick.sh
index 7011e3a421..7a8715d3f4 100755
--- c/t/t3501-revert-cherry-pick.sh
+++ w/t/t3501-revert-cherry-pick.sh
@@ -190,7 +190,16 @@ test_expect_success 'title of fresh reverts' '
git revert --no-edit HEAD &&
echo "Revert \"Reapply \"B\"\"" >expect &&
git log -1 --pretty=%s >actual &&
- test_cmp expect actual
+ test_cmp expect actual &&
+ git revert --no-edit HEAD &&
+ echo "Reapply \"Reapply \"B\"\"" >expect &&
+ git log -1 --pretty=%s >actual &&
+ test_cmp expect actual &&
+
+ # Give the stronger instruction for unusually complex case
+ git revert --no-edit HEAD &&
+ git log -1 --pretty=%s >actual &&
+ grep -F -e "# *** SAY WHY WE ARE REVERTING" actual
'
test_expect_success 'title of legacy double revert' '
next prev parent reply other threads:[~2023-08-11 17:44 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-28 8:35 [PATCH v2] sequencer: beautify subject of reverts of reverts Oswald Buddenhagen
2023-04-28 18:35 ` Junio C Hamano
2023-04-28 19:11 ` Oswald Buddenhagen
2023-05-01 16:44 ` Junio C Hamano
2023-05-01 19:10 ` Oswald Buddenhagen
2023-05-01 19:12 ` Junio C Hamano
2023-05-05 17:25 ` Junio C Hamano
2023-05-17 9:05 ` Phillip Wood
2023-05-17 10:00 ` Oswald Buddenhagen
2023-05-17 11:20 ` Phillip Wood
2023-05-17 17:02 ` Oswald Buddenhagen
2023-05-18 9:58 ` Phillip Wood
2023-05-18 16:28 ` Junio C Hamano
2023-07-28 5:26 ` Linus Arver
2023-07-28 9:45 ` Oswald Buddenhagen
2023-07-28 15:10 ` Junio C Hamano
2023-07-28 15:37 ` Oswald Buddenhagen
2023-07-28 16:31 ` Junio C Hamano
2023-07-28 16:47 ` Oswald Buddenhagen
2023-07-28 17:36 ` Linus Arver
2023-08-09 17:15 ` [PATCH v3 1/2] " Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v3 2/2] doc: revert: add discussion Oswald Buddenhagen
2023-08-10 21:50 ` Linus Arver
2023-08-10 22:00 ` Linus Arver
2023-08-11 15:10 ` Phillip Wood
2023-08-12 6:25 ` Oswald Buddenhagen
2023-08-13 22:09 ` Junio C Hamano
2023-08-14 14:13 ` Oswald Buddenhagen
2023-08-11 12:49 ` Oswald Buddenhagen
2023-08-11 23:00 ` Linus Arver
2023-08-12 7:19 ` Oswald Buddenhagen
2023-09-07 21:29 ` Linus Arver
2023-08-11 15:08 ` Phillip Wood
2023-08-11 17:10 ` Junio C Hamano
2023-08-11 17:05 ` Junio C Hamano
2023-08-11 17:44 ` Junio C Hamano [this message]
2023-08-11 17:53 ` Re* " Junio C Hamano
2023-08-11 17:56 ` rsbecker
2023-08-11 18:16 ` Eric Sunshine
2023-08-11 18:16 ` Oswald Buddenhagen
2023-08-11 19:43 ` Junio C Hamano
2023-08-11 15:05 ` [PATCH v3 1/2] sequencer: beautify subject of reverts of reverts Phillip Wood
2023-08-11 16:59 ` Junio C Hamano
2023-08-11 17:13 ` Oswald Buddenhagen
2023-08-21 17:07 ` [PATCH v4 " Oswald Buddenhagen
2023-08-21 17:07 ` [PATCH v4 2/2] git-revert.txt: add discussion Oswald Buddenhagen
2023-08-21 18:32 ` [PATCH v4 1/2] sequencer: beautify subject of reverts of reverts Junio C Hamano
2023-08-23 20:08 ` Taylor Blau
2023-08-23 21:38 ` Junio C Hamano
2023-08-24 6:14 ` Oswald Buddenhagen
2023-09-02 7:20 ` [PATCH v5] " Oswald Buddenhagen
2023-09-02 22:24 ` Junio C Hamano
2023-09-11 20:12 ` Kristoffer Haugsbakk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq5y5ltqwd.fsf_-_@gitster.g \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=linusa@google.com \
--cc=oswald.buddenhagen@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).