remote: branch setting fixes #1789
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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>
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>
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>
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>
|
/submit |
|
Submitted as pull.1789.git.1726067917.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -132,6 +132,19 @@ static void add_branch(const char *key, const char *branchname, | |||
| git_config_set_multivar(key, tmp->buf, "^$", 0); | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"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)There was a problem hiding this comment.
On the Git mailing list, Patrick Steinhardt wrote (reply to this):
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".
PatrickThere was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
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.
There was a problem hiding this comment.
On the Git mailing list, phillip.wood123@gmail.com wrote (reply to this):
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)There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
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 ;-)
There was a problem hiding this comment.
On the Git mailing list, phillip.wood123@gmail.com wrote (reply to this):
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
PhillipThere was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
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.
| @@ -158,7 +158,7 @@ static int add(int argc, const char **argv, const char *prefix) | |||
| { | |||
There was a problem hiding this comment.
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>
>
> 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.
| @@ -1567,8 +1567,12 @@ static int update(int argc, const char **argv, const char *prefix) | |||
|
|
|||
There was a problem hiding this comment.
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.
| @@ -1599,6 +1603,7 @@ static int set_remote_branches(const char *remotename, const char **branches, | |||
| } | |||
There was a problem hiding this comment.
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>
>
> 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);
}
There was a problem hiding this comment.
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" <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.
PatrickThere was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
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();
}
There was a problem hiding this comment.
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 <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!
PatrickThere was a problem hiding this comment.
On the Git mailing list, phillip.wood123@gmail.com wrote (reply to this):
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);
> > }
> There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
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.
| @@ -1567,8 +1567,12 @@ static int update(int argc, const char **argv, const char *prefix) | |||
|
|
|||
There was a problem hiding this comment.
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
|
User |
| @@ -158,7 +158,7 @@ static int add(int argc, const char **argv, const char *prefix) | |||
| { | |||
There was a problem hiding this comment.
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 <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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Cc: Han Jiang jhcarl0814@gmail.com
cc: Patrick Steinhardt ps@pks.im