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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Philippe

On 09/09/2021 13:40, Philippe Blain wrote:
>>> 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.

it would also affect fast-forwards

>> 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.
> 
> Yeah, it's not that useful, I have to admit; it can also be very confusing
> since some parts of rebase are affected, and some not. For example, any 
> time
> the rebase stops, like for 'edit', 'break', and when there are 
> conflicts, the
> submodules are not updated. So I think a full solution is better than a 
> partial
> solution; in the meantime I'm thinking the change you are proposing 
> would actually
> be less confusing, even if it slightly changes behaviour...
> 
> As an aside, I *think* reading submodule.recurse in rebase like it's 
> done in checkout
> et al., i.e. something like this:
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 33e0961900..125ec907e4 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -26,6 +26,7 @@
>   #include "rerere.h"
>   #include "branch.h"
>   #include "sequencer.h"
> +#include "submodule.h"
>   #include "rebase-interactive.h"
>   #include "reset.h"
> 
> @@ -1106,6 +1107,9 @@ static int rebase_config(const char *var, const 
> char *value, void *data)
>           return git_config_string(&opts->default_backend, var, value);
>       }
> 
> +    if (!strcmp(var, "submodule.recurse"))
> +        return git_default_submodule_config(var, value, data);

That looks about right to me though I think it would be safer to call 
git_default_submodule_config() for submodule.* rather than just 
submodule.recurse

>       return git_default_config(var, value, data);
>   }
> 
> 
> would actually also affect the merges
> performed during the rebase, since that would affect the "global" state 
> in submodule.c.
> I hacked exactly that the other day but did not test extensively...

merge-ort.c:checkout() which is used by merge_switch_to_result() uses 
unpack_trees() so it will pick up the global state and hopefully should 
just work (cc'ing Elijah to check as I didn't look what happens when 
there are conflicts). merge-recursive.c:update_file_flags() does this 
when updating the work tree

        if (S_ISGITLINK(contents->mode)) { 

                 /* 

                  * We may later decide to recursively descend into 

                  * the submodule directory and update its index 

                  * and/or work tree, but we do not do that now. 

                  */ 

                 update_wd = 0; 

                 goto update_index; 

        } 

 

so it looks like it does not update the submodule directory. Given 
merge-ort is now the default perhaps it's time for rebase (and 
cherry-pick/revert) to start reading the submodule config settings (we 
parse the config before we know if we'll be using merge-ort so I don't 
know how easy it would be to only parse the submodule settings if we are 
using merge-ort).

Best Wishes

Phillip