| Message ID | 20230323162234.995465-1-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sequencer: beautify subject of reverts of reverts | expand |
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" <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.
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> "
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.
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.)
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>...::
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);
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);
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(+)