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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 30, 2022, 22:28 UTC
Message-ID: <220331.867d8bt6i6.gmgdl@evledraar.gmail.com>
In-Reply-To: <8e6ea3478b376b24835f3a3fef7fca39bed0afec.1648584079.git.gitgitgadget@gmail.com>

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

Show 14 quoted lines

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

Show 22 quoted lines

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