[PATCH 0/4] branch --recurse-submodules: Bug fixes and clean ups

[PATCH 0/4] branch --recurse-submodules: Bug fixes and clean ups

[Glen Choo via GitGitGadget]

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/211210.86ee6ldwlc.gmgdl@evledraar.gmail.com. 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

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

[Glen Choo via GitGitGadget]

From: Glen Choo <chooglen@google.com>

"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 <chooglen@google.com>
---
 branch.c                    | 29 +++++++++++++++++++++++++---
 builtin/submodule--helper.c |  7 ++++---
 t/t3207-branch-submodule.sh | 38 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 67 insertions(+), 7 deletions(-)

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
-- 
gitgitgadget

[PATCH 2/4] branch: give submodule updating advice before exit

[Glen Choo via GitGitGadget]

From: Glen Choo <chooglen@google.com>

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

Signed-off-by: Glen Choo <chooglen@google.com>
---
 branch.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/branch.c b/branch.c
index 7377b9f451a..133e6047bc6 100644
--- a/branch.c
+++ b/branch.c
@@ -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

[PATCH 3/4] branch --set-upstream-to: be consistent when advising

[Glen Choo via GitGitGadget]

From: Glen Choo <chooglen@google.com>

"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/211210.86ee6ldwlc.gmgdl@evledraar.gmail.com

Signed-off-by: Glen Choo <chooglen@google.com>
---
 branch.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/branch.c b/branch.c
index 133e6047bc6..4a8796489c7 100644
--- a/branch.c
+++ b/branch.c
@@ -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

[PATCH 4/4] branch: remove negative exit code

[Glen Choo via GitGitGadget]

From: Glen Choo <chooglen@google.com>

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.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/branch.c b/branch.c
index 4a8796489c7..eb231b950bb 100644
--- a/branch.c
+++ b/branch.c
@@ -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

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

[Junio C Hamano]

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> 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.

Re: [PATCH 2/4] branch: give submodule updating advice before exit

[Junio C Hamano]

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Glen Choo <chooglen@google.com>
>
> Fix a bug where "hint:" was printed _before_ "fatal:" (instead of the
> other way around).
>
> Signed-off-by: Glen Choo <chooglen@google.com>
> ---
>  branch.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 7377b9f451a..133e6047bc6 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -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(

Great.

Thanks.

Re: [PATCH 3/4] branch --set-upstream-to: be consistent when advising

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 29 2022, Glen Choo via GitGitGadget wrote:

> From: Glen Choo <chooglen@google.com>
>
> "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/211210.86ee6ldwlc.gmgdl@evledraar.gmail.com

Thanks for following up on this :)

> Signed-off-by: Glen Choo <chooglen@google.com>
> ---
>  branch.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 133e6047bc6..4a8796489c7 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -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);
>  		}

This is really close to being much better, i.e. we can now just do this
(this is on top of your branch):
	
	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);
	 	}
	
I.e. the only reason we needed to mention upstream_missing multiple
times is because we didn't have something like die_message() before, now
we can just skip that other "die" entirely.

The advise_if_enabled() might be worthwhile to change while at it, maybe
not.

But also useful, is that we can now simply inline the "upstream_missing"
string, which will give us type checks for the printf format. The reason
we had a variable before was also because of the lack of die_message()>

I notice that we can do likewise with the advice itself, and with
"upstream_not_branch" if we either make that a "goto", or add a trivial
helper function.

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

[Glen Choo]

Junio C Hamano <gitster@pobox.com> writes:

> "Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> 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.

Re: [PATCH 3/4] branch --set-upstream-to: be consistent when advising

[Glen Choo]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Tue, Mar 29 2022, Glen Choo via GitGitGadget wrote:
>
>> From: Glen Choo <chooglen@google.com>
>>
>> "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/211210.86ee6ldwlc.gmgdl@evledraar.gmail.com
>
> Thanks for following up on this :)

:)

>> Signed-off-by: Glen Choo <chooglen@google.com>
>> ---
>>  branch.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/branch.c b/branch.c
>> index 133e6047bc6..4a8796489c7 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -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);
>>  		}
>
> This is really close to being much better, i.e. we can now just do this
> (this is on top of your branch):
> 	
> 	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);
> 	 	}
> 	
> I.e. the only reason we needed to mention upstream_missing multiple
> times is because we didn't have something like die_message() before, now
> we can just skip that other "die" entirely.

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

> The advise_if_enabled() might be worthwhile to change while at it, maybe
> not.

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

>
> But also useful, is that we can now simply inline the "upstream_missing"
> string, which will give us type checks for the printf format. The reason
> we had a variable before was also because of the lack of die_message()>
>
> I notice that we can do likewise with the advice itself, and with
> "upstream_not_branch" if we either make that a "goto", or add a trivial
> helper function.

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.

[PATCH v2 0/4] branch --recurse-submodules: Bug fixes and clean ups

[Glen Choo via 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/211210.86ee6ldwlc.gmgdl@evledraar.gmail.com
      
     +    Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
          Signed-off-by: Glen Choo <chooglen@google.com>
      
       ## 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

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

[Glen Choo via GitGitGadget]

From: Glen Choo <chooglen@google.com>

"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 <chooglen@google.com>
---
 branch.c                    | 29 +++++++++++++++++++++++++---
 builtin/submodule--helper.c |  7 ++++---
 t/t3207-branch-submodule.sh | 38 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 67 insertions(+), 7 deletions(-)

diff --git a/branch.c b/branch.c
index 6b31df539a5..aac24591d22 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". 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(
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
-- 
gitgitgadget

[PATCH v2 2/4] branch: give submodule updating advice before exit

[Glen Choo via GitGitGadget]

From: Glen Choo <chooglen@google.com>

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

Signed-off-by: Glen Choo <chooglen@google.com>
---
 branch.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/branch.c b/branch.c
index aac24591d22..1252b17b980 100644
--- a/branch.c
+++ b/branch.c
@@ -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

[PATCH v2 3/4] branch --set-upstream-to: be consistent when advising

[Glen Choo via GitGitGadget]

From: Glen Choo <chooglen@google.com>

"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/211210.86ee6ldwlc.gmgdl@evledraar.gmail.com

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Glen Choo <chooglen@google.com>
---
 branch.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/branch.c b/branch.c
index 1252b17b980..ca7f0c3adf8 100644
--- a/branch.c
+++ b/branch.c
@@ -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

[PATCH v2 4/4] branch: remove negative exit code

[Glen Choo via GitGitGadget]

From: Glen Choo <chooglen@google.com>

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.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/branch.c b/branch.c
index ca7f0c3adf8..581afd634da 100644
--- a/branch.c
+++ b/branch.c
@@ -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

Re: [PATCH v2 0/4] branch --recurse-submodules: Bug fixes and clean ups

[Junio C Hamano]

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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

[PATCH 1/2] branch: rework comments for future developers

[Junio C Hamano]

From: Glen Choo <chooglen@google.com>

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

Signed-off-by: Glen Choo <chooglen@google.com>
---
 branch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/branch.c b/branch.c
index ed6f993aa6..8ee9f43539 100644
--- a/branch.c
+++ b/branch.c
@@ -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

[PATCH 2/2] branch.c: simplify advice-and-die sequence

[Junio C Hamano]

From: Glen Choo <chooglen@google.com>

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.

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Glen Choo <chooglen@google.com>
---
 branch.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/branch.c b/branch.c
index 8ee9f43539..b673143cbe 100644
--- a/branch.c
+++ b/branch.c
@@ -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

Re: [PATCH 2/2] branch.c: simplify advice-and-die sequence

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 31 2022, Junio C Hamano wrote:

> From: Glen Choo <chooglen@google.com>
>
> 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.
>
> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Glen Choo <chooglen@google.com>
> ---
>  branch.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 8ee9f43539..b673143cbe 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -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);
>  	}

Looks good, in case you missed it I noted that we could also get extra
help from the compiler by folding the "upstream_missing" variable inline
as an argument to die_message(), likewise for upstream_advice.

I.e. the latter already had only one user, but the former was used in
two places (now one).

That was mainly for getting more compiler aid with format checking,
although the readibility of avoiding the indirection is arguably a good
reason to do it.

But I see from testing just now that I was (mostly) wrong about
that. I.e. if we do:
	
	diff --git a/branch.c b/branch.c
	index 6b31df539a5..8a704c3ff53 100644
	--- a/branch.c
	+++ b/branch.c
	@@ -339,8 +339,7 @@ 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_missing[] = "blah %s blah %s";
	 static const char upstream_advice[] =
	 N_("\n"
	 "If you are planning on basing your work on an upstream\n

Both my local gcc and clang will warn on it. What I was recalling is
that if you try these variants:
	
	diff --git a/branch.c b/branch.c
	index 6b31df539a5..eea50605c8c 100644
	--- a/branch.c
	+++ b/branch.c
	@@ -339,8 +339,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"
	@@ -384,6 +382,10 @@ static void dwim_branch_start(struct repository *r, const char *start_name,
	 
	 	real_ref = NULL;
	 	if (get_oid_mb(start_name, &oid)) {
	+		char *upstream_missing = "blah %s blah %s";
	+		const char *upstream_missing = "blah %s blah %s";
	+		const char *const upstream_missing = "blah %s blah %s";
	+		static const char upstream_missing[] = "blah %s blah %s";
	 		if (explicit_tracking) {
	 			if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {
	 				error(_(upstream_missing), start_name);

My clang version starts warning on the 3rd one (const char *const), but
it takes until the 4th one (static const char ... []) until GCC will
warn on it.

We've had bugs in the past where we passed some variants of that to a
format-expecting function.

So I think that's a good argument for wider use of "const char *const"
in some cases over "const char *" if possible. E.g. if I do this:
	
	diff --git a/sequencer.c b/sequencer.c
	index a1bb39383db..cd8f59c2a2d 100644
	--- a/sequencer.c
	+++ b/sequencer.c
	@@ -3035,7 +3035,6 @@ static int create_seq_dir(struct repository *r)
	 {
	 	enum replay_action action;
	 	const char *in_progress_error = NULL;
	-	const char *in_progress_advice = NULL;
	 	unsigned int advise_skip =
	 		refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD") ||
	 		refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD");
	@@ -3044,19 +3043,19 @@ static int create_seq_dir(struct repository *r)
	 		switch (action) {
	 		case REPLAY_REVERT:
	 			in_progress_error = _("revert is already in progress");
	-			in_progress_advice =
	-			_("try \"git revert (--continue | %s--abort | --quit)\"");
	 			break;
	 		case REPLAY_PICK:
	 			in_progress_error = _("cherry-pick is already in progress");
	-			in_progress_advice =
	-			_("try \"git cherry-pick (--continue | %s--abort | --quit)\"");
	 			break;
	 		default:
	 			BUG("unexpected action in create_seq_dir");
	 		}
	 	}
	 	if (in_progress_error) {
	+		const char *in_progress_advice = action == REPLAY_REVERT
	+			? _("try \"git cherry-pick (--continue | %s--abort | --quit)\" %s")
	+			: _("try \"git revert (--continue | %s--abort | --quit)\" %s");
	+
	 		error("%s", in_progress_error);
	 		if (advice_enabled(ADVICE_SEQUENCER_IN_USE))
	 			advise(in_progress_advice,

Neither clang nor gcc will detect that I snuck in an extra %s into the
message, which would be a runtime error, but make that "const char
*const" and clang will spot it, but gcc won't.

This was with Debian clang version 13.0.1-3+b2 & gcc (GCC) 12.0.1
20220120 (experimental). I also tested this last one on gcc (Debian
10.2.1-6) 10.2.1 20210110. YMMV.

Re: [PATCH v2 0/4] branch --recurse-submodules: Bug fixes and clean ups

[Glen Choo]

Junio C Hamano <gitster@pobox.com> writes:

> "Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> 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.
>
> Thanks, but sorry that I've already merged the previous round.  Let
> me turn them into incrementals.

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

Thanks for that :)

Re: [PATCH v2 0/4] branch --recurse-submodules: Bug fixes and clean ups

[Junio C Hamano]

Glen Choo <chooglen@google.com> writes:

>> Thanks, but sorry that I've already merged the previous round.  Let
>> me turn them into incrementals.
>
> Ah whoops, sorry I didn't send them out sooner.

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 that :)

Thanks for working on the topic.