{"thread":{"id":"60623","subject":"Why is `revert` undocumented in interactive rebase todo help?","startedAt":"2023-12-18T06:53:37Z","lastAt":"2023-12-20T08:53:42Z","messageCount":8,"participants":["Michael Lohmann","Johannes Schindelin","Phillip Wood","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"485764","messageId":"20231218065313.55725-1-mi.al.lohmann@gmail.com","threadId":"60623","inReplyTo":null,"subject":"Why is `revert` undocumented in interactive rebase todo help?","fromName":"Michael Lohmann","fromEmail":"mial.lohmann@gmail.com","sentAt":"2023-12-18T06:53:13Z","receivedAt":"2023-12-18T06:53:37Z","isPatch":false,"sender":{"key":"mial.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"Hi!\nI wanted to align rebase and revert/cherry-pick a bit more (for the\nlatter I am currently finishing my patch for --show-current-patch and\nthen looked into possibly implementing --edit-todo). To avoid code\nduplication I wanted to reuse the existing interactive-rebase code as\nmuch as possible and ended up at the todo script parsing in the\nsequencer. I was a bit surprised to find that the file could already\nhandle the command `revert`, even though it isn't documented in\n`append_todo_help` of rebase-interactive.c - is that by choice or just\nmissing documentation?\n\nWhenever I wanted to achieve this I used `break` and then manually did\nthe revert, which obviously works fine, but it is much nicer to put the\ncommand in the todo file... (Now that I think about it I could also have\ndone it with `exec`, but that is also not the nicest solution :D ). The\nonly other command not mentioned is `noop` which is obviously not too\nuseful apart from distinguishing an empty list and aborting, so I\ntotally understand it missing.\n\nYes - in contrast to all the other options it does not have a single\nchar notation (and 'r' is obviously already taken und 'u' for undo as\nwell or 't' for the last letter), but why not show it in the list\nwithout it? Or maybe add 'v' for \"reVert\"?\n\nCheers\nMichael\n\nP.S.: @Johannes Schindelin I saw your work of making the todo files in\nthe sequencer more reusable and the many reworks/improvements, so I\nadded you in cc - I hope that was alright (otherwise I'll buy you a\nKölsch as an apology ;) )...\n"},{"id":"485772","messageId":"3e71666c-22a0-f52b-4025-dddb096e7e6c@gmx.de","threadId":"60623","inReplyTo":"20231218065313.55725-1-mi.al.lohmann@gmail.com","subject":"Re: Why is `revert` undocumented in interactive rebase todo help?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-12-18T10:53:57Z","receivedAt":"2023-12-18T10:54:01Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michael,\n\nOn Mon, 18 Dec 2023, Michael Lohmann wrote:\n\n> I wanted to align rebase and revert/cherry-pick a bit more (for the\n> latter I am currently finishing my patch for --show-current-patch and\n> then looked into possibly implementing --edit-todo). To avoid code\n> duplication I wanted to reuse the existing interactive-rebase code as\n> much as possible and ended up at the todo script parsing in the\n> sequencer. I was a bit surprised to find that the file could already\n> handle the command `revert`, even though it isn't documented in\n> `append_todo_help` of rebase-interactive.c - is that by choice or just\n> missing documentation?\n\nThe reason that it is not documented, and that it has no single-letter\n\"short command\", is that it is more of a historic accident than design\nthat interactive rebases support the \"revert\" command: In 004fefa754a4\n(sequencer: completely revamp the \"todo\" script parsing, 2016-10-21), I\nrevamped sequencer's parsing of the \"todo script\", in preparation for\nteaching it the trick to parse full-blown todo scripts of interactive\nrebases in addition to parsing the (hitherto quite limited) `cherry-pick`\nand `revert` \"scripts\", a trick that was completed with cca3be6ea15b\n(Merge branch 'js/prepare-sequencer', 2016-10-27). These days, `git rebase\n--interactive` uses that code to parse todo scripts.\n\nNaturally, to be able to continue parsing the \"revert scripts\", the\n`revert` command needed to be supported, and I never thought of disabling\nit specifically for interactive rebases.\n\n> Whenever I wanted to achieve this I used `break` and then manually did\n> the revert, which obviously works fine, but it is much nicer to put the\n> command in the todo file... (Now that I think about it I could also have\n> done it with `exec`, but that is also not the nicest solution :D ).\n\n\nRight. I often find myself adding commands like this one:\n\n\tx git revert -n <reverse-fixup> && git commit --amend --no-edit\n\nto amend a commit with a reversal of another commit, most prominently when\nI squashed a fixup into another commit than I had originally intended and\nnow need to fix that.\n\n> The only other command not mentioned is `noop` which is obviously not\n> too useful apart from distinguishing an empty list and aborting, so I\n> totally understand it missing.\n\nCorrect, that one is intentionally not described, for the reasons you\ndescribed.\n\n> Yes - in contrast to all the other options it does not have a single\n> char notation (and 'r' is obviously already taken und 'u' for undo as\n> well or 't' for the last letter), but why not show it in the list\n> without it? Or maybe add 'v' for \"reVert\"?\n\nSure ;-)\n\nCiao,\nJohannes\n"},{"id":"485778","messageId":"20231218152313.72896-1-mi.al.lohmann@gmail.com","threadId":"60623","inReplyTo":"3e71666c-22a0-f52b-4025-dddb096e7e6c@gmx.de","subject":"[PATCH] rebase-interactive: show revert option and add single letter shortcut","fromName":"Michael Lohmann","fromEmail":"mial.lohmann@gmail.com","sentAt":"2023-12-18T15:23:13Z","receivedAt":"2023-12-18T15:23:36Z","isPatch":true,"sender":{"key":"mial.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>\n---\n Documentation/git-rebase.txt | 3 +++\n rebase-interactive.c         | 1 +\n sequencer.c                  | 2 +-\n 3 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 1dd6555f66..75f6fe39a1 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -911,6 +911,9 @@ commit, the message from the final one is used.  You can also use\n \"fixup -C\" to get the same behavior as \"fixup -c\" except without opening\n an editor.\n \n+To revert a commit, add a line starting with \"revert\" followed by the commit\n+name.\n+\n `git rebase` will stop when \"pick\" has been replaced with \"edit\" or\n when a command fails due to merge errors. When you are done editing\n and/or resolving conflicts you can continue with `git rebase --continue`.\ndiff --git a/rebase-interactive.c b/rebase-interactive.c\nindex d9718409b3..e1fd1e09e3 100644\n--- a/rebase-interactive.c\n+++ b/rebase-interactive.c\n@@ -53,6 +53,7 @@ void append_todo_help(int command_count,\n \"                   commit's log message, unless -C is used, in which case\\n\"\n \"                   keep only this commit's message; -c is same as -C but\\n\"\n \"                   opens the editor\\n\"\n+\"v, revert <commit> = revert the changes introduced by that commit\\n\"\n \"x, exec <command> = run command (the rest of the line) using shell\\n\"\n \"b, break = stop here (continue rebase later with 'git rebase --continue')\\n\"\n \"d, drop <commit> = remove commit\\n\"\ndiff --git a/sequencer.c b/sequencer.c\nindex d584cac8ed..3c18f71ed6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1767,7 +1767,7 @@ static struct {\n \tconst char *str;\n } todo_command_info[] = {\n \t[TODO_PICK] = { 'p', \"pick\" },\n-\t[TODO_REVERT] = { 0,   \"revert\" },\n+\t[TODO_REVERT] = { 'v', \"revert\" },\n \t[TODO_EDIT] = { 'e', \"edit\" },\n \t[TODO_REWORD] = { 'r', \"reword\" },\n \t[TODO_FIXUP] = { 'f', \"fixup\" },\n-- \n2.43.0.77.g0ff82d959c\n\n"},{"id":"485783","messageId":"c2489476-f23b-4c03-8651-d6a8799ff67c@gmail.com","threadId":"60623","inReplyTo":"20231218152313.72896-1-mi.al.lohmann@gmail.com","subject":"Re: [PATCH] rebase-interactive: show revert option and add single letter shortcut","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-12-18T16:32:32Z","receivedAt":"2023-12-18T16:32:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Michael\n\nThanks for the patch, I'm wondering why you want to revert a commit when \nyou're rebasing. I think it would be helpful to explain that in the \ncommit message. In particular why it is necessary to revert a commit \nrather than simply dropping it (presumably you're using rebase to do \nsomething more that just rework a series of commits)\n\nBest Wishes\n\nPhillip\n\nOn 18/12/2023 15:23, Michael Lohmann wrote:\n> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>\n> ---\n>   Documentation/git-rebase.txt | 3 +++\n>   rebase-interactive.c         | 1 +\n>   sequencer.c                  | 2 +-\n>   3 files changed, 5 insertions(+), 1 deletion(-)\n> \n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index 1dd6555f66..75f6fe39a1 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -911,6 +911,9 @@ commit, the message from the final one is used.  You can also use\n>   \"fixup -C\" to get the same behavior as \"fixup -c\" except without opening\n>   an editor.\n>   \n> +To revert a commit, add a line starting with \"revert\" followed by the commit\n> +name.\n> +\n>   `git rebase` will stop when \"pick\" has been replaced with \"edit\" or\n>   when a command fails due to merge errors. When you are done editing\n>   and/or resolving conflicts you can continue with `git rebase --continue`.\n> diff --git a/rebase-interactive.c b/rebase-interactive.c\n> index d9718409b3..e1fd1e09e3 100644\n> --- a/rebase-interactive.c\n> +++ b/rebase-interactive.c\n> @@ -53,6 +53,7 @@ void append_todo_help(int command_count,\n>   \"                   commit's log message, unless -C is used, in which case\\n\"\n>   \"                   keep only this commit's message; -c is same as -C but\\n\"\n>   \"                   opens the editor\\n\"\n> +\"v, revert <commit> = revert the changes introduced by that commit\\n\"\n>   \"x, exec <command> = run command (the rest of the line) using shell\\n\"\n>   \"b, break = stop here (continue rebase later with 'git rebase --continue')\\n\"\n>   \"d, drop <commit> = remove commit\\n\"\n> diff --git a/sequencer.c b/sequencer.c\n> index d584cac8ed..3c18f71ed6 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1767,7 +1767,7 @@ static struct {\n>   \tconst char *str;\n>   } todo_command_info[] = {\n>   \t[TODO_PICK] = { 'p', \"pick\" },\n> -\t[TODO_REVERT] = { 0,   \"revert\" },\n> +\t[TODO_REVERT] = { 'v', \"revert\" },\n>   \t[TODO_EDIT] = { 'e', \"edit\" },\n>   \t[TODO_REWORD] = { 'r', \"reword\" },\n>   \t[TODO_FIXUP] = { 'f', \"fixup\" },\n"},{"id":"485786","messageId":"20231218170912.73535-1-mi.al.lohmann@gmail.com","threadId":"60623","inReplyTo":"c2489476-f23b-4c03-8651-d6a8799ff67c@gmail.com","subject":"[PATCH] rebase-interactive: show revert option and add single letter shortcut","fromName":"Michael Lohmann","fromEmail":"mial.lohmann@gmail.com","sentAt":"2023-12-18T17:09:12Z","receivedAt":"2023-12-18T17:09:23Z","isPatch":true,"sender":{"key":"mial.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"A `revert` in an interactive rebase can be useful, e.g. if a faulty\ncommit was pushed to the main branch already, so you can't just drop it.\nWhen you are already working in a feature branch you might just want to\nrevert said commit right where you branched off from main, so you can\ncontinue working on the feature you intend while still being up-to-date\notherwise.\n\nAnother reason why you might not want to drop a commit is if it is a\nwork in progress one and you want to properly fix it later, but for now\nneed to revert the changes. That way it is a lot cleaner to structure\nyour branch like this:\n\n    A---B---C       (B is WIP commit you cannot use as is)\n=>\n    A---B---~B---C  (temporarily revert B (called \"~B\") directly after\n                     it is created, until you find the time to fix it -\n                     at which point in time you will naturally drop the\n                     revert commit)\n\nThis way you still have the WIP patch, but \"your history is not broken\nthe whole time\".\n\nSigned-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>\n---\n Documentation/git-rebase.txt | 3 +++\n rebase-interactive.c         | 1 +\n sequencer.c                  | 2 +-\n 3 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 1dd6555f66..75f6fe39a1 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -911,6 +911,9 @@ commit, the message from the final one is used.  You can also use\n \"fixup -C\" to get the same behavior as \"fixup -c\" except without opening\n an editor.\n \n+To revert a commit, add a line starting with \"revert\" followed by the commit\n+name.\n+\n `git rebase` will stop when \"pick\" has been replaced with \"edit\" or\n when a command fails due to merge errors. When you are done editing\n and/or resolving conflicts you can continue with `git rebase --continue`.\ndiff --git a/rebase-interactive.c b/rebase-interactive.c\nindex d9718409b3..e1fd1e09e3 100644\n--- a/rebase-interactive.c\n+++ b/rebase-interactive.c\n@@ -53,6 +53,7 @@ void append_todo_help(int command_count,\n \"                   commit's log message, unless -C is used, in which case\\n\"\n \"                   keep only this commit's message; -c is same as -C but\\n\"\n \"                   opens the editor\\n\"\n+\"v, revert <commit> = revert the changes introduced by that commit\\n\"\n \"x, exec <command> = run command (the rest of the line) using shell\\n\"\n \"b, break = stop here (continue rebase later with 'git rebase --continue')\\n\"\n \"d, drop <commit> = remove commit\\n\"\ndiff --git a/sequencer.c b/sequencer.c\nindex d584cac8ed..3c18f71ed6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1767,7 +1767,7 @@ static struct {\n \tconst char *str;\n } todo_command_info[] = {\n \t[TODO_PICK] = { 'p', \"pick\" },\n-\t[TODO_REVERT] = { 0,   \"revert\" },\n+\t[TODO_REVERT] = { 'v', \"revert\" },\n \t[TODO_EDIT] = { 'e', \"edit\" },\n \t[TODO_REWORD] = { 'r', \"reword\" },\n \t[TODO_FIXUP] = { 'f', \"fixup\" },\n-- \n2.39.3 (Apple Git-145)\n\n"},{"id":"485787","messageId":"20231218172645.74133-1-mi.al.lohmann@gmail.com","threadId":"60623","inReplyTo":"20231218170912.73535-1-mi.al.lohmann@gmail.com","subject":"Re: [PATCH] rebase-interactive: show revert option and add single letter shortcut","fromName":"Michael Lohmann","fromEmail":"mial.lohmann@gmail.com","sentAt":"2023-12-18T17:26:45Z","receivedAt":"2023-12-18T17:26:52Z","isPatch":true,"sender":{"key":"mial.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"Hi Phillip\n\n> Thanks for the patch, I'm wondering why you want to revert a commit\n> when you're rebasing.\n\nI know this is probably not going to be the most used command, but I\nthink (depending on your workflow) it can be as important as e.g.\n`reset` or `update-ref`\n\n> I think it would be helpful to explain that in the commit message. In\n> particular why it is necessary to revert a commit rather than simply\n> dropping it (presumably you're using rebase to do something more that\n> just rework a series of commits)\n\nI gave two examples - maybe they can give a hint on why this actually\ncan be a useful feature. Over the last few years I might have only\nwanted to do this twice or so, but I know that I read through the help\nstring at least once to see how to do a revert.\n\nCheers!\n\nP.S. I am sorry I missed the \"v2\" in the previous patch - I am still\nlearning how to deal with the mailing list...\n"},{"id":"485790","messageId":"xmqqmsu7e4hp.fsf@gitster.g","threadId":"60623","inReplyTo":"20231218170912.73535-1-mi.al.lohmann@gmail.com","subject":"Re: [PATCH] rebase-interactive: show revert option and add single letter shortcut","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-18T18:43:14Z","receivedAt":"2023-12-18T18:43:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Lohmann <mial.lohmann@gmail.com> writes:\n\n> A `revert` in an interactive rebase can be useful, e.g. if a faulty\n> commit was pushed to the main branch already, so you can't just drop it.\n\nBut wouldn't that be typically done simply by running \"git revert\",\ntotally outside the context of \"git rebase -i\"?\n\nInteractive rebase is more geared toward rearranging an already\nbuilt history *after* such a \"git revert\" is made and possibly other\ncommits are made either before or after that commit that was created\nby \"git revert\".\n\nAnd when \"git rebase -i\" sees such a series of commits, e.g.,\n\n\tgit checkout -b side-branch master\n\tgit commit -m \"some work\"\n\tgit commit -m \"some more work\"\n\tgit revert -m \"revert a bad one for now\" master~4\n\tgit commit -m \"tentative alternative for what master~4 did\"\n\tgit rebase -i master\n\nThe \"revert a bad one for now\" commit looks to the machinery just\nlike any other commit in the todo list.\n\n> When you are already working in a feature branch you might just want to\n> revert said commit right where you branched off from main, so you can\n> continue working on the feature you intend while still being up-to-date\n> otherwise.\n\nYes, I can see why sometimes you want to work on a history with\neffects from certain commits removed.  But that does not explain why\nyou want to _insert_ a revert that you do not even have in the\nhistory anywhere before you start your interactive rebase.\n\n> Another reason why you might not want to drop a commit is if it is a\n> work in progress one and you want to properly fix it later, but for now\n> need to revert the changes. That way it is a lot cleaner to structure\n> your branch like this:\n>\n>     A---B---C       (B is WIP commit you cannot use as is)\n> =>\n>     A---B---~B---C  (temporarily revert B (called \"~B\") directly after\n>                      it is created, until you find the time to fix it -\n>                      at which point in time you will naturally drop the\n>                      revert commit)\n>\n> This way you still have the WIP patch, but \"your history is not broken\n> the whole time\".\n\nA much cleaner way to structure your branch is not to muck with such\ntentative changes *on* the branch you eventually want to store the\nfinal result on.  Fork another branch and rebase B away:\n\n    $ git checkout -b topic-ng topic [*]\n    $ git rebase -i A\n\nto obtain\n\n\tA---B---C    topic\n\t \\\n\t  C          topic-ng\n\nand then you'd build on top a better version of B eventually\n\n\tA---B---C\n\t \\\n\t  C---D---E---...---B*---X\n\nAnd after that you may \"rebase -i\" to refine the result, and then\nget rid of the tentative work:\n\n    $ work work work (still on topic-ng)\n    ...\n    $ git commit -m \"X\"\n    $ git rebase -i A\n    $ git branch -M topic \n\nNowhere in the above two flow, there is no need to manually insert a\nnew \"make a revert here of that other commit\" in the todo list.\nSo I am not sure if I buy the above, especially this part:\n\n> +To revert a commit, add a line starting with \"revert\" followed by the commit\n> +name.\n\nIt really smells like a confusing odd man out, among all other\nexisting instructions that are naturally created by the\nrebase/sequencer machinery and all the user needs to do is to\nshuffle them, never creating a new thing.\n\n\n\n[Footnote]\n\n * I do this too frequently that I often do without a separate -ng\n   branch; once you get used to the flow, you learn to do this kind\n   of thing on detached HEAD instead, so this step would look more\n   like\n\n   $ git checkout --detach topic\n\n   and the remainder of the above procedure will not change.  The\n   last step would become\n\n   $ git checkout -B topic\n\n   to bring me back to the target branch in a completed form.\n\n   One beauty about this \"detached HEAD\" approach is that output\n   from \"git reflog topic\" will show the refinement of the topic as\n   a single atomic event.\n\n"},{"id":"485838","messageId":"20231220085334.13234-1-mi.al.lohmann@gmail.com","threadId":"60623","inReplyTo":"xmqqmsu7e4hp.fsf@gitster.g","subject":"Re: [PATCH] rebase-interactive: show revert option and add single letter shortcut","fromName":"Michael Lohmann","fromEmail":"mial.lohmann@gmail.com","sentAt":"2023-12-20T08:53:34Z","receivedAt":"2023-12-20T08:53:42Z","isPatch":true,"sender":{"key":"mial.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"Hi Junio,\n\nOn 18. Dec 2023, at 19:43, Junio C Hamano <gitster@pobox.com> wrote:\n> Michael Lohmann <mial.lohmann@gmail.com> writes:\n>\n>> A `revert` in an interactive rebase can be useful, e.g. if a faulty\n>> commit was pushed to the main branch already, so you can't just drop it.\n> \n> But wouldn't that be typically done simply by running \"git revert\",\n> totally outside the context of \"git rebase -i\"?\n> \n> Interactive rebase is more geared toward rearranging an already\n> built history *after* such a \"git revert\" is made and possibly other\n> commits are made either before or after that commit that was created\n> by \"git revert\".\n\n\nRight - I recently found myself in a situation where a coworker merged a\nfaulty commit leading to a build failure and (given that only the two of\nus actively worked on that project) we coordinated that he would prepare\na proper fix, while I wanted to rebase my current feature branch on\nmaster, but with that commit reverted. For the sake of a clean history I\npreferred to have the revert commit right at the merge-base, instead of\nsomewhere in the middle of all of my commits.\n\nBut you are right - if there are more than two people working on a\nproject, the typical way of properly doing this would be to revert the\noffending commit in the master branch until a fix is available.\n\nYou can also do the revert you want to create in your feature branch and\ndirectly update the master:\n- `git rebase -i origin/master`\n- add the following two lines to the top of the todo list:\n    revert B\n    exec git push origin @:refs/heads/revert-commit\n\nInstead of\n- `git switch origin/master -c revert-commit`\n- `git revert B`\n- `git push origin HEAD`\n- `git switch -`\n- `git rebase revert-commit`\n\nSo with the interactive revert you can create the \"revert-commit” branch\n\"directly from within your own branch”. But indeed - it really is not\nneeded...\n\n> A much cleaner way to structure your branch is not to muck with such\n> tentative changes *on* the branch you eventually want to store the\n> final result on.  Fork another branch and rebase B away:\n\nI really like that workflow! I’ll adapt it :)\n\nSo in total: I don’t think documenting this is necessary (that is also\nwhy my first message was not directly the patch, but the question why\nthis is undocumented) and it might even lead to the unclean workflow\nthat I ended up having, so even from that perspective it might not be a\ngood thing.\n\nThank you very much for this very detailed explanation of the workflow!\nMichael\n\nP.S. I am sorry - the first reply only went directly to Junio and not the\nmailing list\n"}]}