Re: [PATCH v6 5/5] branch: add --recurse-submodules option for branch creation

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jan 11, 2022, 03:28 UTC
Message-ID: <20220111032842.1247928-1-jonathantanmy@google.com>
In-Reply-To: <20211220233459.45739-6-chooglen@google.com>

I am not very familiar with branch tracking, so I'll review everything except that part.

Glen Choo <chooglen@google.com> writes:

> Although this commit does not introduce breaking changes, it is
> incompatible with existing --recurse-submodules semantics e.g. `git
> checkout` does not recursively checkout the expected branches created by
> `git branch` yet.

Probably worth explaining that it will not recursively checkout the expected branches *if* any of them are subsequently updated. Maybe say:

  If a user were to create branches in this way, create a commit on the
  branch in a submodule, and run "git checkout --recurse-submodules" in
  the superproject, the commits to be checked out (which are based on
  the gitlink in the superproject, not on the ref store of the
  submodule) probably wouldn't match the user's expectation.

> To ensure that the correct set of semantics is used,
> this commit introduces a new configuration value,
> `submodule.propagateBranches`, which enables submodule branching when
> true (defaults to false).

And then this could be reworded to:

  Because of this, this commit introduces a new configuration value
  `submodule.propagateBranches`. The plan is for Git commands to
  prioritize submodule ref store information over superproject gitlink
  if this is true. Because "git branch --recurse-submodules" writes to
  submodule ref stores, for the sake of clarity, it will not function
  unless this configuration value is set.

Show 13 quoted lines

> @@ -71,6 +68,23 @@ submodule.recurse::
>  	`git fetch` but does not have a `--no-recurse-submodules` option.
>  	For these commands a workaround is to temporarily change the
>  	configuration value by using `git -c submodule.recurse=0`.
> +	+
> +	The following list shows the commands that accept
> +	`--recurse-submodules` and whether they are supported by this
> +	setting.
> +	* `checkout`, `fetch`, `grep`, `pull`, `push`, `read-tree`,
> +	`reset`, `restore` and `switch` are always supported.
> +	* `clone` and `ls-files` are not supported.
> +	* `branch` is supported only if `submodule.propagateBranches` is
> +	enabled

One oddity is that paragraphs after the "+" cannot be indented - see other documentation files for examples.

Show 23 quoted lines

> diff --git a/branch.c b/branch.c
> index 55c7ba4a25..6d0d9a8e1b 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -8,6 +8,8 @@
>  #include "sequencer.h"
>  #include "commit.h"
>  #include "worktree.h"
> +#include "submodule-config.h"
> +#include "run-command.h"
>  
>  struct tracking {
>  	struct refspec_item spec;
> @@ -478,6 +480,134 @@ void dwim_and_setup_tracking(struct repository *r, const char *new_ref,
>  	setup_tracking(new_ref, real_orig_ref, track, quiet);
>  }
>  
> +/**
> + * Creates a branch in a submodule by calling
> + * create_branches_recursively() in a child process. The child process
> + * is necessary because install_branch_config() (and its variants) do
> + * not support writing configs to submodules.
> + */

Makes sense that we need a child process, but could the child process be "branch" instead of "submodule--helper"? If not, also mention why.

As for the reason, probably better to explicitly mention install_branch_config_multiple_remotes() and say "(which is called by setup_tracking())".

Show 55 quoted lines

> diff --git a/t/t3207-branch-submodule.sh b/t/t3207-branch-submodule.sh
> new file mode 100755
> index 0000000000..a2dfb5ad7f
> --- /dev/null
> +++ b/t/t3207-branch-submodule.sh
> @@ -0,0 +1,291 @@
> +#!/bin/sh
> +
> +test_description='git branch submodule tests'
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-rebase.sh
> +
> +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 &&
> +	# Submodule in a submodule
> +	git -C sub-upstream submodule add "$TRASH_DIRECTORY/sub-sub-upstream" sub-sub &&
> +	git -C sub-upstream commit -m "add submodule" &&
> +	# Regular submodule
> +	git -C super submodule add "$TRASH_DIRECTORY/sub-upstream" sub &&
> +	# Submodule in a subdirectory
> +	git -C super submodule add "$TRASH_DIRECTORY/sub-sub-upstream" second/sub &&
> +	git -C super commit -m "add submodule" &&
> +	git -C super config submodule.propagateBranches true &&
> +	git -C super/sub submodule update --init
> +'
> +
> +CLEANUP_SCRIPT_PATH="$TRASH_DIRECTORY/cleanup_branches.sh"
> +
> +cat >"$CLEANUP_SCRIPT_PATH" <<'EOF'
> +	#!/bin/sh
> +
> +	super_dir="$1"
> +	shift
> +	(
> +		cd "$super_dir" &&
> +		git checkout main &&
> +		for branch_name in "$@"; do
> +			git branch -D "$branch_name"
> +			git submodule foreach "$TRASH_DIRECTORY/cleanup_branches.sh . $branch_name || true"
> +		done
> +	)
> +EOF
> +chmod +x "$CLEANUP_SCRIPT_PATH"
> +
> +cleanup_branches() {
> +	TRASH_DIRECTORY="\"$TRASH_DIRECTORY\"" "$CLEANUP_SCRIPT_PATH" "$@"
> +} >/dev/null 2>/dev/null

I don't think that the cleanup is saving much in process invocation anymore - could we just delete the whole thing and start anew in each test?

The rest of the tests are assuming that the cleanup works as expected - I didn't take a close look.

Also, you should probably use "$(pwd)" instead of $TRASH_DIRECTORY.

Show 11 quoted lines

> +test_expect_success '--recurse-submodules should fail when not creating branches' '
> +	test_when_finished "cleanup_branches super branch-a" &&
> +	(
> +		cd super &&
> +		git branch --recurse-submodules branch-a &&
> +		test_must_fail git branch --recurse-submodules -D branch-a &&
> +		# Assert that the branches were not deleted
> +		git rev-parse --abbrev-ref branch-a &&
> +		git -C sub rev-parse --abbrev-ref branch-a
> +	)
> +'

If we're just checking that the ref exists, no need for "--abbrev-ref". Same comment throughout the file.

Show 15 quoted lines

> +test_expect_success 'should not create any branches if branch is not valid for all repos' '
> +	test_when_finished "cleanup_branches super branch-a" &&
> +	(
> +		cd super &&
> +		git -C sub branch branch-a &&
> +		test_must_fail git branch --recurse-submodules branch-a 2>actual &&
> +		test_must_fail git rev-parse branch-a &&
> +
> +		cat >expected <<-EOF &&
> +		submodule ${SQ}sub${SQ}: fatal: A branch named ${SQ}branch-a${SQ} already exists.
> +		fatal: submodule ${SQ}sub${SQ}: cannot create branch ${SQ}branch-a${SQ}
> +		EOF
> +		test_cmp expected actual
> +	)
> +'

The error message seems too specific - probably enough to grep for the information about the branch already existing.

Show 14 quoted lines

> +test_expect_success 'should create branches if branch exists and --force is given' '
> +	test_when_finished "cleanup_branches super branch-a" &&
> +	(
> +		cd super &&
> +		git -C sub rev-parse HEAD >expected &&
> +		test_commit -C sub baz &&
> +		git -C sub branch branch-a HEAD~1 &&
> +		git branch --recurse-submodules --force branch-a &&
> +		git rev-parse branch-a &&
> +		# assert that sub:branch-a was moved
> +		git -C sub rev-parse branch-a >actual &&
> +		test_cmp expected actual
> +	)
> +'

Should we create branch-a at HEAD instead of HEAD~1?

Show 20 quoted lines

> +test_expect_success 'should create branch when submodule is not in HEAD:.gitmodules' '
> +	test_when_finished "cleanup_branches super branch-a branch-b branch-c" &&
> +	(
> +		cd super &&
> +		git branch branch-a &&
> +		git checkout -b branch-b &&
> +		git submodule add ../sub-upstream sub2 &&
> +		git -C sub2 submodule update --init &&
> +		# branch-b now has a committed submodule not in branch-a
> +		git commit -m "add second submodule" &&
> +		git checkout branch-a &&
> +		git branch --recurse-submodules branch-c branch-b &&
> +		git rev-parse branch-c &&
> +		git -C sub rev-parse branch-c &&
> +		git -C second/sub rev-parse branch-c &&
> +		git checkout --recurse-submodules branch-c &&
> +		git -C sub2 rev-parse branch-c &&
> +		git -C sub2/sub-sub rev-parse branch-c
> +	)
> +'

No need to check so many repos, I think - just sub2 will do.

[skip tracking tests]

Show 10 quoted lines

> +test_expect_success 'should not create branches in inactive submodules' '
> +	test_when_finished "cleanup_branches super branch-a" &&
> +	test_config -C super submodule.sub.active false &&
> +	(
> +		cd super &&
> +		git branch --recurse-submodules branch-a &&
> +		git rev-parse branch-a &&
> +		test_must_fail git -C sub branch-a
> +	)
> +'

Makes sense, but could all the tracking tests be together in the file? Or is this order for a reason?

[skip tracking tests]

> +test_expect_success 'setup remote-tracking tests' '

This setup is not just for remote-tracking tests.

Show 13 quoted lines

> +	(
> +		cd super &&
> +		git branch branch-a &&
> +		git checkout -b branch-b &&
> +		git submodule add ../sub-upstream sub2 &&
> +		# branch-b now has a committed submodule not in branch-a
> +		git commit -m "add second submodule"
> +	) &&
> +	git clone --branch main --recurse-submodules super super-clone &&
> +	git -C super-clone config submodule.propagateBranches true
> +'
> +
> +test_expect_success 'should not create branch when submodule is not in .git/modules' '

I understand that no branch is created, but the title is ambiguous, to me, whether it is a fatal error or not. Maybe the title should be "fatal error upon branch creation when submodule is not in .git/modules".

Show 7 quoted lines

> +	# The cleanup needs to delete sub2 separately because main does not have sub2
> +	test_when_finished "git -C super-clone/sub2 branch -D branch-b && \
> +		git -C super-clone/sub2/sub-sub branch -D branch-b && \
> +		cleanup_branches super-clone branch-a branch-b" &&
> +	(
> +		cd super-clone &&
> +		# This should succeed because super-clone has sub.

"has sub in .git/modules", I think.

> +		git branch --recurse-submodules branch-a origin/branch-a &&
> +		# This should fail because super-clone does not have sub2.

Likewise.

Show 14 quoted lines

> +		test_must_fail git branch --recurse-submodules branch-b origin/branch-b 2>actual &&
> +		cat >expected <<-EOF &&
> +		hint: You may try updating the submodules using ${SQ}git checkout origin/branch-b && git submodule update --init${SQ}
> +		fatal: submodule ${SQ}sub2${SQ}: unable to find submodule
> +		EOF
> +		test_cmp expected actual &&
> +		test_must_fail git rev-parse branch-b &&
> +		test_must_fail git -C sub rev-parse branch-b &&
> +		# User can fix themselves by initializing the submodule
> +		git checkout origin/branch-b &&
> +		git submodule update --init --recursive &&
> +		git branch --recurse-submodules branch-b origin/branch-b
> +	)
> +'

Apart from the branch tracking part (which I didn't review), overall, this patch set looks good.