{"thread":{"id":"59458","subject":"[PATCH] sequencer: beautify subject of reverts of reverts","startedAt":"2023-03-23T16:48:06Z","lastAt":"2023-03-28T14:04:21Z","messageCount":8,"participants":["Oswald Buddenhagen","Kristoffer Haugsbakk","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"474018","messageId":"20230323162234.995465-1-oswald.buddenhagen@gmx.de","threadId":"59458","inReplyTo":null,"subject":"[PATCH] sequencer: beautify subject of reverts of reverts","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T16:22:34Z","receivedAt":"2023-03-23T16:48:06Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"Instead of generating a silly-looking `Revert \"Revert \"foo\"\"`, make it\n`Reapply \"foo\"`.\n\nSigned-off-by: Oswald Buddenhagen \n---\n sequencer.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3be23d7ca2..853b4ed334 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2234,6 +2234,9 @@ static int do_pick_commit(struct repository *r,\n \t\tif (opts->commit_use_reference) {\n \t\t\tstrbuf_addstr(&msgbuf,\n \t\t\t\t\"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n+\t\t} else if (starts_with(msg.subject, \"Revert \\\"\")) {\n+\t\t\tstrbuf_addstr(&msgbuf, \"Reapply \");\n+\t\t\tstrbuf_addstr(&msgbuf, msg.subject + 7);\n \t\t} else {\n \t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n \t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"474166","messageId":"210376fb-f830-4883-b576-2936dabc09cc@app.fastmail.com","threadId":"59458","inReplyTo":"20230323162234.995465-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH] sequencer: beautify subject of reverts of reverts","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-03-25T10:39:43Z","receivedAt":"2023-03-25T10:40:11Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Thu, Mar 23, 2023, at 17:22, Oswald Buddenhagen wrote:\n> Instead of generating a silly-looking `Revert \"Revert \"foo\"\"`, make it\n> `Reapply \"foo\"`.\n\nNice addition.\n\n $ echo change >> README.md\n $ ./bin-wrappers/git add README.md\n $ ./bin-wrappers/git commit -m 'A change'\n $ ./bin-wrappers/git revert --no-edit @\n $ ./bin-wrappers/git revert --no-edit @\n $ ./bin-wrappers/git log --oneline master..\n adfce56c6a (HEAD -> reapply) Reapply \"A change\"\n 395894c2ce Revert \"A change\"\n a01e3d6b3d A change\n 058643b69f sequencer: beautify subject of reverts of reverts\n\n-- \nKristoffer Haugsbakk\n"},{"id":"474262","messageId":"xmqqa5zxn85r.fsf@gitster.g","threadId":"59458","inReplyTo":"210376fb-f830-4883-b576-2936dabc09cc@app.fastmail.com","subject":"Re: [PATCH] sequencer: beautify subject of reverts of reverts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-27T22:25:52Z","receivedAt":"2023-03-27T22:26:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> On Thu, Mar 23, 2023, at 17:22, Oswald Buddenhagen wrote:\n>> Instead of generating a silly-looking `Revert \"Revert \"foo\"\"`, make it\n>> `Reapply \"foo\"`.\n>\n> Nice addition.\n>\n> $ echo change >> README.md\n> $ ./bin-wrappers/git add README.md\n> $ ./bin-wrappers/git commit -m 'A change'\n> $ ./bin-wrappers/git revert --no-edit @\n> $ ./bin-wrappers/git revert --no-edit @\n> $ ./bin-wrappers/git log --oneline master..\n> adfce56c6a (HEAD -> reapply) Reapply \"A change\"\n> 395894c2ce Revert \"A change\"\n> a01e3d6b3d A change\n> 058643b69f sequencer: beautify subject of reverts of reverts\n\nI think Oswald saw the end of a thread a few years ago that\ndiscussed a similar idea and the patch is a continuation of that\nthread, but what should happen when the re-application turns out to\nbe bad?\n\nThe significance of the act of reverting such a reapplication to the\nproject would be different from the initial revert (where \"yikes,\nthe change introduces a regression---let's revert to the state\nbefore the change, regroup, and see what we should do\" was the\nmotivation). Somebody thought that it now was OK to reintroduce the\nchange, presumably because the code paths around it now have become\nready for it and the phrase \"Reapply\" makes a perfect sense to\ndescribe it (\"Revert an earlier revert\" is not too bad, either,\nthough). But then it turns out it still was a bad idea. Should we\nsay\n\n Revert Reapply \"A change\"\n\nNext time somebody thinks the code paths around there are finally\nready, do they do\n\n Reapply Reapply \"A change\"\n\nor something else? That may be shorter, but even more cryptic than\n'Revert Revert Revert Revert \"A change\"'---\"Revert^4 \"A change\"\" the\nother thread proposed start to look less horrible. So, I dunno.\n"},{"id":"474267","messageId":"4f570b05-036d-45a1-8caa-cebfce3026c9@app.fastmail.com","threadId":"59458","inReplyTo":"xmqqa5zxn85r.fsf@gitster.g","subject":"Re: [PATCH] sequencer: beautify subject of reverts of reverts","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-03-28T06:04:43Z","receivedAt":"2023-03-28T06:05:13Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Tue, Mar 28, 2023, at 00:25, Junio C Hamano wrote:\n> \"\"Revert^4 \"A change\" the other thread proposed start to look less horrible.\n> So, I dunno.\n\nThat looks good. And the transformations are just:\n\n Revert \" → Reapply \"\n Reapply \" → Revert^3 \"\n Revert^ \" → Revert^ \"\n\n-- \nKristoffer Haugsbakk\n"},{"id":"474278","messageId":"ZCLqbmWsN/7Ihjg+@ugly","threadId":"59458","inReplyTo":"4f570b05-036d-45a1-8caa-cebfce3026c9@app.fastmail.com","subject":"Re: [PATCH] sequencer: beautify subject of reverts of reverts","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-28T13:23:58Z","receivedAt":"2023-03-28T13:24:12Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Tue, Mar 28, 2023 at 08:04:43AM +0200, Kristoffer Haugsbakk wrote:\n>On Tue, Mar 28, 2023, at 00:25, Junio C Hamano wrote:\n>> \"\"Revert^4 \"A change\" the other thread proposed start to look less horrible.\n>> So, I dunno.\n>\n>That looks good. And the transformations are just:\n>\n> Revert \" → Reapply \"\n> Reapply \" → Revert^3 \"\n> Revert^ \" → Revert^ \"\n>\ni thought about that already, and concluded that it's getting a bit \"too \nnerdy\" and over-engineered.\nthe main motivation for me is to break the dogmatism with which some \npeople are approaching the matter - \"$tool did it, so it is _the_ way\". \nset an example where the tool does something \"humane\", and you may \nchange some minds.\nwhen it gets to round 3 of reverting, i expect people to get creative:\n\n Revert \"foo\" again\n Reapply \"foo\", take 2\n etc.\n\nbut i don't mind, as long as `Revert \"Revert \"Revert \"foo\"\"\"` cannot be \nargued to be canon any more.\n"},{"id":"474279","messageId":"5333b921-0be6-49c7-b2dd-7d6ee5700385@app.fastmail.com","threadId":"59458","inReplyTo":"ZCLqbmWsN/7Ihjg+@ugly","subject":"Re: [PATCH] sequencer: beautify subject of reverts of reverts","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-03-28T13:43:46Z","receivedAt":"2023-03-28T13:46:46Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Tue, Mar 28, 2023, at 15:23, Oswald Buddenhagen wrote:\n> i thought about that already, and concluded that it's getting a bit \"too\n> nerdy\" and over-engineered.\n\nI see that point too.\n\n> the main motivation for me is to break the dogmatism with which some\n> people are approaching the matter - \"$tool did it, so it is _the_ way\".\n> set an example where the tool does something \"humane\", and you may\n> change some minds.\n\nGood thinking.\n\nYour patch does something “humane” without adding any involved logic. A\ngood default for those who don’t want to change the provided revert\nmessage. (Why? I would hope that they at least write something in the\nbody (after the subject) about why they are (reverting a\nrevert)/reapplying.)\n\n-- \nKristoffer Haugsbakk\nFrequent reverter\n"},{"id":"474299","messageId":"xmqq5yalj7qu.fsf@gitster.g","threadId":"59458","inReplyTo":"ZCLqbmWsN/7Ihjg+@ugly","subject":"Re: [PATCH] sequencer: beautify subject of reverts of reverts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-28T14:00:41Z","receivedAt":"2023-03-28T14:02:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n>>That looks good. And the transformations are just:\n>>\n>> Revert \" → Reapply \"\n>> Reapply \" → Revert^3 \"\n>> Revert^ \" → Revert^ \"\n>>\n> i thought about that already, and concluded that it's getting a bit\n> \"too nerdy\" and over-engineered.\n\nI agree that Revert^ looks too nerdy, and suspect that it was\nalso one of the reasons why the old discussion thread died out, even\nthough nobody in the thread explicitly mentioned it as the reason\nto reject it.\n\n> ...\n> but i don't mind, as long as `Revert \"Revert \"Revert \"foo\"\"\"` cannot\n> be argued to be canon any more.\n\nAt least, that form can be mechanically understood what it means\nwith a single rule (i.e. \"count the leading Revert\"), instead of\nrequiring a set of rules (i.e. \"if it begins with Reapply, then that\nis reverted twice, count Reapply, multiply by two, and then add the\nnumber of Revert\").\n\nI personally prefer a format that conveys how things happened in a\nway that can be easily understood, over a format that looks pretty\non surface but needs more special cases to understand what led to\nthe outcome it represents. But because reverting a commit ought to\nbe a rare event, and reverting a revert (or doing so recursively for\nmore levels) ought to be even rarer, it shouldn't matter all that\nmuch either way how we phrase the reversion of a revert, or reversion\nof such a reversion.\n\nThere will be folks who will complain no matter how we change the\nway we phrase the reversion of a revert, while there may also be\nother folks who like the change we make, and I feel that it would\nnot be worth my time to deal with the complaints for _this_\nparticular change that was proposed. As long as proponents for this\nchange promise to handle all such complaints on and off list, I am\nfine with the change, though.\n\nHaving said all that, I think this change, if we were to apply,\nshould be described in the documentation. Perhaps something along\nthis line?\n\ndiff --git c/Documentation/git-revert.txt w/Documentation/git-revert.txt\nindex d2e10d3dce..d09311dd8a 100644\n--- c/Documentation/git-revert.txt\n+++ w/Documentation/git-revert.txt\n@@ -31,6 +31,13 @@ both will discard uncommitted changes in your working directory.\n See \"Reset, restore and revert\" in linkgit:git[1] for the differences\n between the three commands.\n \n+The command by default gives \"Revert ''\" as the title to the\n+resulting commit when reverting the original commit whose title is\n+'<title>'. A commit that reverts such a reversion commit is given\n+\"Reapply '<title>' as its title. These can of course be edited in\n+the editor when the reason for reverting is described.\n+\n+\n OPTIONS\n -------\n <commit>...::\n\n\n"},{"id":"474300","messageId":"xmqqtty5ht2x.fsf@gitster.g","threadId":"59458","inReplyTo":"20230323162234.995465-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH] sequencer: beautify subject of reverts of reverts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-28T14:02:46Z","receivedAt":"2023-03-28T14:04:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 3be23d7ca2..853b4ed334 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2234,6 +2234,9 @@ static int do_pick_commit(struct repository *r,\n> \t\tif (opts->commit_use_reference) {\n> \t\t\tstrbuf_addstr(&msgbuf,\n> \t\t\t\t\"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n> +\t\t} else if (starts_with(msg.subject, \"Revert \\\"\")) {\n> +\t\t\tstrbuf_addstr(&msgbuf, \"Reapply \");\n> +\t\t\tstrbuf_addstr(&msgbuf, msg.subject + 7);\n> \t\t} else {\n> \t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n> \t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n\nTwo and half comments:\n\n * The hard-coded +7 looks fragile. Perhaps use skip_prefix?\n\n * During transition to introduce a new version of Git with this\n feature, when reverting an existing revert of a revert, care must\n be taken. Such a commit would begin as\n\n\tRevert \"Revert ...\n\n and turning it to\n\n\tReapply \"Revert ...\n\n may not be a good way to label such a reversion of a double\n revert without end-user confusion. As it is very likely that\n such a reversion commit was created by existing versions of Git,\n the easiest and least confusing way out would be to notice and\n refrain from touching such a commit.\n\n * The change lacks tests.\n\nRemoval of hardcoded +7 (i.e. the first point) may look like this.\n\n sequencer.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git i/sequencer.c w/sequencer.c\nindex 853b4ed334..520113ec63 100644\n--- i/sequencer.c\n+++ w/sequencer.c\n@@ -2227,6 +2227,8 @@ static int do_pick_commit(struct repository *r,\n \t */\n \n \tif (command == TODO_REVERT) {\n+\t\tconst char *original_title;\n+\n \t\tbase = commit;\n \t\tbase_label = msg.label;\n \t\tnext = parent;\n@@ -2234,9 +2236,9 @@ static int do_pick_commit(struct repository *r,\n \t\tif (opts->commit_use_reference) {\n \t\t\tstrbuf_addstr(&msgbuf,\n \t\t\t\t\"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n-\t\t} else if (starts_with(msg.subject, \"Revert \\\"\")) {\n+\t\t} else if (skip_prefix(msg.subject, \"Revert \\\"\", &original_title)) {\n-\t\t\tstrbuf_addstr(&msgbuf, \"Reapply \");\n+\t\t\tstrbuf_addstr(&msgbuf, \"Reapply \\\"\");\n-\t\t\tstrbuf_addstr(&msgbuf, msg.subject + 7);\n+\t\t\tstrbuf_addstr(&msgbuf, original_title);\n \t\t} else {\n \t\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n \t\t\tstrbuf_addstr(&msgbuf, msg.subject);\n"}]}