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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 9, 2015, 18:47 UTC
Message-ID: <xmqq38207m77.fsf@gitster.dls.corp.google.com>
In-Reply-To: <9f6f3432467c2b12329c16a46c52670f772993e5.1433763494.git.mhagger@alum.mit.edu>

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

Show 10 quoted lines

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

I am 80% convinced ;-)

The existing caller never used this for removing tags, so 'could not remove branch' was equally correct for it and was more specific than 'could not remove reference'. If you change it to 'could not remove that thing %s', it would still be correct for the existing caller; it would be even less specific for them, though ;-)

The new callers you will add in later patch of course cannot live with 'could not remove branch', so I think that this is an acceptable compromise we can live with. If somebody later wants to make the message more specific, they can add code that switches on the prefix of the ref when coming up with the error message (and use that code consistently in other error messages e.g. 'could not add reference').

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 c413282..2a2a06d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2827,7 +2827,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;