Re: [PATCH v2 05/12] delete_refs(): improve error message

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 15, 2015, 18:29 UTC
Message-ID: <xmqq7fr4akr7.fsf@gitster.dls.corp.google.com>
In-Reply-To: <64949bba1031b19abf86629267b8bc40666a4631.1434206062.git.mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Subject: Re: [PATCH v2 05/12] delete_refs(): improve error message

I'd call this "make error message more generic".

Show 11 quoted lines

> Change the error message from
>
>     Could not remove branch %s
>
> to
>
>     could not remove reference %s
>
> * s/branch/reference/. This change makes sense even for the existing
>   caller, which uses the function to delete remote-tracking
>   branches.

and replace this bullet with something like:

* Originally 'branch -d' was the only caller of this, but as part
  of the refs API, we will allow it to be called on refs that is not
  a branch or a remote-tracking branch.

as calling a remote-tracking branch a 'branch' is not incorrect per-se. What would count as true improvement is ...

> * Convert it to lower case, as per our usual convention.

... this one. If somebody eventually chooses to make the message finer grained by switching on the prefix refs/{what}, so that the user can differentiate between branches, remote-tracking branches, tags, etc., that would also count as improvement as well.

Show 19 quoted lines

>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 6f62bd1..5386077 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2831,7 +2831,7 @@ int delete_refs(struct string_list *refnames)
>  		const char *refname = refnames->items[i].string;
>  
>  		if (delete_ref(refname, NULL, 0))
> -			result |= error(_("Could not remove branch %s"), refname);
> +			result |= error(_("could not remove reference %s"), refname);
>  	}
>  
>  	return result;