Re: [PATCH 2/4] remote: print an error if refspec cannot be removed
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 11, 2024, 20:52 UTC
- Message-ID
- <xmqqseu56hhb.fsf@gitster.g>
- In-Reply-To
- <a8dfe403d0683aec4265bf920921e45d5b59cec3.1726067917.git.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 23 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If the existing fetch refspecs cannot be removed when replacing the set
> of branches to fetch with "git remote set-branches" the command silently
> fails. Add an error message to tell the user what when wrong.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> builtin/remote.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 794396ba02f..4dbf7a4c506 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -1603,6 +1603,7 @@ static int set_remote_branches(const char *remotename, const char **branches,
> }
>
> 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 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.
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);}