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

[Glen Choo <chooglen@google.com>]

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