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

From: Junio C Hamano <gitster@pobox.com>
Date: May 22, 2013, 17:35 UTC
Message-ID: <7vhahukjeg.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1369219195-20096-2-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 2. Callers calling in with programmatic data, and expecting the function
>    to return and not die().  In this case, why would anyone ever
>    construct a string containing "@{u}" programmatically in the first
>    place?

If you have to ask why, and cannot answer the question yourself, then you would not know if such a caller exists. After a code audit, I know there is no such caller that appends @{u} but if you were writing a quick-and-dirty caller, I would not be surprised if you find it more convenient to form a textual extended SHA-1 expression and have get_sha1() do its work, instead of asking the same question programmatically.

In this case, I think you already checked there is no such problem, and it is a more straight-forward justification to say that you did a code-audit and made sure that all the callers that used to hit one of these errors() want to die().

Also such a caller, if existed, would either

    (1) want to die itself, in which case these error() messages are
        superfluous; or

    (2) want to continue (possibly dying with its own message), in
        which case these error() messages are unwanted.

Because you are changing only the existing call sites of error() into die(), and not changing silent -1 returns to die(), this change is safe for both kinds of such callers, I think.

Show 16 quoted lines

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

OK.

>  	if (!upstream->merge || !upstream->merge[0]->dst) {
>  		if (!ref_exists(upstream->refname))
> +			die(_("No such branch: '%s'"), cp);

OK.

> +		if (!upstream->merge) {
> +			die(_("No upstream configured for branch '%s'"),
> +				upstream->name);
> +		}

OK, but I would not add extra {} if I were doing this change.

> +		die(
>  			_("Upstream branch '%s' not stored as a remote-tracking branch"),
>  			upstream->merge[0]->src);

OK, but I would fix the indentation while at it if I were doing this change.

Show 11 quoted lines

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

Much nicer.