diff mbox series

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

Message ID 8e6ea3478b376b24835f3a3fef7fca39bed0afec.1648584079.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit ac59c742de5f548ed07735fb212cc87129383bcd
Headers show
Series branch --recurse-submodules: Bug fixes and clean ups | expand

Commit Message

Glen Choo March 29, 2022, 8:01 p.m. UTC 
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(-)

Comments

Ævar Arnfjörð Bjarmason March 30, 2022, 10:28 p.m. UTC | #1 
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.
Glen Choo March 31, 2022, 4:52 p.m. UTC | #2 
Æ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.
diff mbox series

Patch

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);
 		}