On Wed, Sep 11, 2024 at 01:52:16PM -0700, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx>
> > ---
> > 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);
>
> }
I don't think we want to return the error code from `error()`, do we?
`set_branches()` is wired up as a subcommand, so we'd ultimately do
`exit(-1)` instead of `exit(1)` if we returned the `error()` code here.
Patrick
