Re: [PATCH 1/4] remote: fix set-branches when no branches are set

[Patrick Steinhardt <ps@pks.im>]

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