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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 1, 2022, 10:03 UTC
Message-ID: <220401.86lewpnlie.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220331224118.3014407-2-gitster@pobox.com>

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

Show 37 quoted lines

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