| Message ID | a8dfe403d0683aec4265bf920921e45d5b59cec3.1726067917.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | remote: branch setting fixes | expand |
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > 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); }
On Wed, Sep 11, 2024 at 01:52:16PM -0700, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > 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); > > } 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
Patrick Steinhardt <ps@pks.im> writes: > 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. Hmph, I thought there was somebody doing !! to canonicalize the return value to exit status in the call chain. ... goes and looks again ... After finding the subcommand in fn, cmd_remote() ends with if (fn) { return !!fn(argc, argv, prefix); } else { ... return !!show_all(); }
On Thu, Sep 12, 2024 at 09:22:13AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > 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. > > Hmph, I thought there was somebody doing !! to canonicalize the > return value to exit status in the call chain. > > ... goes and looks again ... > > After finding the subcommand in fn, cmd_remote() ends with > > if (fn) { > return !!fn(argc, argv, prefix); > } else { > ... > return !!show_all(); > } Ah, never mind in that case. I didn't look far enough indeed. Thanks for correcting my claim! Patrick
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); > > } >
phillip.wood123@gmail.com writes: > 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. That makes sense. Thanks.
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; }