Re: [PATCH 1/2] sha1_name: fix error message for @{u}

[Junio C Hamano <gitster@pobox.com>]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Currently, when no (valid) upstream is configured for a branch, we get
> an error like:
>
>   $ git show @{u}
>   error: No upstream configured for branch 'upstream-error'
>   error: No upstream configured for branch 'upstream-error'
>   fatal: ambiguous argument '@{u}': unknown revision or path not in the working tree.
>   Use '--' to separate paths from revisions, like this:
>   'git <command> [<revision>...] -- [<file>...]'
>
> The "error: " line actually appears twice, and the rest of the error
> message is useless.  In sha1_name.c:interpret_branch_name(), there is
> really no point in processing further if @{u} couldn't be resolved, and
> we might as well die() instead of returning an error().  After making
> this change, you get:
>
>   $ git show @{u}
>   fatal: No upstream configured for branch 'upstream-error'
>
> Also tweak a few tests in t1507 to expect this output.

Does a failure in interpret-branch-name that issue these error
messages always followed by die() in the caller?  I know you looked
at the cases you noticed as an end-user (like the above "git show @{u}"
example), but if some codepaths did this:

	if (interpret-branch-name()) {
        	you do not seem to have upstream defined,
	        so I will helpfully do something else that
                you probably have meant.
	}

this patch will break that codepath you did not look.

I do not offhand know if there is such a codepath, so if you did a
code audit and know this patch is regression-free, please say that
in the log message.  "I ran all the tests and they passed" is not
good enough.

Other than that, the idea sounds OK.

>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  sha1_name.c                   | 13 +++++++------
>  t/t1507-rev-parse-upstream.sh | 15 +++++----------
>  2 files changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 3820f28..416a673 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -1033,14 +1033,15 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
>  	 * points to something different than a branch.
>  	 */
>  	if (!upstream)
> -		return error(_("HEAD does not point to a branch"));
> +		die(_("HEAD does not point to a branch"));
>  	if (!upstream->merge || !upstream->merge[0]->dst) {
>  		if (!ref_exists(upstream->refname))
> -			return error(_("No such branch: '%s'"), cp);
> -		if (!upstream->merge)
> -			return error(_("No upstream configured for branch '%s'"),
> -				     upstream->name);
> -		return error(
> +			die(_("No such branch: '%s'"), cp);
> +		if (!upstream->merge) {
> +			die(_("No upstream configured for branch '%s'"),
> +				upstream->name);
> +		}
> +		die(
>  			_("Upstream branch '%s' not stored as a remote-tracking branch"),
>  			upstream->merge[0]->src);
>  	}
> diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
> index b27a720..2a19e79 100755
> --- a/t/t1507-rev-parse-upstream.sh
> +++ b/t/t1507-rev-parse-upstream.sh
> @@ -129,8 +129,7 @@ test_expect_success 'branch@{u} works when tracking a local branch' '
>  
>  test_expect_success 'branch@{u} error message when no upstream' '
>  	cat >expect <<-EOF &&
> -	error: No upstream configured for branch ${sq}non-tracking${sq}
> -	fatal: Needed a single revision
> +	fatal: No upstream configured for branch ${sq}non-tracking${sq}
>  	EOF
>  	error_message non-tracking@{u} 2>actual &&
>  	test_i18ncmp expect actual
> @@ -138,8 +137,7 @@ test_expect_success 'branch@{u} error message when no upstream' '
>  
>  test_expect_success '@{u} error message when no upstream' '
>  	cat >expect <<-EOF &&
> -	error: No upstream configured for branch ${sq}master${sq}
> -	fatal: Needed a single revision
> +	fatal: No upstream configured for branch ${sq}master${sq}
>  	EOF
>  	test_must_fail git rev-parse --verify @{u} 2>actual &&
>  	test_i18ncmp expect actual
> @@ -147,8 +145,7 @@ test_expect_success '@{u} error message when no upstream' '
>  
>  test_expect_success 'branch@{u} error message with misspelt branch' '
>  	cat >expect <<-EOF &&
> -	error: No such branch: ${sq}no-such-branch${sq}
> -	fatal: Needed a single revision
> +	fatal: No such branch: ${sq}no-such-branch${sq}
>  	EOF
>  	error_message no-such-branch@{u} 2>actual &&
>  	test_i18ncmp expect actual
> @@ -156,8 +153,7 @@ test_expect_success 'branch@{u} error message with misspelt branch' '
>  
>  test_expect_success '@{u} error message when not on a branch' '
>  	cat >expect <<-EOF &&
> -	error: HEAD does not point to a branch
> -	fatal: Needed a single revision
> +	fatal: HEAD does not point to a branch
>  	EOF
>  	git checkout HEAD^0 &&
>  	test_must_fail git rev-parse --verify @{u} 2>actual &&
> @@ -166,8 +162,7 @@ test_expect_success '@{u} error message when not on a branch' '
>  
>  test_expect_success 'branch@{u} error message if upstream branch not fetched' '
>  	cat >expect <<-EOF &&
> -	error: Upstream branch ${sq}refs/heads/side${sq} not stored as a remote-tracking branch
> -	fatal: Needed a single revision
> +	fatal: Upstream branch ${sq}refs/heads/side${sq} not stored as a remote-tracking branch
>  	EOF
>  	error_message bad-upstream@{u} 2>actual &&
>  	test_i18ncmp expect actual