branch --recurse-submodules: Bug fixes and clean ups

This a catch-all series that address some bugs discovered while testing "git branch --recurse-submodules" (patches 1-2), as well as some clean ups that came up when "branch --recurse-submodules" was first proposed (patches 3-4).

== Patch organization

Patches 1-2 are bugfixes, 3-4 are clean ups.

* Patch 1 fixes a bug where "git branch --recurse-submodules" would not propagate the "--track" option if its value was "--no-track" or "--track=inherit".

* Patch 2 fixes a bug where "git branch --recurse-submodules" would give advice before telling the user what the problem is (instead of the other way around).

* Patch 3 fixes some old inconsistencies when "git branch

--set-upstream-to" gives advice and when it doesn't.

* Patch 4 replaces exit(-1) with exit(1).

== Outstanding concerns

Patch 4: exit(1) was suggested in
https://lore.kernel.org/git/[email protected]. I'm not sure if we have a strong convention around exit codes and I'm using "1" as "unspecified error. Perhaps others could chime in.

Glen Choo (4):
branch: support more tracking modes when recursing
branch: give submodule updating advice before exit branch --set-upstream-to: be consistent when advising
branch: remove negative exit code

 branch.c                    | 42 ++++++++++++++++++++++++++++++-------
 builtin/submodule--helper.c |  7 ++++---
 t/t3207-branch-submodule.sh | 38 ++++++++++++++++++++++++++++++++-
 3 files changed, 75 insertions(+), 12 deletions(-)

base-commit: abf474a5dd901f28013c52155411a48fd4c09922
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1190%2Fchooglen%2Fbranch%2Frecursive-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1190/chooglen/branch/recursive-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1190

gitgitgadget

From: Glen Choo <[email protected]>

"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.

---
 branch.c                    | 29 +++++++++++++++++++++++++---
 builtin/submodule--helper.c |  7 ++++---
 t/t3207-branch-submodule.sh | 38 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 67 insertions(+), 7 deletions(-)

index 6b31df539a5..7377b9f451a 100644
@@ -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(

index 5301612d24b..081c8267c33 100644
@@ -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()

index 0d93f7516c9..cfde6b237f5 100755
@@ -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

gitgitgadget

As you discussed here...

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

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".

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.

From: Glen Choo <[email protected]>

Fix a bug where "hint:" was printed _before_ "fatal:" (instead of the other way around).

---
 branch.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 7377b9f451a..133e6047bc6 100644
@@ -607,11 +607,13 @@ void create_branches_recursively(struct repository *r, const char *name,
 	 */
 	for (i = 0; i < submodule_entry_list.entry_nr; i++) {
 		if (submodule_entry_list.entries[i].repo == NULL) {
+			int code = die_message(
+				_("submodule '%s': unable to find submodule"),
+				submodule_entry_list.entries[i].submodule->name);
 			if (advice_enabled(ADVICE_SUBMODULES_NOT_UPDATED))
 				advise(_("You may try updating the submodules using 'git checkout %s && git submodule update --init'"),
 				       start_commitish);
-			die(_("submodule '%s': unable to find submodule"),
-			    submodule_entry_list.entries[i].submodule->name);
+			exit(code);
 		}
 
 		if (submodule_create_branch(

gitgitgadget

From: Glen Choo <[email protected]>

"git branch --set-upstream-to" behaves differently when advice is enabled/disabled:

|                 | error prefix | exit code |
|-----------------+--------------+-----------|
| advice enabled  | error:       |         1 |
| advice disabled | fatal:       |       128 |

Make both cases consistent by using die_message() when advice is enabled (this was first proposed in [1]).

[1] https://lore.kernel.org/git/[email protected]

---
 branch.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 133e6047bc6..4a8796489c7 100644
@@ -389,9 +389,10 @@ static void dwim_branch_start(struct repository *r, const char *start_name,
 	if (get_oid_mb(start_name, &oid)) {
 		if (explicit_tracking) {
 			if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {
-				error(_(upstream_missing), start_name);
+				int code = die_message(_(upstream_missing),
+						       start_name);
 				advise(_(upstream_advice));
-				exit(1);
+				exit(code);
 			}
 			die(_(upstream_missing), start_name);
 		}

gitgitgadget

Thanks for following up on this :)

	diff --git a/branch.c b/branch.c
	index eb231b950bb..5b648cb27ed 100644
	--- a/branch.c
	+++ b/branch.c
	@@ -342,8 +342,6 @@ static int validate_remote_tracking_branch(char *ref)
	 
	 static const char upstream_not_branch[] =
	 N_("cannot set up tracking information; starting point '%s' is not a branch");
	-static const char upstream_missing[] =
	-N_("the requested upstream branch '%s' does not exist");
	 static const char upstream_advice[] =
	 N_("\n"
	 "If you are planning on basing your work on an upstream\n"
	@@ -388,13 +386,11 @@ static void dwim_branch_start(struct repository *r, const char *start_name,
	 	real_ref = NULL;
	 	if (get_oid_mb(start_name, &oid)) {
	 		if (explicit_tracking) {
	-			if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {
	-				int code = die_message(_(upstream_missing),
	-						       start_name);
	-				advise(_(upstream_advice));
	-				exit(code);
	-			}
	-			die(_(upstream_missing), start_name);
	+			int code = die_message(_("the requested upstream branch '%s' does not exist"),
	+					       start_name);
	+			advise_if_enabled(ADVICE_SET_UPSTREAM_FAILURE,
	+					  _(upstream_advice));
	+			exit(code);
	 		}
	 		die(_("not a valid object name: '%s'"), start_name);
	 	}

:)

Oh, good point. Yeah I like this better, I'll do that.

I think it's worthwhile; this does exactly what we want. I would have used it if I had known it existed.

Interesting, I hadn't considered type checking. So in general we prefer to inline the strings and not use variables? I'll keep that in mind.

From: Glen Choo <[email protected]>

Replace an instance of "exit(-1)" with "exit(1)". We don't use negative exit codes - they are misleading because Unix machines will coerce them to 8-bit unsigned values, losing the sign.

---
 branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 4a8796489c7..eb231b950bb 100644
@@ -263,7 +263,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 		string_list_append(tracking.srcs, orig_ref);
 	if (install_branch_config_multiple_remotes(config_flags, new_ref,
 				tracking.remote, tracking.srcs) < 0)
-		exit(-1);
+		exit(1);
 
 cleanup:
 	string_list_clear(&tracking_srcs, 0);

gitgitgadget

Thanks for the feedback, all. This version incorporates most of the suggestions (which were pretty small anyway).

== Patch organization

Patches 1-2 are bugfixes, 3-4 are clean ups.

Patch 1 fixes a bug where "git branch --recurse-submodules" would not propagate the "--track" option if its value was "--no-track" or "--track=inherit".

Patch 2 fixes a bug where "git branch --recurse-submodules" would give advice before telling the user what the problem is (instead of the other way around).

Patch 3 fixes some old inconsistencies when "git branch --set-upstream-to" gives advice and when it doesn't.

Patch 4 replaces exit(-1) with exit(1).

== Changes

Since v1:

* Patch 1: reword the --track comments to be prescriptive
* Patch 3: remove a now-unnecessary die(). I didn't include a suggestion to inline the advice string to save reviewers the trouble of proofreading (and the format string has no placeholders anyway, so I don't think we'd get much benefit out of typechecking). We can inline it in another series.

Glen Choo (4):
branch: support more tracking modes when recursing
branch: give submodule updating advice before exit branch --set-upstream-to: be consistent when advising
branch: remove negative exit code

 branch.c                    | 47 +++++++++++++++++++++++++++----------
 builtin/submodule--helper.c |  7 +++---
 t/t3207-branch-submodule.sh | 38 +++++++++++++++++++++++++++++-
 3 files changed, 76 insertions(+), 16 deletions(-)

base-commit: abf474a5dd901f28013c52155411a48fd4c09922
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1190%2Fchooglen%2Fbranch%2Frecursive-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1190/chooglen/branch/recursive-fixes-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1190

Range-diff vs v1:

1: 0b682c173c8 ! 1: 5e96a09199f branch: support more tracking modes when recursing
@@ branch.c: static int submodule_create_branch(struct repository *r,
+ strvec_push(&child.args, "--track=inherit");
+ break;
+ case BRANCH_TRACK_UNSPECIFIED:
-+ /* Default for "git checkout". No need to pass --track. */
++ /* Default for "git checkout". Do not pass --track. */
+ case BRANCH_TRACK_REMOTE:
-+ /* Default for "git branch". No need to pass --track. */
++ /* Default for "git branch". Do not pass --track. */
+ break;
+ }

 2:  f21d283e5ad = 2:  74b839bfc4e branch: give submodule updating advice before exit
 3:  8e6ea3478b3 ! 3:  64d9b8e0f44 branch --set-upstream-to: be consistent when advising
     @@ Commit message
      
          [1] https://lore.kernel.org/git/[email protected]
      
     +    Helped-by: Ævar Arnfjörð Bjarmason <[email protected]>
          Signed-off-by: Glen Choo <[email protected]>
      
       ## branch.c ##
      @@ branch.c: static void dwim_branch_start(struct repository *r, const char *start_name,
     + 	real_ref = NULL;
       	if (get_oid_mb(start_name, &oid)) {
       		if (explicit_tracking) {
     - 			if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {
     +-			if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {
      -				error(_(upstream_missing), start_name);
     -+				int code = die_message(_(upstream_missing),
     -+						       start_name);
     - 				advise(_(upstream_advice));
     +-				advise(_(upstream_advice));
      -				exit(1);
     -+				exit(code);
     - 			}
     - 			die(_(upstream_missing), start_name);
     +-			}
     +-			die(_(upstream_missing), start_name);
     ++			int code = die_message(_(upstream_missing), start_name);
     ++			advise_if_enabled(ADVICE_SET_UPSTREAM_FAILURE,
     ++					  _(upstream_advice));
     ++			exit(code);
       		}
     + 		die(_("not a valid object name: '%s'"), start_name);
     + 	}

4: fb2b472d9ae = 4: 306858373cc branch: remove negative exit code

gitgitgadget

From: Glen Choo <[email protected]>

"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.

---
 branch.c                    | 29 +++++++++++++++++++++++++---
 builtin/submodule--helper.c |  7 ++++---
 t/t3207-branch-submodule.sh | 38 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 67 insertions(+), 7 deletions(-)

index 6b31df539a5..aac24591d22 100644
@@ -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". Do not pass --track. */
+	case BRANCH_TRACK_REMOTE:
+		/* Default for "git branch". Do not 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(

index 5301612d24b..081c8267c33 100644
@@ -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()

index 0d93f7516c9..cfde6b237f5 100755
@@ -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

gitgitgadget

From: Glen Choo <[email protected]>

Fix a bug where "hint:" was printed _before_ "fatal:" (instead of the other way around).

---
 branch.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index aac24591d22..1252b17b980 100644
@@ -607,11 +607,13 @@ void create_branches_recursively(struct repository *r, const char *name,
 	 */
 	for (i = 0; i < submodule_entry_list.entry_nr; i++) {
 		if (submodule_entry_list.entries[i].repo == NULL) {
+			int code = die_message(
+				_("submodule '%s': unable to find submodule"),
+				submodule_entry_list.entries[i].submodule->name);
 			if (advice_enabled(ADVICE_SUBMODULES_NOT_UPDATED))
 				advise(_("You may try updating the submodules using 'git checkout %s && git submodule update --init'"),
 				       start_commitish);
-			die(_("submodule '%s': unable to find submodule"),
-			    submodule_entry_list.entries[i].submodule->name);
+			exit(code);
 		}
 
 		if (submodule_create_branch(

gitgitgadget

From: Glen Choo <[email protected]>

"git branch --set-upstream-to" behaves differently when advice is enabled/disabled:

|                 | error prefix | exit code |
|-----------------+--------------+-----------|
| advice enabled  | error:       |         1 |
| advice disabled | fatal:       |       128 |

Make both cases consistent by using die_message() when advice is enabled (this was first proposed in [1]).

[1] https://lore.kernel.org/git/[email protected]

---
 branch.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

index 1252b17b980..ca7f0c3adf8 100644
@@ -388,12 +388,10 @@ static void dwim_branch_start(struct repository *r, const char *start_name,
 	real_ref = NULL;
 	if (get_oid_mb(start_name, &oid)) {
 		if (explicit_tracking) {
-			if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {
-				error(_(upstream_missing), start_name);
-				advise(_(upstream_advice));
-				exit(1);
-			}
-			die(_(upstream_missing), start_name);
+			int code = die_message(_(upstream_missing), start_name);
+			advise_if_enabled(ADVICE_SET_UPSTREAM_FAILURE,
+					  _(upstream_advice));
+			exit(code);
 		}
 		die(_("not a valid object name: '%s'"), start_name);
 	}

gitgitgadget

From: Glen Choo <[email protected]>

Replace an instance of "exit(-1)" with "exit(1)". We don't use negative exit codes - they are misleading because Unix machines will coerce them to 8-bit unsigned values, losing the sign.

---
 branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ca7f0c3adf8..581afd634da 100644
@@ -263,7 +263,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 		string_list_append(tracking.srcs, orig_ref);
 	if (install_branch_config_multiple_remotes(config_flags, new_ref,
 				tracking.remote, tracking.srcs) < 0)
-		exit(-1);
+		exit(1);
 
 cleanup:
 	string_list_clear(&tracking_srcs, 0);

gitgitgadget

Thanks, but sorry that I've already merged the previous round. Let me turn them into incrementals.

From: Glen Choo <[email protected]>

For two cases in which we do not explicitly pass --track=<choice> option down to the submodule--helper subprocess, we have comments that say "we do not have to pass --track", but in fact we not just do not have to, but it would be incorrect to pass any --track option to the subprocess (instead, the correct behaviour is to let the subprocess figure out what is the appropriate tracking mode to use).

---
 branch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index ed6f993aa6..8ee9f43539 100644
@@ -549,9 +549,9 @@ static int submodule_create_branch(struct repository *r,
 		strvec_push(&child.args, "--track=inherit");
 		break;
 	case BRANCH_TRACK_UNSPECIFIED:
-		/* Default for "git checkout". No need to pass --track. */
+		/* Default for "git checkout". Do not pass --track. */
 	case BRANCH_TRACK_REMOTE:
-		/* Default for "git branch". No need to pass --track. */
+		/* Default for "git branch". Do not pass --track. */
 		break;
 	}
 

2.35.1-917-g7c4048624a

From: Glen Choo <[email protected]>

In the dwim_branch_start(), when we cannot find an appropriate upstream, we will die with the same message anyway, whether we issue an advice message.

Flip the code around a bit and simplify the flow using advise_if_enabled() function.

---
 branch.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

index 8ee9f43539..b673143cbe 100644
@@ -383,13 +383,10 @@ static void dwim_branch_start(struct repository *r, const char *start_name,
 	real_ref = NULL;
 	if (get_oid_mb(start_name, &oid)) {
 		if (explicit_tracking) {
-			if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {
-				int code = die_message(_(upstream_missing),
-						       start_name);
-				advise(_(upstream_advice));
-				exit(code);
-			}
-			die(_(upstream_missing), start_name);
+			int code = die_message(_(upstream_missing), start_name);
+			advise_if_enabled(ADVICE_SET_UPSTREAM_FAILURE,
+					  _(upstream_advice));
+			exit(code);
 		}
 		die(_("Not a valid object name: '%s'."), start_name);
 	}

2.35.1-917-g7c4048624a

Ah whoops, sorry I didn't send them out sooner.

Thanks for that :)

No, it was me this time. I have prepared 'next' long in advance so I could have redone it if I delayed pushing the result out, but in any case, we are working asynchronously and these things are bound to happen.

Thanks for working on the topic.