{"thread":{"id":"62098","subject":"[PATCH 1/4] remote: fix set-branches when no branches are set","startedAt":"2024-09-11T15:18:40Z","lastAt":"2024-09-18T20:24:09Z","messageCount":22,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano","Patrick Steinhardt","phillip.wood123@gmail.com"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"502623","messageId":"d95a07a22aadccd0c0e0e63bbb98b3a4306545d2.1726067917.git.gitgitgadget@gmail.com","threadId":"62098","inReplyTo":"pull.1789.git.1726067917.gitgitgadget@gmail.com","subject":"[PATCH 1/4] remote: fix set-branches when no branches are set","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-11T15:18:34Z","receivedAt":"2024-09-11T15:18:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nTo replace the list of branches to be fetched \"git remote set-branches\"\nfirst removes the fetch refspecs for the remote and then creates a new\nset of fetch refspecs based and the branches passed on the commandline.\nWhen deleting the existing refspecs git_config_set_multivar_gently()\nwill return a non-zero result if there was nothing to delete.\nUnfortunately the calling code treats that as an error and bails out\nrather than setting up the new branches. Fix this by not treating a\nreturn value of CONFIG_NOTHING_SET as an error.\n\nReported-by: Han Jiang <jhcarl0814@gmail.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n builtin/remote.c  |  8 ++++++--\n t/t5505-remote.sh | 14 +++++++++++---\n 2 files changed, 17 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex d1f9292ed2b..794396ba02f 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1567,8 +1567,12 @@ static int update(int argc, const char **argv, const char *prefix)\n \n static int remove_all_fetch_refspecs(const char *key)\n {\n-\treturn git_config_set_multivar_gently(key, NULL, NULL,\n-\t\t\t\t\t      CONFIG_FLAGS_MULTI_REPLACE);\n+\tint res = git_config_set_multivar_gently(key, NULL, NULL,\n+\t\t\t\t\t\t CONFIG_FLAGS_MULTI_REPLACE);\n+\tif (res == CONFIG_NOTHING_SET)\n+\t\tres = 0;\n+\n+\treturn res;\n }\n \n static void add_branches(struct remote *remote, const char **branches,\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 08424e878e1..cfbd6139e00 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -1131,7 +1131,9 @@ test_expect_success 'remote set-branches' '\n \t+refs/heads/next:refs/remotes/scratch/next\n \t+refs/heads/seen:refs/remotes/scratch/seen\n \tEOF\n-\n+\tcat  <<-\\EOF >expect.replace-missing &&\n+\t+refs/heads/topic:refs/remotes/scratch/topic\n+\tEOF\n \tgit clone .git/ setbranches &&\n \t(\n \t\tcd setbranches &&\n@@ -1161,14 +1163,20 @@ test_expect_success 'remote set-branches' '\n \n \t\tgit remote set-branches --add scratch seen &&\n \t\tgit config --get-all remote.scratch.fetch >config-result &&\n-\t\tsort <config-result >../actual.respect-ffonly\n+\t\tsort <config-result >../actual.respect-ffonly &&\n+\n+\t\tgit config --unset-all remote.scratch.fetch &&\n+\t\tgit remote set-branches scratch topic &&\n+\t\tgit config --get-all remote.scratch.fetch \\\n+\t\t\t\t\t>../actual.replace-missing\n \t) &&\n \ttest_cmp expect.initial actual.initial &&\n \ttest_cmp expect.add actual.add &&\n \ttest_cmp expect.replace actual.replace &&\n \ttest_cmp expect.add-two actual.add-two &&\n \ttest_cmp expect.setup-ffonly actual.setup-ffonly &&\n-\ttest_cmp expect.respect-ffonly actual.respect-ffonly\n+\ttest_cmp expect.respect-ffonly actual.respect-ffonly &&\n+\ttest_cmp expect.replace-missing actual.replace-missing\n '\n \n test_expect_success 'remote set-branches with --mirror' '\n-- \ngitgitgadget\n\n"},{"id":"502624","messageId":"pull.1789.git.1726067917.gitgitgadget@gmail.com","threadId":"62098","inReplyTo":null,"subject":"[PATCH 0/4] remote: branch setting fixes","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-11T15:18:33Z","receivedAt":"2024-09-11T15:18:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"This series fixes some rough edges when setting remote tracking branches\nwith \"git remote set-branches\" or \"git remote add -t\". Han Jiang reported\nthat if there is no fetch refspec set for a remote then \"git remote\nset-branches\" silently fails. While investigating that I noticed that the\ncode does not check that the branch names are syntactically valid refnames\nand so can create invalid refspecs. This series fixes both issues.\n\nPhillip Wood (4):\n  remote: fix set-branches when no branches are set\n  remote: print an error if refspec cannot be removed\n  remote add: use strvec to store tracking branches\n  remote: check branch names\n\n builtin/remote.c  | 41 ++++++++++++++++++++++++++++++++---------\n t/t5505-remote.sh | 42 +++++++++++++++++++++++++++++++++++++++---\n 2 files changed, 71 insertions(+), 12 deletions(-)\n\n\nbase-commit: 4590f2e9412378c61eac95966709c78766d326ba\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1789%2Fphillipwood%2Fremote-set-branches-missing-fetch-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1789/phillipwood/remote-set-branches-missing-fetch-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1789\n-- \ngitgitgadget\n"},{"id":"502625","messageId":"a8dfe403d0683aec4265bf920921e45d5b59cec3.1726067917.git.gitgitgadget@gmail.com","threadId":"62098","inReplyTo":"pull.1789.git.1726067917.gitgitgadget@gmail.com","subject":"[PATCH 2/4] remote: print an error if refspec cannot be removed","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-11T15:18:35Z","receivedAt":"2024-09-11T15:18:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIf the existing fetch refspecs cannot be removed when replacing the set\nof branches to fetch with \"git remote set-branches\" the command silently\nfails. Add an error message to tell the user what when wrong.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n builtin/remote.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 794396ba02f..4dbf7a4c506 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1603,6 +1603,7 @@ static int set_remote_branches(const char *remotename, const char **branches,\n \t}\n \n \tif (!add_mode && remove_all_fetch_refspecs(key.buf)) {\n+\t\terror(_(\"could not remove existing fetch refspec\"));\n \t\tstrbuf_release(&key);\n \t\treturn 1;\n \t}\n-- \ngitgitgadget\n\n"},{"id":"502626","messageId":"f30c77bc36072df57662cac0cb7bf1bbea378062.1726067917.git.gitgitgadget@gmail.com","threadId":"62098","inReplyTo":"pull.1789.git.1726067917.gitgitgadget@gmail.com","subject":"[PATCH 3/4] remote add: use strvec to store tracking branches","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-11T15:18:36Z","receivedAt":"2024-09-11T15:18:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nStore the list of branches to track in a ’struct strvec' instead of a\n'struct string_list'. This in preparation for the next commit where it\nwill be convenient to have them stored in a NULL terminated array. This\nmeans that we now duplicate the strings when storing them but the\noverhead is not significant.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n builtin/remote.c | 13 ++++++-------\n 1 file changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 4dbf7a4c506..318701496ed 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -158,7 +158,7 @@ static int add(int argc, const char **argv, const char *prefix)\n {\n \tint fetch = 0, fetch_tags = TAGS_DEFAULT;\n \tunsigned mirror = MIRROR_NONE;\n-\tstruct string_list track = STRING_LIST_INIT_NODUP;\n+\tstruct strvec track = STRVEC_INIT;\n \tconst char *master = NULL;\n \tstruct remote *remote;\n \tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;\n@@ -171,8 +171,8 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\t\t    N_(\"import all tags and associated objects when fetching\\n\"\n \t\t\t       \"or do not fetch any tag at all (--no-tags)\"),\n \t\t\t    TAGS_SET),\n-\t\tOPT_STRING_LIST('t', \"track\", &track, N_(\"branch\"),\n-\t\t\t\tN_(\"branch(es) to track\")),\n+\t\tOPT_STRVEC('t', \"track\", &track, N_(\"branch\"),\n+\t\t\t   N_(\"branch(es) to track\")),\n \t\tOPT_STRING('m', \"master\", &master, N_(\"branch\"), N_(\"master branch\")),\n \t\tOPT_CALLBACK_F(0, \"mirror\", &mirror, \"(push|fetch)\",\n \t\t\tN_(\"set up remote as a mirror to push to or fetch from\"),\n@@ -210,10 +210,9 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"remote.%s.fetch\", name);\n \t\tif (track.nr == 0)\n-\t\t\tstring_list_append(&track, \"*\");\n+\t\t\tstrvec_push(&track, \"*\");\n \t\tfor (i = 0; i < track.nr; i++) {\n-\t\t\tadd_branch(buf.buf, track.items[i].string,\n-\t\t\t\t   name, mirror, &buf2);\n+\t\t\tadd_branch(buf.buf, track.v[i], name, mirror, &buf2);\n \t\t}\n \t}\n \n@@ -246,7 +245,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \n \tstrbuf_release(&buf);\n \tstrbuf_release(&buf2);\n-\tstring_list_clear(&track, 0);\n+\tstrvec_clear(&track);\n \n \treturn 0;\n }\n-- \ngitgitgadget\n\n"},{"id":"502627","messageId":"dba31245607f85c48947da60fe0955a6ed3e2c43.1726067917.git.gitgitgadget@gmail.com","threadId":"62098","inReplyTo":"pull.1789.git.1726067917.gitgitgadget@gmail.com","subject":"[PATCH 4/4] remote: check branch names","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-11T15:18:37Z","receivedAt":"2024-09-11T15:18:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nMake sure the names passed to \"git remote add -t <branch>\" and \"git\nremote set-branches <branch>\" are syntactically valid so that we do not\ncreate invalid refspecs. This check needs to be performed before\ncreating the remote or modifying the existing configuration so a new\nfunction is added rather than modifying add_branch()\n\nTests are added for both commands that to ensure (i) we report all the\ninvalid branch names passed on the command line, (ii) the branch names\nare validated before modifying the configuration and (iii) wildcards\nare accepted.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n builtin/remote.c  | 19 +++++++++++++++++++\n t/t5505-remote.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 47 insertions(+)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 318701496ed..fd84bfbfe7a 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -132,6 +132,19 @@ static void add_branch(const char *key, const char *branchname,\n \tgit_config_set_multivar(key, tmp->buf, \"^$\", 0);\n }\n \n+static int check_branch_names(const char **branches)\n+{\n+\tint ret = 0;\n+\n+\tfor (const char **b = branches; *b; b++) {\n+\t\tif (check_refname_format(*b, REFNAME_ALLOW_ONELEVEL |\n+\t\t\t\t\t\tREFNAME_REFSPEC_PATTERN))\n+\t\t\tret = error(_(\"invalid branch name '%s'\"), *b);\n+\t}\n+\n+\treturn ret;\n+}\n+\n static const char mirror_advice[] =\n N_(\"--mirror is dangerous and deprecated; please\\n\"\n    \"\\t use --mirror=fetch or --mirror=push instead\");\n@@ -203,6 +216,9 @@ static int add(int argc, const char **argv, const char *prefix)\n \tif (!valid_remote_name(name))\n \t\tdie(_(\"'%s' is not a valid remote name\"), name);\n \n+\tif (check_branch_names(track.v))\n+\t\texit(128);\n+\n \tstrbuf_addf(&buf, \"remote.%s.url\", name);\n \tgit_config_set(buf.buf, url);\n \n@@ -1601,6 +1617,9 @@ static int set_remote_branches(const char *remotename, const char **branches,\n \t\texit(2);\n \t}\n \n+\tif (check_branch_names(branches))\n+\t\texit(128);\n+\n \tif (!add_mode && remove_all_fetch_refspecs(key.buf)) {\n \t\terror(_(\"could not remove existing fetch refspec\"));\n \t\tstrbuf_release(&key);\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex cfbd6139e00..709cbe65924 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -1195,6 +1195,34 @@ test_expect_success 'remote set-branches with --mirror' '\n \ttest_cmp expect.replace actual.replace\n '\n \n+test_expect_success 'remote set-branches rejects invalid branch name' '\n+\tgit remote add test https://git.example.com/repo &&\n+\ttest_when_finished \"git config --unset-all remote.test.fetch; \\\n+\t\t\t    git config --unset remote.test.url\" &&\n+\ttest_must_fail git remote set-branches test \"topic/*\" in..valid \\\n+\t\t\t\tfeature \"b a d\" 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: invalid branch name ${SQ}in..valid${SQ}\n+\terror: invalid branch name ${SQ}b a d${SQ}\n+\tEOF\n+\ttest_cmp expect err &&\n+\tgit config --get-all remote.test.fetch >actual &&\n+\techo \"+refs/heads/*:refs/remotes/test/*\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'remote add -t rejects invalid branch name' '\n+\ttest_must_fail git remote add test -t .bad -t \"topic/*\" -t in:valid \\\n+\t\t\t\t https://git.example.com/repo 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: invalid branch name ${SQ}.bad${SQ}\n+\terror: invalid branch name ${SQ}in:valid${SQ}\n+\tEOF\n+\ttest_cmp expect err &&\n+\ttest_expect_code 1 git config --get-regexp ^remote\\\\.test\\\\. >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_expect_success 'new remote' '\n \tgit remote add someremote foo &&\n \techo foo >expect &&\n-- \ngitgitgadget\n"},{"id":"502634","messageId":"xmqqfrq686n5.fsf@gitster.g","threadId":"62098","inReplyTo":"dba31245607f85c48947da60fe0955a6ed3e2c43.1726067917.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] remote: check branch names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-11T17:03:26Z","receivedAt":"2024-09-11T17:03:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> +static int check_branch_names(const char **branches)\n> +{\n> +\tint ret = 0;\n> +\n> +\tfor (const char **b = branches; *b; b++) {\n> +\t\tif (check_refname_format(*b, REFNAME_ALLOW_ONELEVEL |\n> +\t\t\t\t\t\tREFNAME_REFSPEC_PATTERN))\n> +\t\t\tret = error(_(\"invalid branch name '%s'\"), *b);\n> +\t}\n> +\n> +\treturn ret;\n> +}\n\nThis implementation is inconsistent with what \"git branch new HEAD\"\nuses to check the validity of \"new\", which is in this call chain:\n\n    builtin/branch.c:cmd_branch()\n    -> branch.c:create_branch()\n       -> branch.c:validate_new_branchname()\n          -> branch.c:validate_branchname()\n             -> object-name.c:strbuf_check_branch_ref()\n\nAt least, we should prepend \"refs/heads/\" to *b, so that we can\nreject \"refs/heads/HEAD\".  The authoritative logic in the above\nhowever may further evolve, and we need to make sure that these two\nchecks from drifting away from each other over time.  We probably\nshould refactor the leaf function in the above call chain so that\nboth places can use it (the main difference is that you allow '*' in\nyours when calling check_refname_format()).\n\n    Side note: we *should* lose \"strbuf_\" from its name, as it is\n               not about string manipulation but the \"strbuf'-ness\n               of the function is merely that as the side effect of\n               checking it computes a full refname and it happens to\n               use strbuf as a mechanism to return it.\n\nSomething like the patch attached at the end.\n\n>  static const char mirror_advice[] =\n>  N_(\"--mirror is dangerous and deprecated; please\\n\"\n>     \"\\t use --mirror=fetch or --mirror=push instead\");\n> @@ -203,6 +216,9 @@ static int add(int argc, const char **argv, const char *prefix)\n>  \tif (!valid_remote_name(name))\n>  \t\tdie(_(\"'%s' is not a valid remote name\"), name);\n>  \n> +\tif (check_branch_names(track.v))\n> +\t\texit(128);\n> +\n\nSeeing that the loop in check_branch_names() is brand new and you\ncould have iterated over a string-list just as easily, I somehow\ndoubt that step [3/4] was fully warranted.\n\n> @@ -1601,6 +1617,9 @@ static int set_remote_branches(const char *remotename, const char **branches,\n>  \t\texit(2);\n>  \t}\n>  \n> +\tif (check_branch_names(branches))\n> +\t\texit(128);\n\nBut here you are already passed \"const char *branches[]\" to this caller,\nand it would be hassle to turn it into string_list, so [3/4] is fine\nafter all.\n\n\n\n object-name.h |  2 ++\n object-name.c | 23 +++++++++++++++++++++--\n 2 files changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git i/object-name.h w/object-name.h\nindex 8dba4a47a4..fa70d42044 100644\n--- i/object-name.h\n+++ w/object-name.h\n@@ -130,4 +130,6 @@ struct object *repo_peel_to_type(struct repository *r,\n /* used when the code does not know or care what the default abbrev is */\n #define FALLBACK_DEFAULT_ABBREV 7\n \n+/* Check if \"name\" is allowed as a branch */ \n+int valid_branch_name(const char *name, int allow_wildcard);\n #endif /* OBJECT_NAME_H */\ndiff --git i/object-name.c w/object-name.c\nindex 09c1bd93a3..e3bed5a664 100644\n--- i/object-name.c\n+++ w/object-name.c\n@@ -1747,7 +1747,8 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)\n \tstrbuf_add(sb, name + used, len - used);\n }\n \n-int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n+static int full_ref_from_branch_name_internal(struct strbuf *sb, const char *name,\n+\t\t\t\t\t      int crf_flags)\n {\n \tif (startup_info->have_repository)\n \t\tstrbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);\n@@ -1766,7 +1767,25 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n \t    !strcmp(sb->buf, \"refs/heads/HEAD\"))\n \t\treturn -1;\n \n-\treturn check_refname_format(sb->buf, 0);\n+\treturn check_refname_format(sb->buf, crf_flags);\n+}\n+\n+/* NEEDSWORK: rename this to full_ref_from_branch_name */\n+int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n+{\n+\treturn full_ref_from_branch_name_internal(sb, name, 0);\n+}\n+\n+int valid_branch_name(const char *name, int allow_wildcard)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint ret;\n+\tint flags;\n+\n+\tflags = allow_wildcard ? REFNAME_REFSPEC_PATTERN : 0;\n+\tret = full_ref_from_branch_name_internal(&sb, name, flags);\n+\tstrbuf_release(&sb);\n+\treturn ret;\n }\n \n void object_context_release(struct object_context *ctx)\n"},{"id":"502635","messageId":"xmqq8qvy86kd.fsf@gitster.g","threadId":"62098","inReplyTo":"f30c77bc36072df57662cac0cb7bf1bbea378062.1726067917.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] remote add: use strvec to store tracking branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-11T17:05:06Z","receivedAt":"2024-09-11T17:05:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> Store the list of branches to track in a ’struct strvec' instead of a\n> 'struct string_list'. This in preparation for the next commit where it\n> will be convenient to have them stored in a NULL terminated array. This\n> means that we now duplicate the strings when storing them but the\n> overhead is not significant.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  builtin/remote.c | 13 ++++++-------\n>  1 file changed, 6 insertions(+), 7 deletions(-)\n\nThis has a slight conflict with a topic that has already graduated\nbut nothing serious.  If you need to reroll, you may want to base it\non a bit more recent tip of 'master', younger than bb424845 (Merge\nbranch 'rs/remote-leakfix', 2024-09-03).\n\nThanks.\n\n\n"},{"id":"502639","messageId":"xmqq4j6l7wcy.fsf@gitster.g","threadId":"62098","inReplyTo":"d95a07a22aadccd0c0e0e63bbb98b3a4306545d2.1726067917.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] remote: fix set-branches when no branches are set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-11T20:45:33Z","receivedAt":"2024-09-11T20:45:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> To replace the list of branches to be fetched \"git remote set-branches\"\n> first removes the fetch refspecs for the remote and then creates a new\n> set of fetch refspecs based and the branches passed on the commandline.\n> When deleting the existing refspecs git_config_set_multivar_gently()\n> will return a non-zero result if there was nothing to delete.\n> Unfortunately the calling code treats that as an error and bails out\n> rather than setting up the new branches. Fix this by not treating a\n> return value of CONFIG_NOTHING_SET as an error.\n\nMakes sense.\n\n> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\n> index 08424e878e1..cfbd6139e00 100755\n> --- a/t/t5505-remote.sh\n> +++ b/t/t5505-remote.sh\n> @@ -1131,7 +1131,9 @@ test_expect_success 'remote set-branches' '\n>  \t+refs/heads/next:refs/remotes/scratch/next\n>  \t+refs/heads/seen:refs/remotes/scratch/seen\n>  \tEOF\n> -\n> +\tcat  <<-\\EOF >expect.replace-missing &&\n> +\t+refs/heads/topic:refs/remotes/scratch/topic\n> +\tEOF\n>  \tgit clone .git/ setbranches &&\n>  \t(\n>  \t\tcd setbranches &&\n> @@ -1161,14 +1163,20 @@ test_expect_success 'remote set-branches' '\n>  \n>  \t\tgit remote set-branches --add scratch seen &&\n>  \t\tgit config --get-all remote.scratch.fetch >config-result &&\n> -\t\tsort <config-result >../actual.respect-ffonly\n> +\t\tsort <config-result >../actual.respect-ffonly &&\n> +\n> +\t\tgit config --unset-all remote.scratch.fetch &&\n\nOK, so we get rid of all \"fetch\" refspec elements and make sure we\ncan ...\n\n> +\t\tgit remote set-branches scratch topic &&\n\n... set a single new one like this ...\n\n> +\t\tgit config --get-all remote.scratch.fetch \\\n> +\t\t\t\t\t>../actual.replace-missing\n\nand we expect the mapping to appear in the output.  For\nmaintainability, it would be better to also sort this one to mimick\nthe other one that contain multiple entries in the output, but\nbecause we expect only one entry to be in the output, not sorting is\nOK for now.\n\nLooks good.  Thanks.\n"},{"id":"502640","messageId":"xmqqseu56hhb.fsf@gitster.g","threadId":"62098","inReplyTo":"a8dfe403d0683aec4265bf920921e45d5b59cec3.1726067917.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] remote: print an error if refspec cannot be removed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-11T20:52:16Z","receivedAt":"2024-09-11T20:52:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> If the existing fetch refspecs cannot be removed when replacing the set\n> of branches to fetch with \"git remote set-branches\" the command silently\n> fails. Add an error message to tell the user what when wrong.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  builtin/remote.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index 794396ba02f..4dbf7a4c506 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -1603,6 +1603,7 @@ static int set_remote_branches(const char *remotename, const char **branches,\n>  \t}\n>  \n>  \tif (!add_mode && remove_all_fetch_refspecs(key.buf)) {\n> +\t\terror(_(\"could not remove existing fetch refspec\"));\n>  \t\tstrbuf_release(&key);\n>  \t\treturn 1;\n>  \t}\n\nIt is a minor point, but would it help to say what we tried to\nremove (e.g. \"from remote X\") or is it too obvious to the end user\nin the context they get this error?\n\nThe reason why I had the above question was because inserting error()\nbefore strbuf_release(&key) looked curious and I initially suspected\nthat it was because key was used in the error message somehow, but it\nturns out that is not the case at all.\n\nIOW, I would have expected something more like this:\n\n \tif (!add_mode && remove_all_fetch_refspecs(key.buf)) {\n \t\tstrbuf_release(&key);\n+\t\treturn error(_(\"failed to remove fetch refspec from '%s'\"),\n+\t\t\t\tremotename);\n\n \t}\n\n"},{"id":"502662","messageId":"ZuK8xgBroR0WbmzX@pks.im","threadId":"62098","inReplyTo":"d95a07a22aadccd0c0e0e63bbb98b3a4306545d2.1726067917.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] remote: fix set-branches when no branches are set","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-12T10:04:54Z","receivedAt":"2024-09-12T10:04:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Sep 11, 2024 at 03:18:34PM +0000, Phillip Wood via GitGitGadget wrote:\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n> \n> To replace the list of branches to be fetched \"git remote set-branches\"\n> first removes the fetch refspecs for the remote and then creates a new\n> set of fetch refspecs based and the branches passed on the commandline.\n\ns/and/on/\n\n> When deleting the existing refspecs git_config_set_multivar_gently()\n> will return a non-zero result if there was nothing to delete.\n> Unfortunately the calling code treats that as an error and bails out\n> rather than setting up the new branches. Fix this by not treating a\n> return value of CONFIG_NOTHING_SET as an error.\n> \n> Reported-by: Han Jiang <jhcarl0814@gmail.com>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  builtin/remote.c  |  8 ++++++--\n>  t/t5505-remote.sh | 14 +++++++++++---\n>  2 files changed, 17 insertions(+), 5 deletions(-)\n> \n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index d1f9292ed2b..794396ba02f 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -1567,8 +1567,12 @@ static int update(int argc, const char **argv, const char *prefix)\n>  \n>  static int remove_all_fetch_refspecs(const char *key)\n>  {\n> -\treturn git_config_set_multivar_gently(key, NULL, NULL,\n> -\t\t\t\t\t      CONFIG_FLAGS_MULTI_REPLACE);\n> +\tint res = git_config_set_multivar_gently(key, NULL, NULL,\n> +\t\t\t\t\t\t CONFIG_FLAGS_MULTI_REPLACE);\n> +\tif (res == CONFIG_NOTHING_SET)\n> +\t\tres = 0;\n> +\n> +\treturn res;\n>  }\n\nMakes sense.\n\n>  static void add_branches(struct remote *remote, const char **branches,\n> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\n> index 08424e878e1..cfbd6139e00 100755\n> --- a/t/t5505-remote.sh\n> +++ b/t/t5505-remote.sh\n> @@ -1131,7 +1131,9 @@ test_expect_success 'remote set-branches' '\n>  \t+refs/heads/next:refs/remotes/scratch/next\n>  \t+refs/heads/seen:refs/remotes/scratch/seen\n>  \tEOF\n> -\n> +\tcat  <<-\\EOF >expect.replace-missing &&\n\ns/  / /\n\nAlso, the redirect typically comes before the heredoc marker.\n\n> +\t+refs/heads/topic:refs/remotes/scratch/topic\n> +\tEOF\n>  \tgit clone .git/ setbranches &&\n>  \t(\n>  \t\tcd setbranches &&\n> @@ -1161,14 +1163,20 @@ test_expect_success 'remote set-branches' '\n>  \n>  \t\tgit remote set-branches --add scratch seen &&\n>  \t\tgit config --get-all remote.scratch.fetch >config-result &&\n> -\t\tsort <config-result >../actual.respect-ffonly\n> +\t\tsort <config-result >../actual.respect-ffonly &&\n> +\n> +\t\tgit config --unset-all remote.scratch.fetch &&\n> +\t\tgit remote set-branches scratch topic &&\n> +\t\tgit config --get-all remote.scratch.fetch \\\n> +\t\t\t\t\t>../actual.replace-missing\n\nI wonder whether we'd rather wnat to wire this up in a new test instead\nof altering an existing one.\n\nPatrick\n"},{"id":"502663","messageId":"ZuK8yihccDjgQGZV@pks.im","threadId":"62098","inReplyTo":"xmqqseu56hhb.fsf@gitster.g","subject":"Re: [PATCH 2/4] remote: print an error if refspec cannot be removed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-12T10:04:58Z","receivedAt":"2024-09-12T10:05:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Sep 11, 2024 at 01:52:16PM -0700, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n> > From: Phillip Wood <phillip.wood@dunelm.org.uk>\n> >\n> > If the existing fetch refspecs cannot be removed when replacing the set\n> > of branches to fetch with \"git remote set-branches\" the command silently\n> > fails. Add an error message to tell the user what when wrong.\n> >\n> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> > ---\n> >  builtin/remote.c | 1 +\n> >  1 file changed, 1 insertion(+)\n> >\n> > diff --git a/builtin/remote.c b/builtin/remote.c\n> > index 794396ba02f..4dbf7a4c506 100644\n> > --- a/builtin/remote.c\n> > +++ b/builtin/remote.c\n> > @@ -1603,6 +1603,7 @@ static int set_remote_branches(const char *remotename, const char **branches,\n> >  \t}\n> >  \n> >  \tif (!add_mode && remove_all_fetch_refspecs(key.buf)) {\n> > +\t\terror(_(\"could not remove existing fetch refspec\"));\n> >  \t\tstrbuf_release(&key);\n> >  \t\treturn 1;\n> >  \t}\n> \n> It is a minor point, but would it help to say what we tried to\n> remove (e.g. \"from remote X\") or is it too obvious to the end user\n> in the context they get this error?\n> \n> The reason why I had the above question was because inserting error()\n> before strbuf_release(&key) looked curious and I initially suspected\n> that it was because key was used in the error message somehow, but it\n> turns out that is not the case at all.\n> \n> IOW, I would have expected something more like this:\n> \n>  \tif (!add_mode && remove_all_fetch_refspecs(key.buf)) {\n>  \t\tstrbuf_release(&key);\n> +\t\treturn error(_(\"failed to remove fetch refspec from '%s'\"),\n> +\t\t\t\tremotename);\n> \n>  \t}\n\nI don't think we want to return the error code from `error()`, do we?\n`set_branches()` is wired up as a subcommand, so we'd ultimately do\n`exit(-1)` instead of `exit(1)` if we returned the `error()` code here.\n\nPatrick\n"},{"id":"502664","messageId":"ZuK8zjWyvMc77eQH@pks.im","threadId":"62098","inReplyTo":"f30c77bc36072df57662cac0cb7bf1bbea378062.1726067917.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] remote add: use strvec to store tracking branches","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-12T10:05:02Z","receivedAt":"2024-09-12T10:05:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Sep 11, 2024 at 03:18:36PM +0000, Phillip Wood via GitGitGadget wrote:\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n> \n> Store the list of branches to track in a ’struct strvec' instead of a\n> 'struct string_list'. This in preparation for the next commit where it\n\ns/in/is &/\n\n> will be convenient to have them stored in a NULL terminated array. This\n> means that we now duplicate the strings when storing them but the\n> overhead is not significant.\n\nYup. Micro-optimizations like this typically don't really have any real\nworld effect anyway.\n\nPatrick\n"},{"id":"502665","messageId":"ZuK80YvPSo8WUpp2@pks.im","threadId":"62098","inReplyTo":"xmqqfrq686n5.fsf@gitster.g","subject":"Re: [PATCH 4/4] remote: check branch names","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-12T10:05:05Z","receivedAt":"2024-09-12T10:05:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Sep 11, 2024 at 10:03:26AM -0700, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n> > +static int check_branch_names(const char **branches)\n> > +{\n> > +\tint ret = 0;\n> > +\n> > +\tfor (const char **b = branches; *b; b++) {\n> > +\t\tif (check_refname_format(*b, REFNAME_ALLOW_ONELEVEL |\n> > +\t\t\t\t\t\tREFNAME_REFSPEC_PATTERN))\n> > +\t\t\tret = error(_(\"invalid branch name '%s'\"), *b);\n> > +\t}\n> > +\n> > +\treturn ret;\n> > +}\n> \n> This implementation is inconsistent with what \"git branch new HEAD\"\n> uses to check the validity of \"new\", which is in this call chain:\n> \n>     builtin/branch.c:cmd_branch()\n>     -> branch.c:create_branch()\n>        -> branch.c:validate_new_branchname()\n>           -> branch.c:validate_branchname()\n>              -> object-name.c:strbuf_check_branch_ref()\n> \n> At least, we should prepend \"refs/heads/\" to *b, so that we can\n> reject \"refs/heads/HEAD\".  The authoritative logic in the above\n> however may further evolve, and we need to make sure that these two\n> checks from drifting away from each other over time.  We probably\n> should refactor the leaf function in the above call chain so that\n> both places can use it (the main difference is that you allow '*' in\n> yours when calling check_refname_format()).\n> \n>     Side note: we *should* lose \"strbuf_\" from its name, as it is\n>                not about string manipulation but the \"strbuf'-ness\n>                of the function is merely that as the side effect of\n>                checking it computes a full refname and it happens to\n>                use strbuf as a mechanism to return it.\n> \n> Something like the patch attached at the end.\n\nAgreed. It's also kind of curious that the function lives in\n\"object-name.c\" and not in \"refs.c\".\n\nPatrick\n"},{"id":"502709","messageId":"xmqqplp84zbe.fsf@gitster.g","threadId":"62098","inReplyTo":"ZuK8yihccDjgQGZV@pks.im","subject":"Re: [PATCH 2/4] remote: print an error if refspec cannot be removed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-12T16:22:13Z","receivedAt":"2024-09-12T16:22:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> I don't think we want to return the error code from `error()`, do we?\n> `set_branches()` is wired up as a subcommand, so we'd ultimately do\n> `exit(-1)` instead of `exit(1)` if we returned the `error()` code here.\n\nHmph, I thought there was somebody doing !! to canonicalize the\nreturn value to exit status in the call chain.\n\n\t... goes and looks again ...\n\nAfter finding the subcommand in fn, cmd_remote() ends with\n\n\tif (fn) {\n\t\treturn !!fn(argc, argv, prefix);\n\t} else {\n\t\t...\n\t\treturn !!show_all();\n\t}\n\n\n"},{"id":"502710","messageId":"xmqqjzfg4yv1.fsf@gitster.g","threadId":"62098","inReplyTo":"ZuK80YvPSo8WUpp2@pks.im","subject":"Re: [PATCH 4/4] remote: check branch names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-12T16:32:02Z","receivedAt":"2024-09-12T16:32:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Agreed. It's also kind of curious that the function lives in\n> \"object-name.c\" and not in \"refs.c\".\n\nBecause the helper groks things like \"-\" (aka \"@{-1}\"), it does a\nbit more than \"is this a reasonable name for a ref\" and \"please give\nme the current value of this ref\".  Also \"refs/remotes/origin/HEAD\"\nmay be valid as a refname, but forbidding \"refs/heads/HEAD\" is done\nconceptually one level closer to the end-users.  Eventually, I think\nit should move next to branch.c:validate_branchname() as a common\nhelper between \"git branch\" and \"git remote\" (possibly also with\n\"git switch/checkout\", if they need to do validation themselves, but\nI suspect they just call into branch.c at a bit higher \"here is a\nname, create it and you are free to complain---I do not care about\nthe details of why you decide the name is bad\" interface).\n\nThanks.\n\n\n\n\n"},{"id":"502727","messageId":"ZuOsr8k0JapOx0gf@pks.im","threadId":"62098","inReplyTo":"xmqqplp84zbe.fsf@gitster.g","subject":"Re: [PATCH 2/4] remote: print an error if refspec cannot be removed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-13T03:08:38Z","receivedAt":"2024-09-13T03:08:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Sep 12, 2024 at 09:22:13AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > I don't think we want to return the error code from `error()`, do we?\n> > `set_branches()` is wired up as a subcommand, so we'd ultimately do\n> > `exit(-1)` instead of `exit(1)` if we returned the `error()` code here.\n> \n> Hmph, I thought there was somebody doing !! to canonicalize the\n> return value to exit status in the call chain.\n> \n> \t... goes and looks again ...\n> \n> After finding the subcommand in fn, cmd_remote() ends with\n> \n> \tif (fn) {\n> \t\treturn !!fn(argc, argv, prefix);\n> \t} else {\n> \t\t...\n> \t\treturn !!show_all();\n> \t}\n\nAh, never mind in that case. I didn't look far enough indeed. Thanks for\ncorrecting my claim!\n\nPatrick\n"},{"id":"502755","messageId":"4915a1ba-eda9-435b-b615-4f78c7fe25f7@gmail.com","threadId":"62098","inReplyTo":"xmqqfrq686n5.fsf@gitster.g","subject":"Re: [PATCH 4/4] remote: check branch names","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-09-13T15:09:58Z","receivedAt":"2024-09-13T15:10:01Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 11/09/2024 18:03, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n> The authoritative logic in the above\n> however may further evolve, and we need to make sure that these two\n> checks from drifting away from each other over time.  We probably\n> should refactor the leaf function in the above call chain so that\n> both places can use it (the main difference is that you allow '*' in\n> yours when calling check_refname_format()).\n> \n>      Side note: we *should* lose \"strbuf_\" from its name, as it is\n>                 not about string manipulation but the \"strbuf'-ness\n>                 of the function is merely that as the side effect of\n>                 checking it computes a full refname and it happens to\n>                 use strbuf as a mechanism to return it.\n> \n> Something like the patch attached at the end.\n\nThanks for the patch, I'll re-roll based on that. I wonder if we really \nwant to support \"@{-N}\" when setting remote tracking branches though - \nshould we be using INTERPRET_BRANCH_REMOTE instead when calling \nstrbuf_branchname()?\n\nBest Wishes\n\nPhillip\n\n>>   static const char mirror_advice[] =\n>>   N_(\"--mirror is dangerous and deprecated; please\\n\"\n>>      \"\\t use --mirror=fetch or --mirror=push instead\");\n>> @@ -203,6 +216,9 @@ static int add(int argc, const char **argv, const char *prefix)\n>>   \tif (!valid_remote_name(name))\n>>   \t\tdie(_(\"'%s' is not a valid remote name\"), name);\n>>   \n>> +\tif (check_branch_names(track.v))\n>> +\t\texit(128);\n>> +\n> \n> Seeing that the loop in check_branch_names() is brand new and you\n> could have iterated over a string-list just as easily, I somehow\n> doubt that step [3/4] was fully warranted.\n> \n>> @@ -1601,6 +1617,9 @@ static int set_remote_branches(const char *remotename, const char **branches,\n>>   \t\texit(2);\n>>   \t}\n>>   \n>> +\tif (check_branch_names(branches))\n>> +\t\texit(128);\n> \n> But here you are already passed \"const char *branches[]\" to this caller,\n> and it would be hassle to turn it into string_list, so [3/4] is fine\n> after all.\n> \n> \n> \n>   object-name.h |  2 ++\n>   object-name.c | 23 +++++++++++++++++++++--\n>   2 files changed, 23 insertions(+), 2 deletions(-)\n> \n> diff --git i/object-name.h w/object-name.h\n> index 8dba4a47a4..fa70d42044 100644\n> --- i/object-name.h\n> +++ w/object-name.h\n> @@ -130,4 +130,6 @@ struct object *repo_peel_to_type(struct repository *r,\n>   /* used when the code does not know or care what the default abbrev is */\n>   #define FALLBACK_DEFAULT_ABBREV 7\n>   \n> +/* Check if \"name\" is allowed as a branch */\n> +int valid_branch_name(const char *name, int allow_wildcard);\n>   #endif /* OBJECT_NAME_H */\n> diff --git i/object-name.c w/object-name.c\n> index 09c1bd93a3..e3bed5a664 100644\n> --- i/object-name.c\n> +++ w/object-name.c\n> @@ -1747,7 +1747,8 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)\n>   \tstrbuf_add(sb, name + used, len - used);\n>   }\n>   \n> -int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n> +static int full_ref_from_branch_name_internal(struct strbuf *sb, const char *name,\n> +\t\t\t\t\t      int crf_flags)\n>   {\n>   \tif (startup_info->have_repository)\n>   \t\tstrbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);\n> @@ -1766,7 +1767,25 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n>   \t    !strcmp(sb->buf, \"refs/heads/HEAD\"))\n>   \t\treturn -1;\n>   \n> -\treturn check_refname_format(sb->buf, 0);\n> +\treturn check_refname_format(sb->buf, crf_flags);\n> +}\n> +\n> +/* NEEDSWORK: rename this to full_ref_from_branch_name */\n> +int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n> +{\n> +\treturn full_ref_from_branch_name_internal(sb, name, 0);\n> +}\n> +\n> +int valid_branch_name(const char *name, int allow_wildcard)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tint ret;\n> +\tint flags;\n> +\n> +\tflags = allow_wildcard ? REFNAME_REFSPEC_PATTERN : 0;\n> +\tret = full_ref_from_branch_name_internal(&sb, name, flags);\n> +\tstrbuf_release(&sb);\n> +\treturn ret;\n>   }\n>   \n>   void object_context_release(struct object_context *ctx)\n"},{"id":"502756","messageId":"acd287c8-990b-42b7-85dd-a206a887b8ee@gmail.com","threadId":"62098","inReplyTo":"xmqqseu56hhb.fsf@gitster.g","subject":"Re: [PATCH 2/4] remote: print an error if refspec cannot be removed","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-09-13T15:11:53Z","receivedAt":"2024-09-13T15:11:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 11/09/2024 21:52, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>>   \tif (!add_mode && remove_all_fetch_refspecs(key.buf)) {\n>> +\t\terror(_(\"could not remove existing fetch refspec\"));\n>>   \t\tstrbuf_release(&key);\n>>   \t\treturn 1;\n>>   \t}\n> \n> It is a minor point, but would it help to say what we tried to\n> remove (e.g. \"from remote X\") or is it too obvious to the end user\n> in the context they get this error?\n\nThe user has to give the remote name on the command line so I think it \nshould be obvious to the user.\n\n> The reason why I had the above question was because inserting error()\n> before strbuf_release(&key) looked curious and I initially suspected\n> that it was because key was used in the error message somehow, but it\n> turns out that is not the case at all.\n\nArguably we should refactor this to use our standard \"goto cleanup\" pattern.\n\nBest Wishes\n\nPhillip\n\n> IOW, I would have expected something more like this:\n> \n>   \tif (!add_mode && remove_all_fetch_refspecs(key.buf)) {\n>   \t\tstrbuf_release(&key);\n> +\t\treturn error(_(\"failed to remove fetch refspec from '%s'\"),\n> +\t\t\t\tremotename);\n> \n>   \t}\n> \n"},{"id":"502766","messageId":"xmqqfrq3zc6n.fsf@gitster.g","threadId":"62098","inReplyTo":"acd287c8-990b-42b7-85dd-a206a887b8ee@gmail.com","subject":"Re: [PATCH 2/4] remote: print an error if refspec cannot be removed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-13T17:38:24Z","receivedAt":"2024-09-13T17:38:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"phillip.wood123@gmail.com writes:\n\n> On 11/09/2024 21:52, Junio C Hamano wrote:\n>> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>>>   \tif (!add_mode && remove_all_fetch_refspecs(key.buf)) {\n>>> +\t\terror(_(\"could not remove existing fetch refspec\"));\n>>>   \t\tstrbuf_release(&key);\n>>>   \t\treturn 1;\n>>>   \t}\n>> It is a minor point, but would it help to say what we tried to\n>> remove (e.g. \"from remote X\") or is it too obvious to the end user\n>> in the context they get this error?\n>\n> The user has to give the remote name on the command line so I think it\n> should be obvious to the user.\n\nThat makes sense.  Thanks.\n"},{"id":"502768","messageId":"xmqqy13vxx3m.fsf@gitster.g","threadId":"62098","inReplyTo":"4915a1ba-eda9-435b-b615-4f78c7fe25f7@gmail.com","subject":"Re: [PATCH 4/4] remote: check branch names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-13T17:49:33Z","receivedAt":"2024-09-13T17:49:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"phillip.wood123@gmail.com writes:\n\n> Thanks for the patch, I'll re-roll based on that. I wonder if we\n> really want to support \"@{-N}\" when setting remote tracking branches\n> though - should we be using INTERPRET_BRANCH_REMOTE instead when\n> calling strbuf_branchname()?\n\nPerhaps.  Users try to use \"-\" in surprising places, though ;-)\n\n"},{"id":"503001","messageId":"82a08e44-3822-4a26-ad09-4ca1a9b44c11@gmail.com","threadId":"62098","inReplyTo":"xmqqy13vxx3m.fsf@gitster.g","subject":"Re: [PATCH 4/4] remote: check branch names","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-09-18T13:18:39Z","receivedAt":"2024-09-18T13:18:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 13/09/2024 18:49, Junio C Hamano wrote:\n> phillip.wood123@gmail.com writes:\n> \n>> Thanks for the patch, I'll re-roll based on that. I wonder if we\n>> really want to support \"@{-N}\" when setting remote tracking branches\n>> though - should we be using INTERPRET_BRANCH_REMOTE instead when\n>> calling strbuf_branchname()?\n> \n> Perhaps.  Users try to use \"-\" in surprising places, though ;-)\n\nstrbuf_check_branch_ref() already rejects \"-\".\n\nINTERPRET_BRANCH_REMOTE supports @{upstream} which might be useful but \nthen we will need to check it refers to the correct remote and expand it \nwhen setting the fetch refspec so a boolean function to check if a name \nis acceptable is insufficient. Given that \"git remote set-branches\" has \nonly ever supported \"real\" branch names and patterns on the command line \nand no-one has complained I wonder if we're better off doing something like\n\n\tif (strbuf_check_branch_ref(&buf, branch_name) ||\n\t    strcmp(buf.buf + 11, branch_name))\n\t\terror(_(\"invalid branch name '%s'\", branch_name));\n\nwhere the \"buf.buf + 11\" skips \"refs/heads/\"\n\nBest Wishes\n\nPhillip\n"},{"id":"503019","messageId":"xmqqh6acbti0.fsf@gitster.g","threadId":"62098","inReplyTo":"82a08e44-3822-4a26-ad09-4ca1a9b44c11@gmail.com","subject":"Re: [PATCH 4/4] remote: check branch names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-18T20:24:07Z","receivedAt":"2024-09-18T20:24:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"phillip.wood123@gmail.com writes:\n\n> ... Given that \"git remote\n> set-branches\" has only ever supported \"real\" branch names and patterns\n> on the command line and no-one has complained I wonder if we're better\n> off doing something like\n>\n> \tif (strbuf_check_branch_ref(&buf, branch_name) ||\n> \t    strcmp(buf.buf + 11, branch_name))\n> \t\terror(_(\"invalid branch name '%s'\", branch_name));\n>\n> where the \"buf.buf + 11\" skips \"refs/heads/\"\n\nYeah, replacing +11 with skip_prefix() or something for readability,\nsuch a check might be good enough in pracrice.\n\nThanks.\n"}]}