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

From: Glen Choo <chooglen@google.com>
Date: Dec 14, 2021, 18:47 UTC
Message-ID: <kl6lr1af81n7.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20211213232008.771414-1-jonathantanmy@google.com>

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

Jonathan Tan <jonathantanmy@google.com> 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/

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/

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