Aborting git rebase --edit-todo

Aborting git rebase --edit-todo

[Victor Toni]

When doing a commit or choosing what to do for an interactive rebase
one can just wipe the whole content of the editor, save and close to
abort the action.
While doing a `git rebase --edit-todo` I came to the conclusion that I
would like to abort the edit and did the same. The final `git rebase
--continue` got me rid of the rest of the commits...
(Fortunately the "missing" commits could be rescued by looking into
`.git/logs/HEAD` so thumbs up for that. )
Unfortunately the behaviour of `--edit-todo` was a bit surprising and
somehow doesn't feel consistent with the other actions involving an
editor.

Can this be considered a bug?

Best regards,
Victor

Re: Aborting git rebase --edit-todo

[Junio C Hamano]

Victor Toni <victor.toni@gmail.com> writes:

> When doing a commit or choosing what to do for an interactive rebase
> one can just wipe the whole content of the editor, save and close to
> abort the action.
> While doing a `git rebase --edit-todo` I came to the conclusion that I
> would like to abort the edit and did the same. The final `git rebase
> --continue` got me rid of the rest of the commits...
> (Fortunately the "missing" commits could be rescued by looking into
> `.git/logs/HEAD` so thumbs up for that. )
> Unfortunately the behaviour of `--edit-todo` was a bit surprising and
> somehow doesn't feel consistent with the other actions involving an
> editor.
>
> Can this be considered a bug?

It is rather unusual (or almost always wrong) to have a totally
empty commit log or initial todo list, so it is understandable for
Git in these situations to stop without doing anything further.

There is no other sensible interpretations of what you are telling
Git to you by returning an empty buffer---it is extremely unlikely
you want to create a commit with no log message (without explicitly
allowing it with --allow-empty-message, the command is likely to
fail anyway), and it is extremely unlikely that you wanted to just
reset the tip of the branch to the --onto commit.

Once an interactive rebase session has started and you are given the
remainder of the steps to edit and you give an empty buffer back,
however, there are two possible interpretations that are equally
sensible, I would think.

 - One is that you are signaling that you are done with the rebase
   session and all the remaining commits are to be discarded.  

 - The other is that you botched editing the todo list, and you wish
   Git to give you another chance to edit it again.

I think the implementor chose the first interpretation.  The "drop"
insn is a relatively recent invention, and back when it was missing
from the vocabulary, I do not think it was possible to say " discard
all the rest" without emptying the todo list, so that design is
understandable.

Now we have the "drop" verb, the latter interpretation becomes
possible without making it impossible for the user to express the
former.  It might be a good idea to

 (1) save away the original before allowing --edit-todo to edit,

 (2) open the editor, and

 (3) when getting an empty buffer back, go back to step (2) using
     the back-up made in step (1).

Either way, the todo list editor buffer can have additional comment
instructing what happens when the buffer is emptied.

I have no strong opinion on this one myself.  Deferring to Dscho,
who may have a lot more to say on the design issue around this
feature than I do.

Thanks.

Re: Aborting git rebase --edit-todo

[Carlo Arenas]

On Thu, Sep 3, 2020 at 10:47 AM Junio C Hamano <gitster@pobox.com> wrote:

> Now we have the "drop" verb, the latter interpretation becomes
> possible without making it impossible for the user to express the
> former.

and for people that would like to enforce the use of the drop verb
there is configuration that prevents deleted lines to "silently"
dropping commits since 5a5445d878 (rebase-interactive: warn if commit
is dropped with `rebase --edit-todo', 2020-01-28) :

  rebase.missingCommitsCheck

AFAIK the correct "signal" to abort is to instruct your editor to exit
with non zero (ex: in vi using <esc>:cq), but agree it could be
confusing or "inconsistent" and might be worth adding it a message at
the footer

Carlo

Re: Aborting git rebase --edit-todo

[Victor Toni]

> > Now we have the "drop" verb, the latter interpretation becomes
> > possible without making it impossible for the user to express the
> > former.
>
> and for people that would like to enforce the use of the drop verb
> there is configuration that prevents deleted lines to "silently"
> dropping commits since 5a5445d878 (rebase-interactive: warn if commit
> is dropped with `rebase --edit-todo', 2020-01-28) :
>
>   rebase.missingCommitsCheck
>

Didn't know about that one, will add it right away to my .gitconfig. Thanks.

> AFAIK the correct "signal" to abort is to instruct your editor to exit
> with non zero (ex: in vi using <esc>:cq), but agree it could be
> confusing or "inconsistent" and might be worth adding it a message at
> the footer
>

This sounds easier than it might be. On some machines I have (Git for)
Windows and use a "regular" text editor which I guess I would have to
kill to make it exit in a way to be recognized by git.
Even when using Linux I never needed to make my editor exit with a non
zero code. From a coding perspective this might work, from a usability
perspective I really don't like it.

Victor

Re: Aborting git rebase --edit-todo

[Junio C Hamano]

Victor Toni <victor.toni@gmail.com> writes:

> Even when using Linux I never needed to make my editor exit with a non
> zero code. From a coding perspective this might work, from a usability
> perspective I really don't like it.

I do not use --edit-todo myself so personally I do not care that
deeply either way, but I have to agree that it is not the most
natural UI to make your editor to exit with non-zero status to
signal something to the program that opened the editor.

Re: Aborting git rebase --edit-todo

[Victor Toni]

> It is rather unusual (or almost always wrong) to have a totally
> empty commit log or initial todo list, so it is understandable for
> Git in these situations to stop without doing anything further.
>
> There is no other sensible interpretations of what you are telling
> Git to you by returning an empty buffer---it is extremely unlikely
> you want to create a commit with no log message (without explicitly
> allowing it with --allow-empty-message, the command is likely to
> fail anyway), and it is extremely unlikely that you wanted to just
> reset the tip of the branch to the --onto commit.
>
> Once an interactive rebase session has started and you are given the
> remainder of the steps to edit and you give an empty buffer back,
> however, there are two possible interpretations that are equally
> sensible, I would think.
>
>  - One is that you are signaling that you are done with the rebase
>    session and all the remaining commits are to be discarded.
>
>  - The other is that you botched editing the todo list, and you wish
>    Git to give you another chance to edit it again.
>
> I think the implementor chose the first interpretation.  The "drop"
> insn is a relatively recent invention, and back when it was missing
> from the vocabulary, I do not think it was possible to say " discard
> all the rest" without emptying the todo list, so that design is
> understandable.
>
> Now we have the "drop" verb, the latter interpretation becomes
> possible without making it impossible for the user to express the
> former.  It might be a good idea to
>
>  (1) save away the original before allowing --edit-todo to edit,
>
>  (2) open the editor, and
>
>  (3) when getting an empty buffer back, go back to step (2) using
>      the back-up made in step (1).
>
> Either way, the todo list editor buffer can have additional comment
> instructing what happens when the buffer is emptied.
>

Personally I would like to see your approach (1,2,3) implemented
because it is not destructive. If the user wants to achieve something
different he can retry.
Option / interpretation a)

>  - One is that you are signaling that you are done with the rebase
>    session and all the remaining commits are to be discarded.

is more difficult to recover from. (I'm still thankful for `.git/logs/HEAD`)

Re: Aborting git rebase --edit-todo

[Carlo Arenas]

On Thu, Sep 3, 2020 at 12:36 PM Victor Toni <victor.toni@gmail.com> wrote:
>
> is more difficult to recover from. (I'm still thankful for `.git/logs/HEAD`)

another command that might not be that well known and that will always
recover from a botched merge (or a rebase in this case) :

  $ git reset --hard ORIG_HEAD

Carlo

PS. `git reflog` might be also interesting; sorry if going slightly off topic

Re: Aborting git rebase --edit-todo

[Victor Toni]

On Do., 3. Sept. 2020 um 21:59 Uhr schrieb Carlo Arenas <carenas@gmail.com>:
>
> On Thu, Sep 3, 2020 at 12:36 PM Victor Toni <victor.toni@gmail.com> wrote:
> >
> > is more difficult to recover from. (I'm still thankful for `.git/logs/HEAD`)
>
> another command that might not be that well known and that will always
> recover from a botched merge (or a rebase in this case) :
>
>   $ git reset --hard ORIG_HEAD
>
I tried it on a backup copy of my repository (created directly after
the "incident").
Unfortunately it would have missed two commits.

> PS. `git reflog` might be also interesting; sorry if going slightly off topic

Thanks for that. Never used it before. git reflog would have helped
directly since it shows the last real commit I did before messing
things up.
From there it's a walk in the park.

Victor

Re: Aborting git rebase --edit-todo

[Junio C Hamano]

Victor Toni <victor.toni@gmail.com> writes:

>> I think the implementor chose the first interpretation.  The "drop"
>> insn is a relatively recent invention, and back when it was missing
>> from the vocabulary, I do not think it was possible to say " discard
>> all the rest" without emptying the todo list, so that design is
>> understandable.
>>
>> Now we have the "drop" verb, the latter interpretation becomes
>> possible without making it impossible for the user to express the
>> former.  It might be a good idea to
>>
>>  (1) save away the original before allowing --edit-todo to edit,
>>
>>  (2) open the editor, and
>>
>>  (3) when getting an empty buffer back, go back to step (2) using
>>      the back-up made in step (1).
>>
>> Either way, the todo list editor buffer can have additional comment
>> instructing what happens when the buffer is emptied.
>>
> Personally I would like to see your approach (1,2,3) implemented
> because it is not destructive. If the user wants to achieve something
> different he can retry.

Obviously I agree that the approach would be nicer than the status
quo.  It would not be as trivial as a microproject, but would be a
good bite-sized starter-task for those aspiring developers who want
to dip their toes in the water to start hacking on the codebase ;-)

Re: Aborting git rebase --edit-todo

[Victor Toni]

Junio C Hamano <gitster@pobox.com> writes:
>
> Victor Toni <victor.toni@gmail.com> writes:
>
> >> I think the implementor chose the first interpretation.  The "drop"
> >> insn is a relatively recent invention, and back when it was missing
> >> from the vocabulary, I do not think it was possible to say " discard
> >> all the rest" without emptying the todo list, so that design is
> >> understandable.
> >>
> >> Now we have the "drop" verb, the latter interpretation becomes
> >> possible without making it impossible for the user to express the
> >> former.  It might be a good idea to
> >>
> >>  (1) save away the original before allowing --edit-todo to edit,
> >>
> >>  (2) open the editor, and
> >>
> >>  (3) when getting an empty buffer back, go back to step (2) using
> >>      the back-up made in step (1).
> >>
> >> Either way, the todo list editor buffer can have additional comment
> >> instructing what happens when the buffer is emptied.
> >>
> > Personally I would like to see your approach (1,2,3) implemented
> > because it is not destructive. If the user wants to achieve something
> > different he can retry.
>
> Obviously I agree that the approach would be nicer than the status
> quo.  It would not be as trivial as a microproject, but would be a
> good bite-sized starter-task for those aspiring developers who want
> to dip their toes in the water to start hacking on the codebase ;-)
>
Nice try ;) Speaking of toes ... I'm currently involved in another
project from tip to toe.
I would like to come back to your offer sometime next year when I've
completed the other one.
Especially since I'd have to polish up my buried C skills... C didn't
get GC lately, did it? ;)

Re: Aborting git rebase --edit-todo

[Johannes Schindelin]

Hi Victor,

On Thu, 3 Sep 2020, Victor Toni wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> >
> > Victor Toni <victor.toni@gmail.com> writes:
> >
> > >> I think the implementor chose the first interpretation.  The "drop"
> > >> insn is a relatively recent invention, and back when it was missing
> > >> from the vocabulary, I do not think it was possible to say " discard
> > >> all the rest" without emptying the todo list, so that design is
> > >> understandable.
> > >>
> > >> Now we have the "drop" verb, the latter interpretation becomes
> > >> possible without making it impossible for the user to express the
> > >> former.  It might be a good idea to
> > >>
> > >>  (1) save away the original before allowing --edit-todo to edit,
> > >>
> > >>  (2) open the editor, and
> > >>
> > >>  (3) when getting an empty buffer back, go back to step (2) using
> > >>      the back-up made in step (1).
> > >>
> > >> Either way, the todo list editor buffer can have additional comment
> > >> instructing what happens when the buffer is emptied.
> > >>
> > > Personally I would like to see your approach (1,2,3) implemented
> > > because it is not destructive. If the user wants to achieve something
> > > different he can retry.
> >
> > Obviously I agree that the approach would be nicer than the status
> > quo.  It would not be as trivial as a microproject, but would be a
> > good bite-sized starter-task for those aspiring developers who want
> > to dip their toes in the water to start hacking on the codebase ;-)
> >
> Nice try ;) Speaking of toes ... I'm currently involved in another
> project from tip to toe.
> I would like to come back to your offer sometime next year when I've
> completed the other one.

Sure. I expect this project to wait quite patiently for you to come back
next year ;-)

Ciao,
Dscho

> Especially since I'd have to polish up my buried C skills... C didn't
> get GC lately, did it? ;)
>

Re: Aborting git rebase --edit-todo

[Junio C Hamano]

Victor Toni <victor.toni@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>>
>> Victor Toni <victor.toni@gmail.com> writes:
>>
>> > Personally I would like to see your approach (1,2,3) implemented
>> > because it is not destructive. If the user wants to achieve something
>> > different he can retry.
>>
>> Obviously I agree that the approach would be nicer than the status
>> quo.  It would not be as trivial as a microproject, but would be a
>> good bite-sized starter-task for those aspiring developers who want
>> to dip their toes in the water to start hacking on the codebase ;-)
>>
> Nice try ;)

For the record I didn't try _you_.

I was writing for general audience, among which there are aspiring
developers seeking a starter-task.  Whether you are part of that
audience was immaterial (even though it would have been nice if you
were) ;-).

Thanks.

Re: Aborting git rebase --edit-todo

[Johannes Schindelin]

Hi Victor,

On Thu, 3 Sep 2020, Victor Toni wrote:

> > It is rather unusual (or almost always wrong) to have a totally
> > empty commit log or initial todo list, so it is understandable for
> > Git in these situations to stop without doing anything further.
> >
> > There is no other sensible interpretations of what you are telling
> > Git to you by returning an empty buffer---it is extremely unlikely
> > you want to create a commit with no log message (without explicitly
> > allowing it with --allow-empty-message, the command is likely to
> > fail anyway), and it is extremely unlikely that you wanted to just
> > reset the tip of the branch to the --onto commit.
> >
> > Once an interactive rebase session has started and you are given the
> > remainder of the steps to edit and you give an empty buffer back,
> > however, there are two possible interpretations that are equally
> > sensible, I would think.
> >
> >  - One is that you are signaling that you are done with the rebase
> >    session and all the remaining commits are to be discarded.
> >
> >  - The other is that you botched editing the todo list, and you wish
> >    Git to give you another chance to edit it again.
> >
> > I think the implementor chose the first interpretation.  The "drop"
> > insn is a relatively recent invention, and back when it was missing
> > from the vocabulary, I do not think it was possible to say " discard
> > all the rest" without emptying the todo list, so that design is
> > understandable.
> >
> > Now we have the "drop" verb, the latter interpretation becomes
> > possible without making it impossible for the user to express the
> > former.  It might be a good idea to
> >
> >  (1) save away the original before allowing --edit-todo to edit,

We already do that:
https://github.com/git/git/blob/v2.28.0/rebase-interactive.c#L113-L115

> >
> >  (2) open the editor, and
> >
> >  (3) when getting an empty buffer back, go back to step (2) using
> >      the back-up made in step (1).

Yes, and we can claim that this is a bug fix to avoid having to respect a
deprecation phase.

> >
> > Either way, the todo list editor buffer can have additional comment
> > instructing what happens when the buffer is emptied.
> >
>
> Personally I would like to see your approach (1,2,3) implemented
> because it is not destructive. If the user wants to achieve something
> different he

or she, or they,

> can retry.
> Option / interpretation a)
>
> >  - One is that you are signaling that you are done with the rebase
> >    session and all the remaining commits are to be discarded.
>
> is more difficult to recover from. (I'm still thankful for `.git/logs/HEAD`)

Indeed, it is pretty tedious to recover from when you can originally made
edits to the todo list that you then accidentally discarded.

Ciao,
Dscho

Re: Aborting git rebase --edit-todo

[Johannes Schindelin]

Hi Junio & Victor,

On Thu, 3 Sep 2020, Junio C Hamano wrote:

> Victor Toni <victor.toni@gmail.com> writes:
>
> > When doing a commit or choosing what to do for an interactive rebase
> > one can just wipe the whole content of the editor, save and close to
> > abort the action.
> > While doing a `git rebase --edit-todo` I came to the conclusion that I
> > would like to abort the edit and did the same. The final `git rebase
> > --continue` got me rid of the rest of the commits...
> > (Fortunately the "missing" commits could be rescued by looking into
> > `.git/logs/HEAD` so thumbs up for that. )
> > Unfortunately the behaviour of `--edit-todo` was a bit surprising and
> > somehow doesn't feel consistent with the other actions involving an
> > editor.
> >
> > Can this be considered a bug?
>
> It is rather unusual (or almost always wrong) to have a totally
> empty commit log or initial todo list, so it is understandable for
> Git in these situations to stop without doing anything further.
>
> There is no other sensible interpretations of what you are telling
> Git to you by returning an empty buffer---it is extremely unlikely
> you want to create a commit with no log message (without explicitly
> allowing it with --allow-empty-message, the command is likely to
> fail anyway), and it is extremely unlikely that you wanted to just
> reset the tip of the branch to the --onto commit.
>
> Once an interactive rebase session has started and you are given the
> remainder of the steps to edit and you give an empty buffer back,
> however, there are two possible interpretations that are equally
> sensible, I would think.
>
>  - One is that you are signaling that you are done with the rebase
>    session and all the remaining commits are to be discarded.
>
>  - The other is that you botched editing the todo list, and you wish
>    Git to give you another chance to edit it again.
>
> I think the implementor chose the first interpretation.  The "drop"
> insn is a relatively recent invention, and back when it was missing
> from the vocabulary, I do not think it was possible to say " discard
> all the rest" without emptying the todo list, so that design is
> understandable.
>
> Now we have the "drop" verb, the latter interpretation becomes
> possible without making it impossible for the user to express the
> former.  It might be a good idea to
>
>  (1) save away the original before allowing --edit-todo to edit,
>
>  (2) open the editor, and
>
>  (3) when getting an empty buffer back, go back to step (2) using
>      the back-up made in step (1).
>
> Either way, the todo list editor buffer can have additional comment
> instructing what happens when the buffer is emptied.
>
> I have no strong opinion on this one myself.  Deferring to Dscho,
> who may have a lot more to say on the design issue around this
> feature than I do.

First of all, some historical background: the idea that deleting
everything in the todo list aborts the rebase *predates* `git rebase
--edit-todo` by quite a bit, in fact, that idea was implemented in either
the very first version of `git rebase -i` or at least very, very short
thereafter.

This idea came from the fact that deleting the commit message would abort
a `git commit`.

In the meantime, `--edit-todo` is a thing (where this behavior makes a lot
less sense), and `drop` is also a thing.

I agree that it may be a good time to deprecate that behavior, after
introducing a new verb `abort` or something like that.

Ciao,
Dscho