Ah, I should make my cover letter more explicit then. I believe some of the comments are addressed in the body of the patches, but that's not the most reviewer-friendly way to do it. I'll address the comments out-of-order because it makes a bit more sense this way. All the patch snippets come from https://lore.kernel.org/git/20211209184928.71413-5-chooglen@xxxxxxxxxx Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes: > I think we need a test where the submodule is at > $ROOT/foo/bar, not just $ROOT/foo This scenario is tested by changing how the test repo is set up: +test_expect_success 'setup superproject and submodule' ' + git init super && + test_commit foo && + git init sub-sub-upstream && + test_commit -C sub-sub-upstream foo && + git init sub-upstream && + git -C sub-upstream submodule add "$TRASH_DIRECTORY/sub-sub-upstream" sub-sub && + git -C sub-upstream commit -m "add submodule" && + git -C super submodule add "$TRASH_DIRECTORY/sub-upstream" sub && + git -C super commit -m "add submodule" && + git -C super config submodule.propagateBranches true && + git -C super/sub submodule update --init +' so the tests are now all written against a superproject with a 2-level submodule. > I just saw that you [...] are still using tree_entry() non-recursively > [2] > [2] Discussed in https://lore.kernel.org/git/20211123021223.3472472-1-jonathantanmy@xxxxxxxxxx/ Yes, but this limitation is overcome by having create_branches_recursively() call itself. I documented this caveat on submodules_of_tree(), which is the function that calls tree_entry(). +/** + * Given a treeish, return all submodules in the tree. This only reads + * one level of the tree, so it will not return nested submodules; + * callers that require nested submodules are expected to handle the + * recursion themselves. + */ +void submodules_of_tree(struct repository *r, + const struct object_id *treeish_name, + struct submodule_entry_list *ret); > [1] I said "If you want to use setup_tracking() through > submodule--helper, I think that needs an explanation as to why a Git > command wouldn't work." in > https://lore.kernel.org/git/20211129210140.937875-1-jonathantanmy@xxxxxxxxxx/ Now that create_branches_recursively() calls itself, I moved setup_tracking() into create_branches_recursively() instead of having a submodule helper that only calls create_branch() and setup_tracking(). As a result, I think this comment isn't as relevant as before. That said, it might still be unobvious that create_branch() does not do the right thing without setup_tracking(), i.e. # Why doesn't create_branch() just do the right thing? + create_branch(the_repository, name, start_name, force, 0, reflog, quiet, + BRANCH_TRACK_NEVER, dry_run); + setup_tracking(name, tracking_name, track, quiet, 0); I was hoping that readers who were unclear would find the comment on create_branches_recursively() adequate: + * - tracking_name is the name used of the ref that will be used to set + * up tracking, e.g. origin/main. This is propagated to submodules so + * that tracking information will appear as if the branch branched off + * tracking_name instead of start_name (which is a plain commit id for + * submodules). If omitted, start_name is used for tracking (just like + * create_branch()).
