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

Hi Philippe,

On Wed, 8 Sep 2021, Philippe Blain wrote:

Hi Phillip,

Le 2021-09-08 à 09:41, Phillip Wood via GitGitGadget a écrit :

quoted

From: Phillip Wood <redacted>

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