[PATCH] sequencer: beautify subject of reverts of reverts

[PATCH] sequencer: beautify subject of reverts of reverts

[Oswald Buddenhagen]

Instead of generating a silly-looking `Revert "Revert "foo""`, make it
`Reapply "foo"`.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sequencer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 3be23d7ca2..853b4ed334 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2234,6 +2234,9 @@ static int do_pick_commit(struct repository *r,
 		if (opts->commit_use_reference) {
 			strbuf_addstr(&msgbuf,
 				"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
+		} else if (starts_with(msg.subject, "Revert \"")) {
+			strbuf_addstr(&msgbuf, "Reapply ");
+			strbuf_addstr(&msgbuf, msg.subject + 7);
 		} else {
 			strbuf_addstr(&msgbuf, "Revert \"");
 			strbuf_addstr(&msgbuf, msg.subject);
-- 
2.40.0.152.g15d061e6df

Re: [PATCH] sequencer: beautify subject of reverts of reverts

[Kristoffer Haugsbakk]

On Thu, Mar 23, 2023, at 17:22, Oswald Buddenhagen wrote:
> Instead of generating a silly-looking `Revert "Revert "foo""`, make it
> `Reapply "foo"`.

Nice addition.

    $ echo change >> README.md
    $ ./bin-wrappers/git add README.md
    $ ./bin-wrappers/git commit -m 'A change'
    $ ./bin-wrappers/git revert --no-edit @
    $ ./bin-wrappers/git revert --no-edit @
    $ ./bin-wrappers/git log --oneline master..
    adfce56c6a (HEAD -> reapply) Reapply "A change"
    395894c2ce Revert "A change"
    a01e3d6b3d A change
    058643b69f sequencer: beautify subject of reverts of reverts

-- 
Kristoffer Haugsbakk

Re: [PATCH] sequencer: beautify subject of reverts of reverts

[Junio C Hamano]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

> On Thu, Mar 23, 2023, at 17:22, Oswald Buddenhagen wrote:
>> Instead of generating a silly-looking `Revert "Revert "foo""`, make it
>> `Reapply "foo"`.
>
> Nice addition.
>
>     $ echo change >> README.md
>     $ ./bin-wrappers/git add README.md
>     $ ./bin-wrappers/git commit -m 'A change'
>     $ ./bin-wrappers/git revert --no-edit @
>     $ ./bin-wrappers/git revert --no-edit @
>     $ ./bin-wrappers/git log --oneline master..
>     adfce56c6a (HEAD -> reapply) Reapply "A change"
>     395894c2ce Revert "A change"
>     a01e3d6b3d A change
>     058643b69f sequencer: beautify subject of reverts of reverts

I think Oswald saw the end of a thread a few years ago that
discussed a similar idea and the patch is a continuation of that
thread, but what should happen when the re-application turns out to
be bad?

The significance of the act of reverting such a reapplication to the
project would be different from the initial revert (where "yikes,
the change introduces a regression---let's revert to the state
before the change, regroup, and see what we should do" was the
motivation).  Somebody thought that it now was OK to reintroduce the
change, presumably because the code paths around it now have become
ready for it and the phrase "Reapply" makes a perfect sense to
describe it ("Revert an earlier revert" is not too bad, either,
though).  But then it turns out it still was a bad idea.  Should we
say

    Revert Reapply "A change"

Next time somebody thinks the code paths around there are finally
ready, do they do

    Reapply Reapply "A change"

or something else?  That may be shorter, but even more cryptic than
'Revert Revert Revert Revert "A change"'---"Revert^4 "A change"" the
other thread proposed start to look less horrible.  So, I dunno.

Re: [PATCH] sequencer: beautify subject of reverts of reverts

[Kristoffer Haugsbakk]

On Tue, Mar 28, 2023, at 00:25, Junio C Hamano wrote:
> ""Revert^4 "A change" the other thread proposed start to look less horrible.
> So, I dunno.

That looks good. And the transformations are just:

    Revert " → Reapply "
    Reapply " → Revert^3 "
    Revert^<n> " → Revert^<n+1> "

-- 
Kristoffer Haugsbakk

Re: [PATCH] sequencer: beautify subject of reverts of reverts

[Oswald Buddenhagen]

On Tue, Mar 28, 2023 at 08:04:43AM +0200, Kristoffer Haugsbakk wrote:
>On Tue, Mar 28, 2023, at 00:25, Junio C Hamano wrote:
>> ""Revert^4 "A change" the other thread proposed start to look less horrible.
>> So, I dunno.
>
>That looks good. And the transformations are just:
>
>    Revert " → Reapply "
>    Reapply " → Revert^3 "
>    Revert^<n> " → Revert^<n+1> "
>
i thought about that already, and concluded that it's getting a bit "too 
nerdy" and over-engineered.
the main motivation for me is to break the dogmatism with which some 
people are approaching the matter - "$tool did it, so it is _the_ way".  
set an example where the tool does something "humane", and you may 
change some minds.
when it gets to round 3 of reverting, i expect people to get creative:

  Revert "foo" again
  Reapply "foo", take 2
  etc.

but i don't mind, as long as `Revert "Revert "Revert "foo"""` cannot be 
argued to be canon any more.

Re: [PATCH] sequencer: beautify subject of reverts of reverts

[Kristoffer Haugsbakk]

On Tue, Mar 28, 2023, at 15:23, Oswald Buddenhagen wrote:
> i thought about that already, and concluded that it's getting a bit "too
> nerdy" and over-engineered.

I see that point too.

> the main motivation for me is to break the dogmatism with which some
> people are approaching the matter - "$tool did it, so it is _the_ way".
> set an example where the tool does something "humane", and you may
> change some minds.

Good thinking.

Your patch does something “humane” without adding any involved logic. A
good default for those who don’t want to change the provided revert
message. (Why? I would hope that they at least write something in the
body (after the subject) about why they are (reverting a
revert)/reapplying.)

-- 
Kristoffer Haugsbakk
Frequent reverter

Re: [PATCH] sequencer: beautify subject of reverts of reverts

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

>>That looks good. And the transformations are just:
>>
>>    Revert " → Reapply "
>>    Reapply " → Revert^3 "
>>    Revert^<n> " → Revert^<n+1> "
>>
> i thought about that already, and concluded that it's getting a bit
> "too nerdy" and over-engineered.

I agree that Revert^<n> looks too nerdy, and suspect that it was
also one of the reasons why the old discussion thread died out, even
though nobody in the thread explicitly mentioned it as the reason
to reject it.

> ...
> but i don't mind, as long as `Revert "Revert "Revert "foo"""` cannot
> be argued to be canon any more.

At least, that form can be mechanically understood what it means
with a single rule (i.e. "count the leading Revert"), instead of
requiring a set of rules (i.e. "if it begins with Reapply, then that
is reverted twice, count Reapply, multiply by two, and then add the
number of Revert").

I personally prefer a format that conveys how things happened in a
way that can be easily understood, over a format that looks pretty
on surface but needs more special cases to understand what led to
the outcome it represents.  But because reverting a commit ought to
be a rare event, and reverting a revert (or doing so recursively for
more levels) ought to be even rarer, it shouldn't matter all that
much either way how we phrase the reversion of a revert, or reversion
of such a reversion.

There will be folks who will complain no matter how we change the
way we phrase the reversion of a revert, while there may also be
other folks who like the change we make, and I feel that it would
not be worth my time to deal with the complaints for _this_
particular change that was proposed.  As long as proponents for this
change promise to handle all such complaints on and off list, I am
fine with the change, though.

Having said all that, I think this change, if we were to apply,
should be described in the documentation.  Perhaps something along
this line?

diff --git c/Documentation/git-revert.txt w/Documentation/git-revert.txt
index d2e10d3dce..d09311dd8a 100644
--- c/Documentation/git-revert.txt
+++ w/Documentation/git-revert.txt
@@ -31,6 +31,13 @@ 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 by default gives "Revert '<title>'" as the title to the
+resulting commit when reverting the original commit whose title is
+'<title>'.  A commit that reverts such a reversion commit is given
+"Reapply '<title>' as its title.  These can of course be edited in
+the editor when the reason for reverting is described.
+
+
 OPTIONS
 -------
 <commit>...::

Re: [PATCH] sequencer: beautify subject of reverts of reverts

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> diff --git a/sequencer.c b/sequencer.c
> index 3be23d7ca2..853b4ed334 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2234,6 +2234,9 @@ static int do_pick_commit(struct repository *r,
>  		if (opts->commit_use_reference) {
>  			strbuf_addstr(&msgbuf,
>  				"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> +		} else if (starts_with(msg.subject, "Revert \"")) {
> +			strbuf_addstr(&msgbuf, "Reapply ");
> +			strbuf_addstr(&msgbuf, msg.subject + 7);
>  		} else {
>  			strbuf_addstr(&msgbuf, "Revert \"");
>  			strbuf_addstr(&msgbuf, msg.subject);

Two and half comments:

 * The hard-coded +7 looks fragile.  Perhaps use skip_prefix?

 * During transition to introduce a new version of Git with this
   feature, when reverting an existing revert of a revert, care must
   be taken.  Such a commit would begin as

	Revert "Revert ...

   and turning it to

	Reapply "Revert ...

   may not be a good way to label such a reversion of a double
   revert without end-user confusion.  As it is very likely that
   such a reversion commit was created by existing versions of Git,
   the easiest and least confusing way out would be to notice and
   refrain from touching such a commit.

 * The change lacks tests.

Removal of hardcoded +7 (i.e. the first point) may look like this.

 sequencer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git i/sequencer.c w/sequencer.c
index 853b4ed334..520113ec63 100644
--- i/sequencer.c
+++ w/sequencer.c
@@ -2227,6 +2227,8 @@ static int do_pick_commit(struct repository *r,
 	 */
 
 	if (command == TODO_REVERT) {
+		const char *original_title;
+
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -2234,9 +2236,9 @@ static int do_pick_commit(struct repository *r,
 		if (opts->commit_use_reference) {
 			strbuf_addstr(&msgbuf,
 				"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
-		} else if (starts_with(msg.subject, "Revert \"")) {
+		} else if (skip_prefix(msg.subject, "Revert \"", &original_title)) {
-			strbuf_addstr(&msgbuf, "Reapply ");
+			strbuf_addstr(&msgbuf, "Reapply \"");
-			strbuf_addstr(&msgbuf, msg.subject + 7);
+			strbuf_addstr(&msgbuf, original_title);
 		} else {
 			strbuf_addstr(&msgbuf, "Revert \"");
 			strbuf_addstr(&msgbuf, msg.subject);