[PATCH 0/4] remote: branch setting fixes

[PATCH 0/4] remote: branch setting fixes

[Phillip Wood via GitGitGadget]

This series fixes some rough edges when setting remote tracking branches
with "git remote set-branches" or "git remote add -t". Han Jiang reported
that if there is no fetch refspec set for a remote then "git remote
set-branches" silently fails. While investigating that I noticed that the
code does not check that the branch names are syntactically valid refnames
and so can create invalid refspecs. This series fixes both issues.

Phillip Wood (4):
  remote: fix set-branches when no branches are set
  remote: print an error if refspec cannot be removed
  remote add: use strvec to store tracking branches
  remote: check branch names

 builtin/remote.c  | 41 ++++++++++++++++++++++++++++++++---------
 t/t5505-remote.sh | 42 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 71 insertions(+), 12 deletions(-)


base-commit: 4590f2e9412378c61eac95966709c78766d326ba
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1789%2Fphillipwood%2Fremote-set-branches-missing-fetch-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1789/phillipwood/remote-set-branches-missing-fetch-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1789
-- 
gitgitgadget

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

[Phillip Wood via GitGitGadget]

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.

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;
 }
 
 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' '
-- 
gitgitgadget

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

[Junio C Hamano]

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

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

[Patrick Steinhardt]

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

[PATCH 2/4] remote: print an error if refspec cannot be removed

[Phillip Wood via GitGitGadget]

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;
 	}
-- 
gitgitgadget

Re: [PATCH 2/4] remote: print an error if refspec cannot be removed

[Junio C Hamano]

"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);

 	}

Re: [PATCH 2/4] remote: print an error if refspec cannot be removed

[Patrick Steinhardt]

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

Re: [PATCH 2/4] remote: print an error if refspec cannot be removed

[Junio C Hamano]

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();
	}

Re: [PATCH 2/4] remote: print an error if refspec cannot be removed

[Patrick Steinhardt]

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

Re: [PATCH 2/4] remote: print an error if refspec cannot be removed

[phillip.wood123]

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);
> 
>   	}
> 

Re: [PATCH 2/4] remote: print an error if refspec cannot be removed

[Junio C Hamano]

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.

[PATCH 3/4] remote add: use strvec to store tracking branches

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Store the list of branches to track in a ’struct strvec' instead of a
'struct string_list'. This in preparation for the next commit where it
will be convenient to have them stored in a NULL terminated array. This
means that we now duplicate the strings when storing them but the
overhead is not significant.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/remote.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 4dbf7a4c506..318701496ed 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -158,7 +158,7 @@ static int add(int argc, const char **argv, const char *prefix)
 {
 	int fetch = 0, fetch_tags = TAGS_DEFAULT;
 	unsigned mirror = MIRROR_NONE;
-	struct string_list track = STRING_LIST_INIT_NODUP;
+	struct strvec track = STRVEC_INIT;
 	const char *master = NULL;
 	struct remote *remote;
 	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;
@@ -171,8 +171,8 @@ static int add(int argc, const char **argv, const char *prefix)
 			    N_("import all tags and associated objects when fetching\n"
 			       "or do not fetch any tag at all (--no-tags)"),
 			    TAGS_SET),
-		OPT_STRING_LIST('t', "track", &track, N_("branch"),
-				N_("branch(es) to track")),
+		OPT_STRVEC('t', "track", &track, N_("branch"),
+			   N_("branch(es) to track")),
 		OPT_STRING('m', "master", &master, N_("branch"), N_("master branch")),
 		OPT_CALLBACK_F(0, "mirror", &mirror, "(push|fetch)",
 			N_("set up remote as a mirror to push to or fetch from"),
@@ -210,10 +210,9 @@ static int add(int argc, const char **argv, const char *prefix)
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "remote.%s.fetch", name);
 		if (track.nr == 0)
-			string_list_append(&track, "*");
+			strvec_push(&track, "*");
 		for (i = 0; i < track.nr; i++) {
-			add_branch(buf.buf, track.items[i].string,
-				   name, mirror, &buf2);
+			add_branch(buf.buf, track.v[i], name, mirror, &buf2);
 		}
 	}
 
@@ -246,7 +245,7 @@ static int add(int argc, const char **argv, const char *prefix)
 
 	strbuf_release(&buf);
 	strbuf_release(&buf2);
-	string_list_clear(&track, 0);
+	strvec_clear(&track);
 
 	return 0;
 }
-- 
gitgitgadget

Re: [PATCH 3/4] remote add: use strvec to store tracking branches

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Store the list of branches to track in a ’struct strvec' instead of a
> 'struct string_list'. This in preparation for the next commit where it
> will be convenient to have them stored in a NULL terminated array. This
> means that we now duplicate the strings when storing them but the
> overhead is not significant.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/remote.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)

This has a slight conflict with a topic that has already graduated
but nothing serious.  If you need to reroll, you may want to base it
on a bit more recent tip of 'master', younger than bb424845 (Merge
branch 'rs/remote-leakfix', 2024-09-03).

Thanks.

Re: [PATCH 3/4] remote add: use strvec to store tracking branches

[Patrick Steinhardt]

On Wed, Sep 11, 2024 at 03:18:36PM +0000, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> Store the list of branches to track in a ’struct strvec' instead of a
> 'struct string_list'. This in preparation for the next commit where it

s/in/is &/

> will be convenient to have them stored in a NULL terminated array. This
> means that we now duplicate the strings when storing them but the
> overhead is not significant.

Yup. Micro-optimizations like this typically don't really have any real
world effect anyway.

Patrick

[PATCH 4/4] remote: check branch names

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Make sure the names passed to "git remote add -t <branch>" and "git
remote set-branches <branch>" are syntactically valid so that we do not
create invalid refspecs. This check needs to be performed before
creating the remote or modifying the existing configuration so a new
function is added rather than modifying add_branch()

Tests are added for both commands that to ensure (i) we report all the
invalid branch names passed on the command line, (ii) the branch names
are validated before modifying the configuration and (iii) wildcards
are accepted.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/remote.c  | 19 +++++++++++++++++++
 t/t5505-remote.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/builtin/remote.c b/builtin/remote.c
index 318701496ed..fd84bfbfe7a 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -132,6 +132,19 @@ static void add_branch(const char *key, const char *branchname,
 	git_config_set_multivar(key, tmp->buf, "^$", 0);
 }
 
+static int check_branch_names(const char **branches)
+{
+	int ret = 0;
+
+	for (const char **b = branches; *b; b++) {
+		if (check_refname_format(*b, REFNAME_ALLOW_ONELEVEL |
+						REFNAME_REFSPEC_PATTERN))
+			ret = error(_("invalid branch name '%s'"), *b);
+	}
+
+	return ret;
+}
+
 static const char mirror_advice[] =
 N_("--mirror is dangerous and deprecated; please\n"
    "\t use --mirror=fetch or --mirror=push instead");
@@ -203,6 +216,9 @@ static int add(int argc, const char **argv, const char *prefix)
 	if (!valid_remote_name(name))
 		die(_("'%s' is not a valid remote name"), name);
 
+	if (check_branch_names(track.v))
+		exit(128);
+
 	strbuf_addf(&buf, "remote.%s.url", name);
 	git_config_set(buf.buf, url);
 
@@ -1601,6 +1617,9 @@ static int set_remote_branches(const char *remotename, const char **branches,
 		exit(2);
 	}
 
+	if (check_branch_names(branches))
+		exit(128);
+
 	if (!add_mode && remove_all_fetch_refspecs(key.buf)) {
 		error(_("could not remove existing fetch refspec"));
 		strbuf_release(&key);
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index cfbd6139e00..709cbe65924 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -1195,6 +1195,34 @@ test_expect_success 'remote set-branches with --mirror' '
 	test_cmp expect.replace actual.replace
 '
 
+test_expect_success 'remote set-branches rejects invalid branch name' '
+	git remote add test https://git.example.com/repo &&
+	test_when_finished "git config --unset-all remote.test.fetch; \
+			    git config --unset remote.test.url" &&
+	test_must_fail git remote set-branches test "topic/*" in..valid \
+				feature "b a d" 2>err &&
+	cat >expect <<-EOF &&
+	error: invalid branch name ${SQ}in..valid${SQ}
+	error: invalid branch name ${SQ}b a d${SQ}
+	EOF
+	test_cmp expect err &&
+	git config --get-all remote.test.fetch >actual &&
+	echo "+refs/heads/*:refs/remotes/test/*" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'remote add -t rejects invalid branch name' '
+	test_must_fail git remote add test -t .bad -t "topic/*" -t in:valid \
+				 https://git.example.com/repo 2>err &&
+	cat >expect <<-EOF &&
+	error: invalid branch name ${SQ}.bad${SQ}
+	error: invalid branch name ${SQ}in:valid${SQ}
+	EOF
+	test_cmp expect err &&
+	test_expect_code 1 git config --get-regexp ^remote\\.test\\. >actual &&
+	test_must_be_empty actual
+'
+
 test_expect_success 'new remote' '
 	git remote add someremote foo &&
 	echo foo >expect &&
-- 
gitgitgadget

Re: [PATCH 4/4] remote: check branch names

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +static int check_branch_names(const char **branches)
> +{
> +	int ret = 0;
> +
> +	for (const char **b = branches; *b; b++) {
> +		if (check_refname_format(*b, REFNAME_ALLOW_ONELEVEL |
> +						REFNAME_REFSPEC_PATTERN))
> +			ret = error(_("invalid branch name '%s'"), *b);
> +	}
> +
> +	return ret;
> +}

This implementation is inconsistent with what "git branch new HEAD"
uses to check the validity of "new", which is in this call chain:

    builtin/branch.c:cmd_branch()
    -> branch.c:create_branch()
       -> branch.c:validate_new_branchname()
          -> branch.c:validate_branchname()
             -> object-name.c:strbuf_check_branch_ref()

At least, we should prepend "refs/heads/" to *b, so that we can
reject "refs/heads/HEAD".  The authoritative logic in the above
however may further evolve, and we need to make sure that these two
checks from drifting away from each other over time.  We probably
should refactor the leaf function in the above call chain so that
both places can use it (the main difference is that you allow '*' in
yours when calling check_refname_format()).

    Side note: we *should* lose "strbuf_" from its name, as it is
               not about string manipulation but the "strbuf'-ness
               of the function is merely that as the side effect of
               checking it computes a full refname and it happens to
               use strbuf as a mechanism to return it.

Something like the patch attached at the end.

>  static const char mirror_advice[] =
>  N_("--mirror is dangerous and deprecated; please\n"
>     "\t use --mirror=fetch or --mirror=push instead");
> @@ -203,6 +216,9 @@ static int add(int argc, const char **argv, const char *prefix)
>  	if (!valid_remote_name(name))
>  		die(_("'%s' is not a valid remote name"), name);
>  
> +	if (check_branch_names(track.v))
> +		exit(128);
> +

Seeing that the loop in check_branch_names() is brand new and you
could have iterated over a string-list just as easily, I somehow
doubt that step [3/4] was fully warranted.

> @@ -1601,6 +1617,9 @@ static int set_remote_branches(const char *remotename, const char **branches,
>  		exit(2);
>  	}
>  
> +	if (check_branch_names(branches))
> +		exit(128);

But here you are already passed "const char *branches[]" to this caller,
and it would be hassle to turn it into string_list, so [3/4] is fine
after all.



 object-name.h |  2 ++
 object-name.c | 23 +++++++++++++++++++++--
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git i/object-name.h w/object-name.h
index 8dba4a47a4..fa70d42044 100644
--- i/object-name.h
+++ w/object-name.h
@@ -130,4 +130,6 @@ struct object *repo_peel_to_type(struct repository *r,
 /* used when the code does not know or care what the default abbrev is */
 #define FALLBACK_DEFAULT_ABBREV 7
 
+/* Check if "name" is allowed as a branch */ 
+int valid_branch_name(const char *name, int allow_wildcard);
 #endif /* OBJECT_NAME_H */
diff --git i/object-name.c w/object-name.c
index 09c1bd93a3..e3bed5a664 100644
--- i/object-name.c
+++ w/object-name.c
@@ -1747,7 +1747,8 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
 	strbuf_add(sb, name + used, len - used);
 }
 
-int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
+static int full_ref_from_branch_name_internal(struct strbuf *sb, const char *name,
+					      int crf_flags)
 {
 	if (startup_info->have_repository)
 		strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
@@ -1766,7 +1767,25 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 	    !strcmp(sb->buf, "refs/heads/HEAD"))
 		return -1;
 
-	return check_refname_format(sb->buf, 0);
+	return check_refname_format(sb->buf, crf_flags);
+}
+
+/* NEEDSWORK: rename this to full_ref_from_branch_name */
+int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
+{
+	return full_ref_from_branch_name_internal(sb, name, 0);
+}
+
+int valid_branch_name(const char *name, int allow_wildcard)
+{
+	struct strbuf sb = STRBUF_INIT;
+	int ret;
+	int flags;
+
+	flags = allow_wildcard ? REFNAME_REFSPEC_PATTERN : 0;
+	ret = full_ref_from_branch_name_internal(&sb, name, flags);
+	strbuf_release(&sb);
+	return ret;
 }
 
 void object_context_release(struct object_context *ctx)

Re: [PATCH 4/4] remote: check branch names

[Patrick Steinhardt]

On Wed, Sep 11, 2024 at 10:03:26AM -0700, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > +static int check_branch_names(const char **branches)
> > +{
> > +	int ret = 0;
> > +
> > +	for (const char **b = branches; *b; b++) {
> > +		if (check_refname_format(*b, REFNAME_ALLOW_ONELEVEL |
> > +						REFNAME_REFSPEC_PATTERN))
> > +			ret = error(_("invalid branch name '%s'"), *b);
> > +	}
> > +
> > +	return ret;
> > +}
> 
> This implementation is inconsistent with what "git branch new HEAD"
> uses to check the validity of "new", which is in this call chain:
> 
>     builtin/branch.c:cmd_branch()
>     -> branch.c:create_branch()
>        -> branch.c:validate_new_branchname()
>           -> branch.c:validate_branchname()
>              -> object-name.c:strbuf_check_branch_ref()
> 
> At least, we should prepend "refs/heads/" to *b, so that we can
> reject "refs/heads/HEAD".  The authoritative logic in the above
> however may further evolve, and we need to make sure that these two
> checks from drifting away from each other over time.  We probably
> should refactor the leaf function in the above call chain so that
> both places can use it (the main difference is that you allow '*' in
> yours when calling check_refname_format()).
> 
>     Side note: we *should* lose "strbuf_" from its name, as it is
>                not about string manipulation but the "strbuf'-ness
>                of the function is merely that as the side effect of
>                checking it computes a full refname and it happens to
>                use strbuf as a mechanism to return it.
> 
> Something like the patch attached at the end.

Agreed. It's also kind of curious that the function lives in
"object-name.c" and not in "refs.c".

Patrick

Re: [PATCH 4/4] remote: check branch names

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Agreed. It's also kind of curious that the function lives in
> "object-name.c" and not in "refs.c".

Because the helper groks things like "-" (aka "@{-1}"), it does a
bit more than "is this a reasonable name for a ref" and "please give
me the current value of this ref".  Also "refs/remotes/origin/HEAD"
may be valid as a refname, but forbidding "refs/heads/HEAD" is done
conceptually one level closer to the end-users.  Eventually, I think
it should move next to branch.c:validate_branchname() as a common
helper between "git branch" and "git remote" (possibly also with
"git switch/checkout", if they need to do validation themselves, but
I suspect they just call into branch.c at a bit higher "here is a
name, create it and you are free to complain---I do not care about
the details of why you decide the name is bad" interface).

Thanks.

Re: [PATCH 4/4] remote: check branch names

[phillip.wood123]

On 11/09/2024 18:03, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> The authoritative logic in the above
> however may further evolve, and we need to make sure that these two
> checks from drifting away from each other over time.  We probably
> should refactor the leaf function in the above call chain so that
> both places can use it (the main difference is that you allow '*' in
> yours when calling check_refname_format()).
> 
>      Side note: we *should* lose "strbuf_" from its name, as it is
>                 not about string manipulation but the "strbuf'-ness
>                 of the function is merely that as the side effect of
>                 checking it computes a full refname and it happens to
>                 use strbuf as a mechanism to return it.
> 
> Something like the patch attached at the end.

Thanks for the patch, I'll re-roll based on that. I wonder if we really 
want to support "@{-N}" when setting remote tracking branches though - 
should we be using INTERPRET_BRANCH_REMOTE instead when calling 
strbuf_branchname()?

Best Wishes

Phillip

>>   static const char mirror_advice[] =
>>   N_("--mirror is dangerous and deprecated; please\n"
>>      "\t use --mirror=fetch or --mirror=push instead");
>> @@ -203,6 +216,9 @@ static int add(int argc, const char **argv, const char *prefix)
>>   	if (!valid_remote_name(name))
>>   		die(_("'%s' is not a valid remote name"), name);
>>   
>> +	if (check_branch_names(track.v))
>> +		exit(128);
>> +
> 
> Seeing that the loop in check_branch_names() is brand new and you
> could have iterated over a string-list just as easily, I somehow
> doubt that step [3/4] was fully warranted.
> 
>> @@ -1601,6 +1617,9 @@ static int set_remote_branches(const char *remotename, const char **branches,
>>   		exit(2);
>>   	}
>>   
>> +	if (check_branch_names(branches))
>> +		exit(128);
> 
> But here you are already passed "const char *branches[]" to this caller,
> and it would be hassle to turn it into string_list, so [3/4] is fine
> after all.
> 
> 
> 
>   object-name.h |  2 ++
>   object-name.c | 23 +++++++++++++++++++++--
>   2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git i/object-name.h w/object-name.h
> index 8dba4a47a4..fa70d42044 100644
> --- i/object-name.h
> +++ w/object-name.h
> @@ -130,4 +130,6 @@ struct object *repo_peel_to_type(struct repository *r,
>   /* used when the code does not know or care what the default abbrev is */
>   #define FALLBACK_DEFAULT_ABBREV 7
>   
> +/* Check if "name" is allowed as a branch */
> +int valid_branch_name(const char *name, int allow_wildcard);
>   #endif /* OBJECT_NAME_H */
> diff --git i/object-name.c w/object-name.c
> index 09c1bd93a3..e3bed5a664 100644
> --- i/object-name.c
> +++ w/object-name.c
> @@ -1747,7 +1747,8 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
>   	strbuf_add(sb, name + used, len - used);
>   }
>   
> -int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
> +static int full_ref_from_branch_name_internal(struct strbuf *sb, const char *name,
> +					      int crf_flags)
>   {
>   	if (startup_info->have_repository)
>   		strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
> @@ -1766,7 +1767,25 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
>   	    !strcmp(sb->buf, "refs/heads/HEAD"))
>   		return -1;
>   
> -	return check_refname_format(sb->buf, 0);
> +	return check_refname_format(sb->buf, crf_flags);
> +}
> +
> +/* NEEDSWORK: rename this to full_ref_from_branch_name */
> +int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
> +{
> +	return full_ref_from_branch_name_internal(sb, name, 0);
> +}
> +
> +int valid_branch_name(const char *name, int allow_wildcard)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	int ret;
> +	int flags;
> +
> +	flags = allow_wildcard ? REFNAME_REFSPEC_PATTERN : 0;
> +	ret = full_ref_from_branch_name_internal(&sb, name, flags);
> +	strbuf_release(&sb);
> +	return ret;
>   }
>   
>   void object_context_release(struct object_context *ctx)

Re: [PATCH 4/4] remote: check branch names

[Junio C Hamano]

phillip.wood123@gmail.com writes:

> Thanks for the patch, I'll re-roll based on that. I wonder if we
> really want to support "@{-N}" when setting remote tracking branches
> though - should we be using INTERPRET_BRANCH_REMOTE instead when
> calling strbuf_branchname()?

Perhaps.  Users try to use "-" in surprising places, though ;-)

Re: [PATCH 4/4] remote: check branch names

[phillip.wood123]

On 13/09/2024 18:49, Junio C Hamano wrote:
> phillip.wood123@gmail.com writes:
> 
>> Thanks for the patch, I'll re-roll based on that. I wonder if we
>> really want to support "@{-N}" when setting remote tracking branches
>> though - should we be using INTERPRET_BRANCH_REMOTE instead when
>> calling strbuf_branchname()?
> 
> Perhaps.  Users try to use "-" in surprising places, though ;-)

strbuf_check_branch_ref() already rejects "-".

INTERPRET_BRANCH_REMOTE supports @{upstream} which might be useful but 
then we will need to check it refers to the correct remote and expand it 
when setting the fetch refspec so a boolean function to check if a name 
is acceptable is insufficient. Given that "git remote set-branches" has 
only ever supported "real" branch names and patterns on the command line 
and no-one has complained I wonder if we're better off doing something like

	if (strbuf_check_branch_ref(&buf, branch_name) ||
	    strcmp(buf.buf + 11, branch_name))
		error(_("invalid branch name '%s'", branch_name));

where the "buf.buf + 11" skips "refs/heads/"

Best Wishes

Phillip

Re: [PATCH 4/4] remote: check branch names

[Junio C Hamano]

phillip.wood123@gmail.com writes:

> ... Given that "git remote
> set-branches" has only ever supported "real" branch names and patterns
> on the command line and no-one has complained I wonder if we're better
> off doing something like
>
> 	if (strbuf_check_branch_ref(&buf, branch_name) ||
> 	    strcmp(buf.buf + 11, branch_name))
> 		error(_("invalid branch name '%s'", branch_name));
>
> where the "buf.buf + 11" skips "refs/heads/"

Yeah, replacing +11 with skip_prefix() or something for readability,
such a check might be good enough in pracrice.

Thanks.