From: phillip.wood123@gmail.com
Date: Fri, 13 Sep 2024 15:11:53 GMT
Subject: Re: [PATCH 2/4] remote: print an error if refspec cannot be removed
Message-ID: <acd287c8-990b-42b7-85dd-a206a887b8ee@gmail.com>
In-Reply-To: <xmqqseu56hhb.fsf@gitster.g>

On 11/09/2024 21:52, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>   	if (!add_mode && remove_all_fetch_refspecs(key.buf)) {
>> +		error(_("could not remove existing fetch refspec"));
>>   		strbuf_release(&key);
>>   		return 1;
>>   	}
> 
> It is a minor point, but would it help to say what we tried to
> remove (e.g. "from remote X") or is it too obvious to the end user
> in the context they get this error?

The user has to give the remote name on the command line so I think it 
should be obvious to the user.

> The reason why I had the above question was because inserting error()
> before strbuf_release(&key) looked curious and I initially suspected
> that it was because key was used in the error message somehow, but it
> turns out that is not the case at all.

Arguably we should refactor this to use our standard "goto cleanup" pattern.

Best Wishes

Phillip

> IOW, I would have expected something more like this:
> 
>   	if (!add_mode && remove_all_fetch_refspecs(key.buf)) {
>   		strbuf_release(&key);
> +		return error(_("failed to remove fetch refspec from '%s'"),
> +				remotename);
> 
>   	}
>