[1/4] branch: support more tracking modes when recursing

Commit Message

Glen Choo March 29, 2022, 8:01 p.m. UTC

From: Glen Choo <[e-mail]>

"git branch --recurse-submodules" does not propagate "--track=inherit"
or "--no-track" to submodules, which causes submodule branches to use
the wrong tracking mode [1]. To fix this, pass the correct options to
the "submodule--helper create-branch" child process and test for it.

While we are refactoring the same code, replace "--track" with the
synonymous, but more consistent-looking "--track=direct" option
(introduced at the same time as "--track=inherit", d3115660b4 (branch:
add flags and config to inherit tracking, 2021-12-20)).

[1] This bug is partially a timing issue: "branch --recurse-submodules"
 was introduced around the same time as "--track=inherit", and even
 though I rebased "branch --recurse-submodules" on top of that, I had
 neglected to support the new tracking mode. Omitting "--no-track"
 was just a plain old mistake, though.

Signed-off-by: Glen Choo <[e-mail]>
---
 branch.c                    | 29 +++++++++++++++++++++++++---
 builtin/submodule--helper.c |  7 ++++---
 t/t3207-branch-submodule.sh | 38 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 67 insertions(+), 7 deletions(-)

Comments

Junio C Hamano March 30, 2022, 9:13 p.m. UTC | #1

"Glen Choo via GitGitGadget" <[e-mail]> writes:

> diff --git a/branch.c b/branch.c
> index 6b31df539a5..7377b9f451a 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -233,6 +233,9 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
>  	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
>  
> +	if (!track)
> +		BUG("asked to set up tracking, but tracking is disallowed");

I am wondering if this wants to be

	if (track == BRANCH_TRACK_NEVER)

instead.  Do we elsewhere rely on the fact that NEVER is assigned 0?

> @@ -534,8 +537,27 @@ static int submodule_create_branch(struct repository *r,
>  		strvec_push(&child.args, "--quiet");
>  	if (reflog)
>  		strvec_push(&child.args, "--create-reflog");
> -	if (track == BRANCH_TRACK_ALWAYS || track == BRANCH_TRACK_EXPLICIT)
> -		strvec_push(&child.args, "--track");
> +
> +	switch (track) {
> +	case BRANCH_TRACK_NEVER:
> +		strvec_push(&child.args, "--no-track");
> +		break;
> +	case BRANCH_TRACK_ALWAYS:
> +	case BRANCH_TRACK_EXPLICIT:
> +		strvec_push(&child.args, "--track=direct");
> +		break;
> +	case BRANCH_TRACK_OVERRIDE:
> +		BUG("BRANCH_TRACK_OVERRIDE cannot be used when creating a branch.");
> +		break;
> +	case BRANCH_TRACK_INHERIT:
> +		strvec_push(&child.args, "--track=inherit");
> +		break;

OK.

> +	case BRANCH_TRACK_UNSPECIFIED:
> +		/* Default for "git checkout". No need to pass --track. */
> +	case BRANCH_TRACK_REMOTE:
> +		/* Default for "git branch". No need to pass --track. */
> +		break;

Is that "no need to pass", or "no need to, and it will be detrimental to, pass"?

IOW, if we are relying on the command spawned via start_command()
interface to read and honor the configured default for themselves,
then passing explicit --track=whatever from this caller will be not
just necessary but is wrong, right?  I am worried about "No need to"
tempting "helpful" developers into doing unnecessary things, just to
be more explicit, for example. 

> @@ -614,7 +636,8 @@ void create_branches_recursively(struct repository *r, const char *name,
>  	 * tedious to determine whether or not tracking was set up in the
>  	 * superproject.
>  	 */
> -	setup_tracking(name, tracking_name, track, quiet);
> +	if (track)
> +		setup_tracking(name, tracking_name, track, quiet);

Here we do rely on the fact that NEVER has the value of 0.  If there
are other instances of code elsewhere that does so, then this one
and the other one at the top of this message are both fine.

Given that we started simple and then gradually added more features,
I would not be surprised if the older code written back when there
were only 0 (no track) and 1 (track) assumed 0 means no.  There is
one in create_branch() where we do

	if (real_ref && track)
		setup_tracking(ref.buf + 11, real_ref, track, quiet);

which also relies on the fact that NEVER is 0.

> -		OPT_SET_INT('t', "track", &track,
> -			    N_("set up tracking mode (see git-pull(1))"),
> -			    BRANCH_TRACK_EXPLICIT),
> +		OPT_CALLBACK_F('t', "track",  &track, "(direct|inherit)",
> +			N_("set branch tracking configuration"),
> +			PARSE_OPT_OPTARG,
> +			parse_opt_tracking_mode),

Hmph, this is quite curious.  How did the whole thing even worked
without this?

Ah, OK, this is in submodule--helper.c and tracking specification in
the top-level were OK.  Just that we forgot to correctly pass it
down when calling down to submodules.  Makes sense.

Thanks.

Glen Choo March 30, 2022, 11:29 p.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

> "Glen Choo via GitGitGadget" <[e-mail]> writes:
>
>> diff --git a/branch.c b/branch.c
>> index 6b31df539a5..7377b9f451a 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -233,6 +233,9 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>>  	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
>>  	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
>>  
>> +	if (!track)
>> +		BUG("asked to set up tracking, but tracking is disallowed");
>
> I am wondering if this wants to be
>
> 	if (track == BRANCH_TRACK_NEVER)
>
> instead.  Do we elsewhere rely on the fact that NEVER is assigned 0?

As you discussed here...

> Given that we started simple and then gradually added more features,
> I would not be surprised if the older code written back when there
> were only 0 (no track) and 1 (track) assumed 0 means no.  There is
> one in create_branch() where we do
>
> 	if (real_ref && track)
> 		setup_tracking(ref.buf + 11, real_ref, track, quiet);
>
> which also relies on the fact that NEVER is 0.

We know the answer is "yes there is older code that relies on NEVER
being 0". I believe this is the only instance though, which means this
patch comprises the majority of instances of "if (!track)", so we can
change it if you prefer. The older code is pretty old after all - the
enum was introduced in 9ed36cfa35 (branch: optionally setup
branch.*.merge from upstream local branches, 2008-02-19).

>> +	case BRANCH_TRACK_UNSPECIFIED:
>> +		/* Default for "git checkout". No need to pass --track. */
>> +	case BRANCH_TRACK_REMOTE:
>> +		/* Default for "git branch". No need to pass --track. */
>> +		break;
>
> Is that "no need to pass", or "no need to, and it will be detrimental to, pass"?
>
> IOW, if we are relying on the command spawned via start_command()
> interface to read and honor the configured default for themselves,
> then passing explicit --track=whatever from this caller will be not
> just necessary but is wrong, right?  I am worried about "No need to"
> tempting "helpful" developers into doing unnecessary things, just to
> be more explicit, for example. 

Hm, interesting, I hadn't considered that temptation. This is the
latter, i.e. it is not correct to pass --track. I'll reword it for
clarity, something like "Should not pass --track".

>> -		OPT_SET_INT('t', "track", &track,
>> -			    N_("set up tracking mode (see git-pull(1))"),
>> -			    BRANCH_TRACK_EXPLICIT),
>> +		OPT_CALLBACK_F('t', "track",  &track, "(direct|inherit)",
>> +			N_("set branch tracking configuration"),
>> +			PARSE_OPT_OPTARG,
>> +			parse_opt_tracking_mode),
>
> Hmph, this is quite curious.  How did the whole thing even worked
> without this?
>
> Ah, OK, this is in submodule--helper.c and tracking specification in
> the top-level were OK.  Just that we forgot to correctly pass it
> down when calling down to submodules.  Makes sense.

Yes, that's correct. This was missed because I only added tests for
--track and the default case (and didn't add tests for --track=inherit
or --no-track.

Patch

diff --git a/branch.c b/branch.c
index 6b31df539a5..7377b9f451a 100644
--- a/branch.c
+++ b/branch.c
@@ -233,6 +233,9 @@  static void setup_tracking(const char *new_ref, const char *orig_ref,
 	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
 	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
 
+	if (!track)
+		BUG("asked to set up tracking, but tracking is disallowed");
+
 	memset(&tracking, 0, sizeof(tracking));
 	tracking.spec.dst = (char *)orig_ref;
 	tracking.srcs = &tracking_srcs;
@@ -534,8 +537,27 @@  static int submodule_create_branch(struct repository *r,
 		strvec_push(&child.args, "--quiet");
 	if (reflog)
 		strvec_push(&child.args, "--create-reflog");
-	if (track == BRANCH_TRACK_ALWAYS || track == BRANCH_TRACK_EXPLICIT)
-		strvec_push(&child.args, "--track");
+
+	switch (track) {
+	case BRANCH_TRACK_NEVER:
+		strvec_push(&child.args, "--no-track");
+		break;
+	case BRANCH_TRACK_ALWAYS:
+	case BRANCH_TRACK_EXPLICIT:
+		strvec_push(&child.args, "--track=direct");
+		break;
+	case BRANCH_TRACK_OVERRIDE:
+		BUG("BRANCH_TRACK_OVERRIDE cannot be used when creating a branch.");
+		break;
+	case BRANCH_TRACK_INHERIT:
+		strvec_push(&child.args, "--track=inherit");
+		break;
+	case BRANCH_TRACK_UNSPECIFIED:
+		/* Default for "git checkout". No need to pass --track. */
+	case BRANCH_TRACK_REMOTE:
+		/* Default for "git branch". No need to pass --track. */
+		break;
+	}
 
 	strvec_pushl(&child.args, name, start_oid, tracking_name, NULL);
 
@@ -614,7 +636,8 @@  void create_branches_recursively(struct repository *r, const char *name,
 	 * tedious to determine whether or not tracking was set up in the
 	 * superproject.
 	 */
-	setup_tracking(name, tracking_name, track, quiet);
+	if (track)
+		setup_tracking(name, tracking_name, track, quiet);
 
 	for (i = 0; i < submodule_entry_list.entry_nr; i++) {
 		if (submodule_create_branch(
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5301612d24b..081c8267c33 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -3021,9 +3021,10 @@  static int module_create_branch(int argc, const char **argv, const char *prefix)
 		OPT__FORCE(&force, N_("force creation"), 0),
 		OPT_BOOL(0, "create-reflog", &reflog,
 			 N_("create the branch's reflog")),
-		OPT_SET_INT('t', "track", &track,
-			    N_("set up tracking mode (see git-pull(1))"),
-			    BRANCH_TRACK_EXPLICIT),
+		OPT_CALLBACK_F('t', "track",  &track, "(direct|inherit)",
+			N_("set branch tracking configuration"),
+			PARSE_OPT_OPTARG,
+			parse_opt_tracking_mode),
 		OPT__DRY_RUN(&dry_run,
 			     N_("show whether the branch would be created")),
 		OPT_END()
diff --git a/t/t3207-branch-submodule.sh b/t/t3207-branch-submodule.sh
index 0d93f7516c9..cfde6b237f5 100755
--- a/t/t3207-branch-submodule.sh
+++ b/t/t3207-branch-submodule.sh
@@ -260,7 +260,7 @@  test_expect_success 'should get fatal error upon branch creation when submodule
 	)
 '
 
-test_expect_success 'should set up tracking of remote-tracking branches' '
+test_expect_success 'should set up tracking of remote-tracking branches by default' '
 	test_when_finished "reset_remote_test" &&
 	(
 		cd super-clone &&
@@ -289,4 +289,40 @@  test_expect_success 'should not fail when unable to set up tracking in submodule
 	)
 '
 
+test_expect_success '--track=inherit should set up tracking correctly' '
+	test_when_finished "reset_remote_test" &&
+	(
+		cd super-clone &&
+		git branch --recurse-submodules branch-a origin/branch-a &&
+		# Set this manually instead of using branch --set-upstream-to
+		# to circumvent the "nonexistent upstream" check.
+		git -C sub config branch.branch-a.remote origin &&
+		git -C sub config branch.branch-a.merge refs/heads/sub-branch-a &&
+		git -C sub/sub-sub config branch.branch-a.remote other &&
+		git -C sub/sub-sub config branch.branch-a.merge refs/heads/sub-sub-branch-a &&
+
+		git branch --recurse-submodules --track=inherit branch-b branch-a &&
+		test_cmp_config origin branch.branch-b.remote &&
+		test_cmp_config refs/heads/branch-a branch.branch-b.merge &&
+		test_cmp_config -C sub origin branch.branch-b.remote &&
+		test_cmp_config -C sub refs/heads/sub-branch-a branch.branch-b.merge &&
+		test_cmp_config -C sub/sub-sub other branch.branch-b.remote &&
+		test_cmp_config -C sub/sub-sub refs/heads/sub-sub-branch-a branch.branch-b.merge
+	)
+'
+
+test_expect_success '--no-track should not set up tracking' '
+	test_when_finished "reset_remote_test" &&
+	(
+		cd super-clone &&
+		git branch --recurse-submodules --no-track branch-a origin/branch-a &&
+		test_cmp_config "" --default "" branch.branch-a.remote &&
+		test_cmp_config "" --default "" branch.branch-a.merge &&
+		test_cmp_config -C sub "" --default "" branch.branch-a.remote &&
+		test_cmp_config -C sub "" --default "" branch.branch-a.merge &&
+		test_cmp_config -C sub/sub-sub "" --default "" branch.branch-a.remote &&
+		test_cmp_config -C sub/sub-sub "" --default "" branch.branch-a.merge
+	)
+'
+
 test_done