From: Phillip Wood <phillip.wood123@gmail.com>
To: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>, git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
Kristoffer Haugsbakk <code@khaugsbakk.name>
Subject: Re: [PATCH v3 1/2] sequencer: beautify subject of reverts of reverts
Date: Fri, 11 Aug 2023 16:05:03 +0100 [thread overview]
Message-ID: <dba3f15a-3575-e4f9-2291-c5a342cfed43@gmail.com> (raw)
In-Reply-To: <20230809171531.2564807-1-oswald.buddenhagen@gmx.de>
Hi Oswald
On 09/08/2023 18:15, Oswald Buddenhagen wrote:
> Instead of generating a silly-looking `Revert "Revert "foo""`, make it
> a more humane `Reapply "foo"`.
>
> This is done for two reasons:
> - To cover the actually common case of just a double revert.
> - To encourage people to rewrite summaries of recursive reverts by
> setting an example (a subsequent commit will also do this explicitly
> in the documentation).
>
> To achieve these goals, the mechanism does not need to be particularly
> sophisticated. Therefore, more complicated alternatives which would
> "compress more efficiently" have not been implemented.
This all looks good to me, it seems quite sensible just to bail out if
we see an existing recursive reversion. I'm not suggesting you change
these tests but for future reference we now have a test_commit_message()
function which was merged a few days ago to simplify tests like these.
Thanks for working on it
Phillip
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>
> ---
> v3:
> - capitulate at first sight of a pre-existing recursive reversion, as
> handling the edge cases is a bottomless pit
> - reworked commit message again
> - moved test into existing file
> - generalized docu change and factored it out
>
> v2:
> - add discussion to commit message
> - add paragraph to docu
> - add test
> - use skip_prefix() instead of starts_with()
> - catch pre-existing double reverts
>
> Cc: Junio C Hamano <gitster@pobox.com>
> Cc: Kristoffer Haugsbakk <code@khaugsbakk.name>
> Cc: Phillip Wood <phillip.wood123@gmail.com>
> ---
> sequencer.c | 11 +++++++++++
> t/t3501-revert-cherry-pick.sh | 25 +++++++++++++++++++++++++
> 2 files changed, 36 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index cc9821ece2..12ec158922 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2249,13 +2249,24 @@ static int do_pick_commit(struct repository *r,
> */
>
> if (command == TODO_REVERT) {
> + const char *orig_subject;
> +
> base = commit;
> base_label = msg.label;
> next = parent;
> next_label = msg.parent_label;
> if (opts->commit_use_reference) {
> strbuf_addstr(&msgbuf,
> "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> + } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
> + /*
> + * We don't touch pre-existing repeated reverts, because
> + * theoretically these can be nested arbitrarily deeply,
> + * thus requiring excessive complexity to deal with.
> + */
> + !starts_with(orig_subject, "Revert \"")) {
> + strbuf_addstr(&msgbuf, "Reapply \"");
> + strbuf_addstr(&msgbuf, orig_subject);
> } else {
> strbuf_addstr(&msgbuf, "Revert \"");
> strbuf_addstr(&msgbuf, msg.subject);
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index e2ef619323..7011e3a421 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -176,6 +176,31 @@ test_expect_success 'advice from failed revert' '
> test_cmp expected actual
> '
>
> +test_expect_success 'title of fresh reverts' '
> + test_commit --no-tag A file1 &&
> + test_commit --no-tag B file1 &&
> + git revert --no-edit HEAD &&
> + echo "Revert \"B\"" >expect &&
> + git log -1 --pretty=%s >actual &&
> + test_cmp expect actual &&
> + git revert --no-edit HEAD &&
> + echo "Reapply \"B\"" >expect &&
> + git log -1 --pretty=%s >actual &&
> + test_cmp expect actual &&
> + git revert --no-edit HEAD &&
> + echo "Revert \"Reapply \"B\"\"" >expect &&
> + git log -1 --pretty=%s >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'title of legacy double revert' '
> + test_commit --no-tag "Revert \"Revert \"B\"\"" file1 &&
> + git revert --no-edit HEAD &&
> + echo "Revert \"Revert \"Revert \"B\"\"\"" >expect &&
> + git log -1 --pretty=%s >actual &&
> + test_cmp expect actual
> +'
> +
> test_expect_success 'identification of reverted commit (default)' '
> test_commit to-ident &&
> test_when_finished "git reset --hard to-ident" &&
next prev parent reply other threads:[~2023-08-11 15:05 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 ` Re* " Junio C Hamano
2023-08-11 17:53 ` 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 ` Phillip Wood [this message]
2023-08-11 16:59 ` [PATCH v3 1/2] sequencer: beautify subject of reverts of reverts 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=dba3f15a-3575-e4f9-2291-c5a342cfed43@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=code@khaugsbakk.name \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=oswald.buddenhagen@gmx.de \
--cc=phillip.wood@dunelm.org.uk \
/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).