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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 15, 2021, 15:44 UTC
Message-ID: <1311e9e9-6d10-40f8-2073-077313a94301@gmail.com>
In-Reply-To: <CABPp-BEbY0BqkBP4r-6XpGk46J+Y+W8+7cVZXQg5fuJXYOntDQ@mail.gmail.com>

Hi Elijah

On 09/09/2021 16:01, Elijah Newren wrote:

Show 28 quoted lines

> [...]
>> 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).
> 
> Yep, merge-ort was designed to just piggy back on checkout code.  The
> checkout() function was basically just code lifted from
> builtin/checkout.c.  Using that code means that merges now also
> benefit from all the special working tree handling that is encoded
> into git-checkout -- whether that's parallel checkout, submodule
> handling, tricky D/F switches or symlink handling, etc.  In contrast
> to merge-recursive, it does not need hundreds and hundreds of lines of
> special worktree updating code sprayed all over the codebase.
> 
> Conflicts are not special in this regard; merge-ort creates a tree
> which has files that include conflict markers, and then merge-ort
> calls checkout() to switch the working copy over to that tree.
> 
> The only issue conflicts present for merge-ort, is that AFTER it has
> checked out that special tree with conflict markers, it then has to go
> and touch up the index afterwards to replace the entries for
> conflicted files with multiple higher order stages.  (You could say
> that merge-recursive is "index-first", since its design focuses on the
> index -- updating it first and then figuring out everything else like
> updating the working tree with special code afterwards.  In contrast,
> merge-ort ignores the index entirely until the very end -- after a new
> merge tree is created and after the working tree is updated.)

Thanks for explaining, it's a nice design feature that you can just reuse the checkout code to update the working copy with the merge result

Show 25 quoted lines

>> 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).
> 
> I'd just parse any needed config in all cases.  The submodule settings
> aren't going to hurt merge-recursive; it'll just ignore them.  (Or are
> you worried about a mix-and-match of rebase calling both checkout and
> merge code doing weird things, and you'd rather not have the checkout
> bits update submodules if the merges won't?)

I'd rather just parse the config when we know submodules are going to be rebased, I think it's confusing if some bit work and others don't. I've tried the diff below locally, but t7402-submodule-rebase.sh does not show any change (I was hoping some text_expect_failure would be fixed) so I'm not sure if it's working or not and I ran out of time.

Best Wishes

Phillip

--- >8 ---

diff --git a/builtin/rebase.c b/builtin/rebase.c
index eed70168df..a35a9e3460 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"
  
@@ -1114,6 +1115,10 @@ static int rebase_config(const char *var, const char *value, void *data)
                 return git_config_string(&opts->default_backend, var, value);
         }
  
+       if (starts_with(var, "submodule.")) {
+               return git_default_submodule_config(var, value, NULL);
+       }
+
         return git_default_config(var, value, data);
  }
  
@@ -1820,6 +1825,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
             getenv("GIT_TEST_MERGE_ALGORITHM"))
                 options.strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM"));
  
+       /* only the "ort" merge strategy handles submodules correctly */
+       if (!is_merge(&options) ||
+           (options.strategy && strcmp(options.strategy, "ort")))
+               git_default_submodule_config("submodule.recurse", "false",
+                                            NULL);
+
         switch (options.type) {
         case REBASE_MERGE:
         case REBASE_PRESERVE_MERGES: