Why is `revert` undocumented in interactive rebase todo help?

Why is `revert` undocumented in interactive rebase todo help?

[Michael Lohmann]

Hi!
I wanted to align rebase and revert/cherry-pick a bit more (for the
latter I am currently finishing my patch for --show-current-patch and
then looked into possibly implementing --edit-todo). To avoid code
duplication I wanted to reuse the existing interactive-rebase code as
much as possible and ended up at the todo script parsing in the
sequencer. I was a bit surprised to find that the file could already
handle the command `revert`, even though it isn't documented in
`append_todo_help` of rebase-interactive.c - is that by choice or just
missing documentation?

Whenever I wanted to achieve this I used `break` and then manually did
the revert, which obviously works fine, but it is much nicer to put the
command in the todo file... (Now that I think about it I could also have
done it with `exec`, but that is also not the nicest solution :D ). The
only other command not mentioned is `noop` which is obviously not too
useful apart from distinguishing an empty list and aborting, so I
totally understand it missing.

Yes - in contrast to all the other options it does not have a single
char notation (and 'r' is obviously already taken und 'u' for undo as
well or 't' for the last letter), but why not show it in the list
without it? Or maybe add 'v' for "reVert"?

Cheers
Michael

P.S.: @Johannes Schindelin I saw your work of making the todo files in
the sequencer more reusable and the many reworks/improvements, so I
added you in cc - I hope that was alright (otherwise I'll buy you a
Kölsch as an apology ;) )...

Re: Why is `revert` undocumented in interactive rebase todo help?

[Johannes Schindelin]

Hi Michael,

On Mon, 18 Dec 2023, Michael Lohmann wrote:

> I wanted to align rebase and revert/cherry-pick a bit more (for the
> latter I am currently finishing my patch for --show-current-patch and
> then looked into possibly implementing --edit-todo). To avoid code
> duplication I wanted to reuse the existing interactive-rebase code as
> much as possible and ended up at the todo script parsing in the
> sequencer. I was a bit surprised to find that the file could already
> handle the command `revert`, even though it isn't documented in
> `append_todo_help` of rebase-interactive.c - is that by choice or just
> missing documentation?

The reason that it is not documented, and that it has no single-letter
"short command", is that it is more of a historic accident than design
that interactive rebases support the "revert" command: In 004fefa754a4
(sequencer: completely revamp the "todo" script parsing, 2016-10-21), I
revamped sequencer's parsing of the "todo script", in preparation for
teaching it the trick to parse full-blown todo scripts of interactive
rebases in addition to parsing the (hitherto quite limited) `cherry-pick`
and `revert` "scripts", a trick that was completed with cca3be6ea15b
(Merge branch 'js/prepare-sequencer', 2016-10-27). These days, `git rebase
--interactive` uses that code to parse todo scripts.

Naturally, to be able to continue parsing the "revert scripts", the
`revert` command needed to be supported, and I never thought of disabling
it specifically for interactive rebases.

> Whenever I wanted to achieve this I used `break` and then manually did
> the revert, which obviously works fine, but it is much nicer to put the
> command in the todo file... (Now that I think about it I could also have
> done it with `exec`, but that is also not the nicest solution :D ).


Right. I often find myself adding commands like this one:

	x git revert -n <reverse-fixup> && git commit --amend --no-edit

to amend a commit with a reversal of another commit, most prominently when
I squashed a fixup into another commit than I had originally intended and
now need to fix that.

> The only other command not mentioned is `noop` which is obviously not
> too useful apart from distinguishing an empty list and aborting, so I
> totally understand it missing.

Correct, that one is intentionally not described, for the reasons you
described.

> Yes - in contrast to all the other options it does not have a single
> char notation (and 'r' is obviously already taken und 'u' for undo as
> well or 't' for the last letter), but why not show it in the list
> without it? Or maybe add 'v' for "reVert"?

Sure ;-)

Ciao,
Johannes

[PATCH] rebase-interactive: show revert option and add single letter shortcut

[Michael Lohmann]

Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---
 Documentation/git-rebase.txt | 3 +++
 rebase-interactive.c         | 1 +
 sequencer.c                  | 2 +-
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 1dd6555f66..75f6fe39a1 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -911,6 +911,9 @@ commit, the message from the final one is used.  You can also use
 "fixup -C" to get the same behavior as "fixup -c" except without opening
 an editor.
 
+To revert a commit, add a line starting with "revert" followed by the commit
+name.
+
 `git rebase` will stop when "pick" has been replaced with "edit" or
 when a command fails due to merge errors. When you are done editing
 and/or resolving conflicts you can continue with `git rebase --continue`.
diff --git a/rebase-interactive.c b/rebase-interactive.c
index d9718409b3..e1fd1e09e3 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -53,6 +53,7 @@ void append_todo_help(int command_count,
 "                   commit's log message, unless -C is used, in which case\n"
 "                   keep only this commit's message; -c is same as -C but\n"
 "                   opens the editor\n"
+"v, revert <commit> = revert the changes introduced by that commit\n"
 "x, exec <command> = run command (the rest of the line) using shell\n"
 "b, break = stop here (continue rebase later with 'git rebase --continue')\n"
 "d, drop <commit> = remove commit\n"
diff --git a/sequencer.c b/sequencer.c
index d584cac8ed..3c18f71ed6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1767,7 +1767,7 @@ static struct {
 	const char *str;
 } todo_command_info[] = {
 	[TODO_PICK] = { 'p', "pick" },
-	[TODO_REVERT] = { 0,   "revert" },
+	[TODO_REVERT] = { 'v', "revert" },
 	[TODO_EDIT] = { 'e', "edit" },
 	[TODO_REWORD] = { 'r', "reword" },
 	[TODO_FIXUP] = { 'f', "fixup" },
-- 
2.43.0.77.g0ff82d959c

Re: [PATCH] rebase-interactive: show revert option and add single letter shortcut

[Phillip Wood]

Hi Michael

Thanks for the patch, I'm wondering why you want to revert a commit when 
you're rebasing. I think it would be helpful to explain that in the 
commit message. In particular why it is necessary to revert a commit 
rather than simply dropping it (presumably you're using rebase to do 
something more that just rework a series of commits)

Best Wishes

Phillip

On 18/12/2023 15:23, Michael Lohmann wrote:
> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> ---
>   Documentation/git-rebase.txt | 3 +++
>   rebase-interactive.c         | 1 +
>   sequencer.c                  | 2 +-
>   3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 1dd6555f66..75f6fe39a1 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -911,6 +911,9 @@ commit, the message from the final one is used.  You can also use
>   "fixup -C" to get the same behavior as "fixup -c" except without opening
>   an editor.
>   
> +To revert a commit, add a line starting with "revert" followed by the commit
> +name.
> +
>   `git rebase` will stop when "pick" has been replaced with "edit" or
>   when a command fails due to merge errors. When you are done editing
>   and/or resolving conflicts you can continue with `git rebase --continue`.
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index d9718409b3..e1fd1e09e3 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -53,6 +53,7 @@ void append_todo_help(int command_count,
>   "                   commit's log message, unless -C is used, in which case\n"
>   "                   keep only this commit's message; -c is same as -C but\n"
>   "                   opens the editor\n"
> +"v, revert <commit> = revert the changes introduced by that commit\n"
>   "x, exec <command> = run command (the rest of the line) using shell\n"
>   "b, break = stop here (continue rebase later with 'git rebase --continue')\n"
>   "d, drop <commit> = remove commit\n"
> diff --git a/sequencer.c b/sequencer.c
> index d584cac8ed..3c18f71ed6 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1767,7 +1767,7 @@ static struct {
>   	const char *str;
>   } todo_command_info[] = {
>   	[TODO_PICK] = { 'p', "pick" },
> -	[TODO_REVERT] = { 0,   "revert" },
> +	[TODO_REVERT] = { 'v', "revert" },
>   	[TODO_EDIT] = { 'e', "edit" },
>   	[TODO_REWORD] = { 'r', "reword" },
>   	[TODO_FIXUP] = { 'f', "fixup" },

[PATCH] rebase-interactive: show revert option and add single letter shortcut

[Michael Lohmann]

A `revert` in an interactive rebase can be useful, e.g. if a faulty
commit was pushed to the main branch already, so you can't just drop it.
When you are already working in a feature branch you might just want to
revert said commit right where you branched off from main, so you can
continue working on the feature you intend while still being up-to-date
otherwise.

Another reason why you might not want to drop a commit is if it is a
work in progress one and you want to properly fix it later, but for now
need to revert the changes. That way it is a lot cleaner to structure
your branch like this:

    A---B---C       (B is WIP commit you cannot use as is)
=>
    A---B---~B---C  (temporarily revert B (called "~B") directly after
                     it is created, until you find the time to fix it -
                     at which point in time you will naturally drop the
                     revert commit)

This way you still have the WIP patch, but "your history is not broken
the whole time".

Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---
 Documentation/git-rebase.txt | 3 +++
 rebase-interactive.c         | 1 +
 sequencer.c                  | 2 +-
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 1dd6555f66..75f6fe39a1 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -911,6 +911,9 @@ commit, the message from the final one is used.  You can also use
 "fixup -C" to get the same behavior as "fixup -c" except without opening
 an editor.
 
+To revert a commit, add a line starting with "revert" followed by the commit
+name.
+
 `git rebase` will stop when "pick" has been replaced with "edit" or
 when a command fails due to merge errors. When you are done editing
 and/or resolving conflicts you can continue with `git rebase --continue`.
diff --git a/rebase-interactive.c b/rebase-interactive.c
index d9718409b3..e1fd1e09e3 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -53,6 +53,7 @@ void append_todo_help(int command_count,
 "                   commit's log message, unless -C is used, in which case\n"
 "                   keep only this commit's message; -c is same as -C but\n"
 "                   opens the editor\n"
+"v, revert <commit> = revert the changes introduced by that commit\n"
 "x, exec <command> = run command (the rest of the line) using shell\n"
 "b, break = stop here (continue rebase later with 'git rebase --continue')\n"
 "d, drop <commit> = remove commit\n"
diff --git a/sequencer.c b/sequencer.c
index d584cac8ed..3c18f71ed6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1767,7 +1767,7 @@ static struct {
 	const char *str;
 } todo_command_info[] = {
 	[TODO_PICK] = { 'p', "pick" },
-	[TODO_REVERT] = { 0,   "revert" },
+	[TODO_REVERT] = { 'v', "revert" },
 	[TODO_EDIT] = { 'e', "edit" },
 	[TODO_REWORD] = { 'r', "reword" },
 	[TODO_FIXUP] = { 'f', "fixup" },
-- 
2.39.3 (Apple Git-145)

Re: [PATCH] rebase-interactive: show revert option and add single letter shortcut

[Michael Lohmann]

Hi Phillip

> Thanks for the patch, I'm wondering why you want to revert a commit
> when you're rebasing.

I know this is probably not going to be the most used command, but I
think (depending on your workflow) it can be as important as e.g.
`reset` or `update-ref`

> I think it would be helpful to explain that in the commit message. In
> particular why it is necessary to revert a commit rather than simply
> dropping it (presumably you're using rebase to do something more that
> just rework a series of commits)

I gave two examples - maybe they can give a hint on why this actually
can be a useful feature. Over the last few years I might have only
wanted to do this twice or so, but I know that I read through the help
string at least once to see how to do a revert.

Cheers!

P.S. I am sorry I missed the "v2" in the previous patch - I am still
learning how to deal with the mailing list...

Re: [PATCH] rebase-interactive: show revert option and add single letter shortcut

[Junio C Hamano]

Michael Lohmann <mial.lohmann@gmail.com> writes:

> A `revert` in an interactive rebase can be useful, e.g. if a faulty
> commit was pushed to the main branch already, so you can't just drop it.

But wouldn't that be typically done simply by running "git revert",
totally outside the context of "git rebase -i"?

Interactive rebase is more geared toward rearranging an already
built history *after* such a "git revert" is made and possibly other
commits are made either before or after that commit that was created
by "git revert".

And when "git rebase -i" sees such a series of commits, e.g.,

	git checkout -b side-branch master
	git commit -m "some work"
	git commit -m "some more work"
	git revert -m "revert a bad one for now" master~4
	git commit -m "tentative alternative for what master~4 did"
	git rebase -i master

The "revert a bad one for now" commit looks to the machinery just
like any other commit in the todo list.

> When you are already working in a feature branch you might just want to
> revert said commit right where you branched off from main, so you can
> continue working on the feature you intend while still being up-to-date
> otherwise.

Yes, I can see why sometimes you want to work on a history with
effects from certain commits removed.  But that does not explain why
you want to _insert_ a revert that you do not even have in the
history anywhere before you start your interactive rebase.

> Another reason why you might not want to drop a commit is if it is a
> work in progress one and you want to properly fix it later, but for now
> need to revert the changes. That way it is a lot cleaner to structure
> your branch like this:
>
>     A---B---C       (B is WIP commit you cannot use as is)
> =>
>     A---B---~B---C  (temporarily revert B (called "~B") directly after
>                      it is created, until you find the time to fix it -
>                      at which point in time you will naturally drop the
>                      revert commit)
>
> This way you still have the WIP patch, but "your history is not broken
> the whole time".

A much cleaner way to structure your branch is not to muck with such
tentative changes *on* the branch you eventually want to store the
final result on.  Fork another branch and rebase B away:

    $ git checkout -b topic-ng topic [*]
    $ git rebase -i A

to obtain

	A---B---C    topic
	 \
	  C          topic-ng

and then you'd build on top a better version of B eventually

	A---B---C
	 \
	  C---D---E---...---B*---X

And after that you may "rebase -i" to refine the result, and then
get rid of the tentative work:

    $ work work work (still on topic-ng)
    ...
    $ git commit -m "X"
    $ git rebase -i A
    $ git branch -M topic 

Nowhere in the above two flow, there is no need to manually insert a
new "make a revert here of that other commit" in the todo list.
So I am not sure if I buy the above, especially this part:

> +To revert a commit, add a line starting with "revert" followed by the commit
> +name.

It really smells like a confusing odd man out, among all other
existing instructions that are naturally created by the
rebase/sequencer machinery and all the user needs to do is to
shuffle them, never creating a new thing.



[Footnote]

 * I do this too frequently that I often do without a separate -ng
   branch; once you get used to the flow, you learn to do this kind
   of thing on detached HEAD instead, so this step would look more
   like

   $ git checkout --detach topic

   and the remainder of the above procedure will not change.  The
   last step would become

   $ git checkout -B topic

   to bring me back to the target branch in a completed form.

   One beauty about this "detached HEAD" approach is that output
   from "git reflog topic" will show the refinement of the topic as
   a single atomic event.

Re: [PATCH] rebase-interactive: show revert option and add single letter shortcut

[Michael Lohmann]

Hi Junio,

On 18. Dec 2023, at 19:43, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Lohmann <mial.lohmann@gmail.com> writes:
>
>> A `revert` in an interactive rebase can be useful, e.g. if a faulty
>> commit was pushed to the main branch already, so you can't just drop it.
> 
> But wouldn't that be typically done simply by running "git revert",
> totally outside the context of "git rebase -i"?
> 
> Interactive rebase is more geared toward rearranging an already
> built history *after* such a "git revert" is made and possibly other
> commits are made either before or after that commit that was created
> by "git revert".


Right - I recently found myself in a situation where a coworker merged a
faulty commit leading to a build failure and (given that only the two of
us actively worked on that project) we coordinated that he would prepare
a proper fix, while I wanted to rebase my current feature branch on
master, but with that commit reverted. For the sake of a clean history I
preferred to have the revert commit right at the merge-base, instead of
somewhere in the middle of all of my commits.

But you are right - if there are more than two people working on a
project, the typical way of properly doing this would be to revert the
offending commit in the master branch until a fix is available.

You can also do the revert you want to create in your feature branch and
directly update the master:
- `git rebase -i origin/master`
- add the following two lines to the top of the todo list:
    revert B
    exec git push origin @:refs/heads/revert-commit

Instead of
- `git switch origin/master -c revert-commit`
- `git revert B`
- `git push origin HEAD`
- `git switch -`
- `git rebase revert-commit`

So with the interactive revert you can create the "revert-commit” branch
"directly from within your own branch”. But indeed - it really is not
needed...

> A much cleaner way to structure your branch is not to muck with such
> tentative changes *on* the branch you eventually want to store the
> final result on.  Fork another branch and rebase B away:

I really like that workflow! I’ll adapt it :)

So in total: I don’t think documenting this is necessary (that is also
why my first message was not directly the patch, but the question why
this is undocumented) and it might even lead to the unclean workflow
that I ended up having, so even from that perspective it might not be a
good thing.

Thank you very much for this very detailed explanation of the workflow!
Michael

P.S. I am sorry - the first reply only went directly to Junio and not the
mailing list