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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 9, 2021, 10:09 UTC
Message-ID: <c42d4051-59cd-094a-4570-32cf4d38ec27@gmail.com>
In-Reply-To: <f05dc55f-a7e4-b8f7-7b0c-5000bf48f803@gmail.com>

Hi Philippe

On 08/09/2021 19:14, Philippe Blain wrote:

Show 24 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.

Thanks for pointing that out. As a non-submodule user my question would be is it actually useful for the initial checkout to work that way if the rest of rebase (and the checkout for the am backend) ignores submodules? reset.c::reset_head() just uses unpack trees like checkout so if rebase read 'submodule.recurse' then reset_head() would work like 'git checkout' and also 'git rebase --abort' and the "reset" command in the todo list would start checking out submodules. I'm reluctant to do that until the merge backend also handles submodules unless there is a good reason that such partial submodule support would help submodule users.

[That is, until

Show 14 quoted lines

> 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.]
> 
> 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,

Yes I'll update the commit message. It should also mention that this fixes the bug reported in [1] where a failing post-checkout hook aborts a rebase.

Best Wishes

Phillip

[1] https://lore.kernel.org/git/CAHr-Uu_KeAJZrd+GzymNP47iFi+dZkvVYsWQPtzT_FQrVnWTDg@mail.gmail.com/

Show 11 quoted lines

> and maybe
> an audit of other configs that might results in behavioural differences 
> should be done.
> 
> Thanks,
> 
> Philippe.
> 
> [1] 
> https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com/t/#m0229af9183a84c2367f21e82adfbd21f08aa4437 
>