From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: Eugen Konkov <kes-kes@yandex.ru>, Johannes Sixt <j6t@kdbg.org>,
Elijah Newren <newren@gmail.com>,
Git Mailing List <git@vger.kernel.org>,
Thomas Gummerer <t.gummerer@gmail.com>
Subject: Re: git rebase/git rebase --abort cause inconsistent state
Date: Tue, 10 Nov 2020 23:28:49 +0100 (CET) [thread overview]
Message-ID: <nycvar.QRO.7.76.6.2011102312020.18437@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <xmqqft5icsd9.fsf@gitster.c.googlers.com>
Hi Junio,
On Mon, 9 Nov 2020, Junio C Hamano wrote:
> Eugen Konkov <kes-kes@yandex.ru> writes:
>
> >> You start at branch dev. Then you use the two argument form
> >
> >> git rebase dev local/dev
> >
> >> and when you later
> >
> >> git rebase --abort
> >
> >> then you are not warped back to dev, but to local/dev:
> >
> > I suppose `git rebase --abort` should return me back to `dev`, because
> > this is the state I was before the command. hmm... suppose it will not
> > return to original branch when [branch] parameter is specified for git
> > rebase
>
> Yes, "git rebase [--onto C] A B" has always been a short-hand for
>
> git checkout B
> git rebase [--onto C] A
>
> which means that if the second rebase step aborts, rebase wants to
> go back to the state before the rebase started, i.e. immediately
> after "checkout B" was done.
>
> I think the root cause of the problem is that addition of the
> "--autostash" feature (which came much later than the two-arg form)
> was designed poorly. If it wanted to keep the "two-arg form is a
> mere short-hand for checkout followed by rebase" semantics to avoid
> confusing existing users (which is probably a good thing and that
> seems to be what the code does), then the auto-stash should have
> been added _after_ we switch to the branch we rebase, i.e. B. That
> way, the stash would be applicable if the rebase gets aborted and
> goes back to the original B, where the stash was taken from.
That makes a ton of sense to me.
> Of course, that would also mean that the original modification in
> the working tree and the index may not allow you to move to branch B
> (i.e. starting from your original branch O, and editing files in the
> working tree, "git checkout B" may notice that you edited files that
> are different between O and B and refuse to check out branch B to
> prevent you from losing your local modifications), but that probably
> is a good thing, if "two-arg form is a mere short-hand" paradigm is
> to be kept. So, "use autostash and you can always rebase in a clean
> state" would no longer hold.
I agree with that, too.
> Another thing we could have done when adding "--autostash", was to
> redefine the meaning of the two-arg form. Then it starts to make
> sense to take a stash _before_ switching to the branch to be rebased
> (i.e. B), to go back to the original branch before switching to B,
> and then to unstash on the working tree of the original branch that
> is checked out after aborting.
>
> Note that such an alternative design would have had its own issues.
> With such a different semantics of two-arg form, if a rebase cleanly
> finishes, instead of staying on the rebased branch B, we MUST go
> back to the original branch to unstash what was autostashed.
> Usually people expect after a rebase to play with the rebased state
> (e.g. test build), so staying on branch B that was just rebased
> would be far more usable than going back to unrelated original
> branch (and possibly unstashing).
>
> In any case, the ship has long sailed, so ...
Right. I think by now, the sanest way out of this fix is to do as you say,
stash only _after_ switching to the branch (if that was asked for).
Unfortunately, it is not that trivial to change `git rebase` to autostash
_after_ switching branches: we actually do skip the actual _checkout_
part, for performance reasons, as of 767a9c417eb (rebase -i: stop checking
out the tip of the branch to rebase, 2020-01-24).
My wishful thinking part wants Elijah's merge-ort work to be complete
already so that we can implement a purely in-memory, throw-away
cherry-pick of the autostashed changes on top of the branch to switch to,
and add that as a mandatory check _right after_ autostashing in `git
rebase`. I guess at some stage that will happen.
In the meantime, we might need to disallow `--autostash` with implicit
branch switching.
Ciao,
Dscho
next prev parent reply other threads:[~2020-11-10 22:28 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-06 18:32 git rebase/git rebase --abort cause inconsistent state Eugen Konkov
2020-11-06 18:34 ` Eugen Konkov
2020-11-06 20:27 ` Elijah Newren
2020-11-06 23:13 ` Johannes Sixt
2020-11-09 11:46 ` Eugen Konkov
2020-11-09 18:11 ` Junio C Hamano
2020-11-10 17:59 ` Eugen Konkov
2020-11-10 22:28 ` Johannes Schindelin [this message]
2020-11-11 7:10 ` Johannes Sixt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=nycvar.QRO.7.76.6.2011102312020.18437@tvgsbejvaqbjf.bet \
--to=johannes.schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=j6t@kdbg.org \
--cc=kes-kes@yandex.ru \
--cc=newren@gmail.com \
--cc=t.gummerer@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).