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

Jump to bottom

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 3 commits
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
22 changes: 13 additions & 9 deletions builtin/remote.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ static int add(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" <[email protected]> writes:

> From: Phillip Wood <[email protected]>
>
> 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 <[email protected]>
> ---
>  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.

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:36PM +0000, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <[email protected]>
> 
> 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

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;
Expand All @@ -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"),
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -1567,8 +1566,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" <[email protected]> writes:

> 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.
> 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 <[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)
{
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 Expand Up @@ -1599,6 +1602,7 @@ static int set_remote_branches(const char *remotename, const char **branches,
}
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" <[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);

 	}

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

Patrick

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

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

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 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!

Patrick

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

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

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