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

[Philippe Blain <levraiphilippeblain@gmail.com>]

Hi Phillip,

Le 2021-09-09 à 06:09, Phillip Wood a écrit :
> Hi Philippe
> 
> On 08/09/2021 19:14, Philippe Blain wrote:
>> 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.

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);
+
  	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...

Cheers,
Philippe.