On 11/09/2024 21:52, Junio C Hamano wrote:
"Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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);
}
