Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remote: branch setting fixes #1789

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

remote: branch setting fixes #1789

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d95a07a
remote: fix set-branches when no branches are set
phillipwood Sep 6, 2024
a8dfe40
remote: print an error if refspec cannot be removed
phillipwood Sep 10, 2024
f30c77b
remote add: use strvec to store tracking branches
phillipwood Sep 10, 2024
dba3124
remote: check branch names
phillipwood Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions builtin/remote.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1567,8 +1567,12 @@ static int update(int argc, const char **argv, const char *prefix)

Copy link

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):

"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.

Copy link

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, Patrick Steinhardt wrote (reply to this):

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

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,
Expand Down
14 changes: 11 additions & 3 deletions t/t5505-remote.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand Down Expand Up @@ -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' '
Expand Down