Re: [PATCH 2/4] remote: print an error if refspec cannot be removed
- From
- phillip.wood123@gmail.com <phillip.wood123@gmail.com>
- Date
- Sep 13, 2024, 15:11 UTC
- 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:
Show 10 quoted lines
> "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
Show 9 quoted lines
> 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);
>
> }
>