Re: [PATCH v3 0/5] implement branch --recurse-submodules

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@google.com (local)

Jonathan Tan [off-list ref] 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@google.com/ (local)

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@google.com/ (local)

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()).