remote: branch setting fixes #1789
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1567,8 +1567,12 @@ static int update(int argc, const char **argv, const char *prefix) | |
|
|
||
|
There was a problem hiding this comment. On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Wed, Sep 11, 2024 at 03:18:34PM +0000, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <[email protected]>
>
> To replace the list of branches to be fetched "git remote set-branches"
> first removes the fetch refspecs for the remote and then creates a new
> set of fetch refspecs based and the branches passed on the commandline.
s/and/on/
> When deleting the existing refspecs git_config_set_multivar_gently()
> will return a non-zero result if there was nothing to delete.
> Unfortunately the calling code treats that as an error and bails out
> rather than setting up the new branches. Fix this by not treating a
> return value of CONFIG_NOTHING_SET as an error.
>
> Reported-by: Han Jiang <[email protected]>
> Signed-off-by: Phillip Wood <[email protected]>
> ---
> builtin/remote.c | 8 ++++++--
> t/t5505-remote.sh | 14 +++++++++++---
> 2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index d1f9292ed2b..794396ba02f 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -1567,8 +1567,12 @@ static int update(int argc, const char **argv, const char *prefix)
>
> static int remove_all_fetch_refspecs(const char *key)
> {
> - return git_config_set_multivar_gently(key, NULL, NULL,
> - CONFIG_FLAGS_MULTI_REPLACE);
> + int res = git_config_set_multivar_gently(key, NULL, NULL,
> + CONFIG_FLAGS_MULTI_REPLACE);
> + if (res == CONFIG_NOTHING_SET)
> + res = 0;
> +
> + return res;
> }
Makes sense.
> static void add_branches(struct remote *remote, const char **branches,
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index 08424e878e1..cfbd6139e00 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -1131,7 +1131,9 @@ test_expect_success 'remote set-branches' '
> +refs/heads/next:refs/remotes/scratch/next
> +refs/heads/seen:refs/remotes/scratch/seen
> EOF
> -
> + cat <<-\EOF >expect.replace-missing &&
s/ / /
Also, the redirect typically comes before the heredoc marker.
> + +refs/heads/topic:refs/remotes/scratch/topic
> + EOF
> git clone .git/ setbranches &&
> (
> cd setbranches &&
> @@ -1161,14 +1163,20 @@ test_expect_success 'remote set-branches' '
>
> git remote set-branches --add scratch seen &&
> git config --get-all remote.scratch.fetch >config-result &&
> - sort <config-result >../actual.respect-ffonly
> + sort <config-result >../actual.respect-ffonly &&
> +
> + git config --unset-all remote.scratch.fetch &&
> + git remote set-branches scratch topic &&
> + git config --get-all remote.scratch.fetch \
> + >../actual.replace-missing
I wonder whether we'd rather wnat to wire this up in a new test instead
of altering an existing one.
Patrick |
||
| static int remove_all_fetch_refspecs(const char *key) | ||
| { | ||
| int res = git_config_set_multivar_gently(key, NULL, NULL, | ||
| CONFIG_FLAGS_MULTI_REPLACE); | ||
| if (res == CONFIG_NOTHING_SET) | ||
| res = 0; | ||
|
|
||
| return res; | ||
| } | ||
|
|
||
| static void add_branches(struct remote *remote, const char **branches, | ||
|
|
@@ -1599,6 +1603,7 @@ static int set_remote_branches(const char *remotename, const char **branches, | |
| } | ||
|
There was a problem hiding this comment. On the Git mailing list, Junio C Hamano wrote (reply to this): "Phillip Wood via GitGitGadget" <[email protected]> writes:
> From: Phillip Wood <[email protected]>
>
> 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 <[email protected]>
> ---
> 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);
}
There was a problem hiding this comment. On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Wed, Sep 11, 2024 at 01:52:16PM -0700, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[email protected]> writes:
>
> > From: Phillip Wood <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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.
PatrickThere was a problem hiding this comment. On the Git mailing list, Junio C Hamano wrote (reply to this): Patrick Steinhardt <[email protected]> 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();
}
There was a problem hiding this comment. On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Thu, Sep 12, 2024 at 09:22:13AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <[email protected]> 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!
PatrickThere was a problem hiding this comment. On the Git mailing list, [email protected] wrote (reply to this): On 11/09/2024 21:52, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[email protected]> 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);
> > }
> There was a problem hiding this comment. On the Git mailing list, Junio C Hamano wrote (reply to this): [email protected] writes:
> On 11/09/2024 21:52, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <[email protected]> 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. |
||
|
|
||
| if (!add_mode && remove_all_fetch_refspecs(key.buf)) { | ||
| error(_("could not remove existing fetch refspec")); | ||
| strbuf_release(&key); | ||
| return 1; | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Junio C Hamano wrote (reply to this):