From: Junio C Hamano <gitster@pobox.com>
To: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Cc: git@vger.kernel.org, Kristoffer Haugsbakk <code@khaugsbakk.name>
Subject: Re: [PATCH v2] sequencer: beautify subject of reverts of reverts
Date: Fri, 28 Apr 2023 11:35:28 -0700 [thread overview]
Message-ID: <xmqqcz3netxr.fsf@gitster.g> (raw)
In-Reply-To: <20230428083528.1699221-1-oswald.buddenhagen@gmx.de> (Oswald Buddenhagen's message of "Fri, 28 Apr 2023 10:35:28 +0200")
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> Instead of generating a silly-looking `Revert "Revert "foo""`, make it
> a more humane `Reapply "foo"`.
>
> The alternative `Revert^2 "foo"`, etc. was considered, but it was deemed
> over-engineered and "too nerdy". Instead, people should get creative
> with the subjects when they recurse reverts that deeply. The proposed
> change encourages that by example and explicit recommendation.
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
> diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
> index d2e10d3dce..e8fa513607 100644
> --- a/Documentation/git-revert.txt
> +++ b/Documentation/git-revert.txt
> @@ -31,6 +31,12 @@ both will discard uncommitted changes in your working directory.
> See "Reset, restore and revert" in linkgit:git[1] for the differences
> between the three commands.
>
> +The command generates the subject 'Revert "<title>"' for the resulting
> +commit, assuming the original commit's subject is '<title>'. Reverting
> +such a reversion commit in turn yields the subject 'Reapply "<title>"'.
Clearly written.
> +These can of course be modified in the editor when the reason for
> +reverting is described.
Not just the title but the entire message can be edited and that is
by design. Having to modify what this new mechanism does when
existing users do not like the new behaviour will annoy them, and
this sentence will not be a good enough excuse to ask them
forgiveness for breaking their established practice, either.
So, I am not sure if there is a point to have this sentence here.
> diff --git a/sequencer.c b/sequencer.c
> index 3be23d7ca2..61e466470e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2227,13 +2227,27 @@ 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)) {
> + if (skip_prefix(orig_subject, "Revert \"", &orig_subject)) {
> + /*
> + * This prevents the generation of somewhat unintuitive (even if
> + * not incorrect) 'Reapply "Revert "' titles from legacy double
> + * reverts. Fixing up deeper recursions is left to the user.
> + */
Good comment but in an overwide paragraph.
> + strbuf_addstr(&msgbuf, "Revert \"Reapply \"");
> + } else {
> + strbuf_addstr(&msgbuf, "Reapply \"");
> + }
> + strbuf_addstr(&msgbuf, orig_subject);
> } else {
> strbuf_addstr(&msgbuf, "Revert \"");
> strbuf_addstr(&msgbuf, msg.subject);
> diff --git a/t/t3515-revert-subjects.sh b/t/t3515-revert-subjects.sh
> new file mode 100755
> index 0000000000..ea4319fd15
> --- /dev/null
> +++ b/t/t3515-revert-subjects.sh
It is a bit unexpectd that we need an entire new file to test this.
It is doubly bad that the title of the file is only about the
subject of revert commits and does not allow other things to be
added later. Are we planning to have a lot more creativity in how
automatically generated subject of revert commits would read?
If there isn't a good enough test coverage for the "git revert"
command already, then having a new file to test "git revert" would
be an excellent idea, adding one here is a very welcome addition,
and it is perfectly fine to start such a new test with only these
new tests that protects the new "Revert Revert to Reapply" feature.
But if there is a test file already for "git revert" that covers
other behaviour of the command, "create two new commits, i.e. revert
and revert of revert, and then try reverting them and see what their
subject says" ought to be a simple addition or two to such an
existing test file. Doesn't t3501 seem a better home for them? The
last handful of tests there are about how the auto-generated log is
phrased, and would form a good group with this new feature, wouldn't
it?
> @@ -0,0 +1,32 @@
> +#!/bin/sh
> +
> +test_description='git revert produces the expected subject'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'fresh reverts' '
> + test_commit --no-tag A file1 &&
> + test_commit --no-tag B file1 &&
> + git revert --no-edit HEAD &&
> + echo "Revert \"B\"" > expect &&
Style. See Documentation/CodingGuidelines and look for "For shell
scripts specifically".
> + 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 'legacy double revert' '
> + test_commit --no-tag "Revert \"Revert \"B\"\"" file1 &&
> + git revert --no-edit HEAD &&
> + echo "Revert \"Reapply \"B\"\"" > expect &&
> + git log -1 --pretty=%s > actual &&
> + test_cmp expect actual
> +'
> +
> +test_done
Thanks.
next prev parent reply other threads:[~2023-04-28 18:36 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 [this message]
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 ` [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=xmqqcz3netxr.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=code@khaugsbakk.name \
--cc=git@vger.kernel.org \
--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).