From d95a07a22aadccd0c0e0e63bbb98b3a4306545d2 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Fri, 6 Sep 2024 10:23:58 +0100 Subject: [PATCH 1/4] remote: fix set-branches when no branches are set 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 Signed-off-by: Phillip Wood --- 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 d1f9292ed2b292..794396ba02f225 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 08424e878e104c..cfbd6139e00b05 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 ../actual.respect-ffonly + sort ../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' ' From a8dfe403d0683aec4265bf920921e45d5b59cec3 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Tue, 10 Sep 2024 10:19:28 +0100 Subject: [PATCH 2/4] remote: print an error if refspec cannot be removed 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 --- builtin/remote.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/remote.c b/builtin/remote.c index 794396ba02f225..4dbf7a4c506b68 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; } From f30c77bc36072df57662cac0cb7bf1bbea378062 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Tue, 10 Sep 2024 14:22:11 +0100 Subject: [PATCH 3/4] remote add: use strvec to store tracking branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- builtin/remote.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/builtin/remote.c b/builtin/remote.c index 4dbf7a4c506b68..318701496ed438 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; } From dba31245607f85c48947da60fe0955a6ed3e2c43 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Mon, 9 Sep 2024 15:04:35 +0100 Subject: [PATCH 4/4] remote: check branch names Make sure the names passed to "git remote add -t " and "git remote set-branches " 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 --- 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 318701496ed438..fd84bfbfe7ab03 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 cfbd6139e00b05..709cbe65924467 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 &&