Hi Elijah
On 09/09/2021 16:01, Elijah Newren wrote:
[...]
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
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:
