| Message ID | d95a07a22aadccd0c0e0e63bbb98b3a4306545d2.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> > > 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. > 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. Makes sense. > 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 && > + +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 && OK, so we get rid of all "fetch" refspec elements and make sure we can ... > + git remote set-branches scratch topic && ... set a single new one like this ... > + git config --get-all remote.scratch.fetch \ > + >../actual.replace-missing and we expect the mapping to appear in the output. For maintainability, it would be better to also sort this one to mimick the other one that contain multiple entries in the output, but because we expect only one entry to be in the output, not sorting is OK for now. Looks good. Thanks.
On Wed, Sep 11, 2024 at 03:18:34PM +0000, Phillip Wood via GitGitGadget wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > 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 <jhcarl0814@gmail.com> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > 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
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; } 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 && + +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 ) && test_cmp expect.initial actual.initial && test_cmp expect.add actual.add && test_cmp expect.replace actual.replace && test_cmp expect.add-two actual.add-two && test_cmp expect.setup-ffonly actual.setup-ffonly && - test_cmp expect.respect-ffonly actual.respect-ffonly + test_cmp expect.respect-ffonly actual.respect-ffonly && + test_cmp expect.replace-missing actual.replace-missing ' test_expect_success 'remote set-branches with --mirror' '