Re: [PATCH 4/5] rebase -i: don't fork git checkout

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 9, 2021, 10:53 UTC
Message-ID: <nycvar.QRO.7.76.6.2109091250540.59@tvgsbejvaqbjf.bet>
In-Reply-To: <f05dc55f-a7e4-b8f7-7b0c-5000bf48f803@gmail.com>

Hi Philippe,

On Wed, 8 Sep 2021, Philippe Blain wrote:

Show 28 quoted lines

> Hi Phillip,
>
> Le 2021-09-08 à 09:41, Phillip Wood via GitGitGadget a écrit :
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > The "apply" based rebase has avoided forking git checkout since
> > ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"",
> > 2018-08-07). The code that handles the checkout was moved into libgit
> > by b309a97108 ("reset: extract reset_head() from rebase", 2020-04-07)
> > so lets start using it for the "merge" based rebase as well. This
> > opens the way for us to stop calling the post-checkout hook in the
> > future.
> >
>
> While in general I think it's a good thing to avoid forking, this change
> might result in behavioral differences. Any config that affects
> 'git checkout' but not the internal 'reset.c::reset_head' function might
> play a role in the rebase UX.
>
> One that immediately came to mind is 'submodule.recurse'. This initial
> 'onto' checkout was pretty much the only part of 'git rebase' that did
> something useful for submodules, so it's kind of sad to see it regress.
> [That is, until someone takes the time to implement 'git rebase
> --recurse-submodules' and makes sure *all* code paths that touch the
> working tree pay attention to this flag, and that will probably
> necessitate 'git merge --recurse-submodules' first because of the
> 'merge' backend... as far as I'm aware it's on Emily's list [1], it's
> also on mine but I don't know when I'll get the time.]

Good point, there is more in the `git_checkout_config()` function (which is `static` in `builtin/checkout.c`, and could easily be moved to `checkout.[ch]`). We should probably fall back to calling that function rather than `git_default_config()` in `rebase_config()`.

> Anyway, I'm not saying that we should not do what this patch is
> proposing, but I think caveats such as that should be documented in the
> commit message, and maybe an audit of other configs that might results
> in behavioural differences should be done.

Since this is already a bug in the `apply` backend, it would be even better to follow-up with a fix, hint, hint, nudge, nudge ;-)

Ciao, Dscho