{"thread":{"id":"62083","subject":"[PATCH] remote: introduce config to set prefetch refs","startedAt":"2024-09-09T09:47:17Z","lastAt":"2024-11-06T11:04:51Z","messageCount":27,"participants":["Shubham Kanodia via GitGitGadget","Shubham Kanodia","Junio C Hamano","Patrick Steinhardt","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"502430","messageId":"pull.1782.git.1725875232922.gitgitgadget@gmail.com","threadId":"62083","inReplyTo":null,"subject":"[PATCH] remote: introduce config to set prefetch refs","fromName":"Shubham Kanodia via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-09T09:47:12Z","receivedAt":"2024-09-09T09:47:17Z","isPatch":true,"sender":{"key":"shubham.kanodia10@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8946207?v=4"},"body":"From: Shubham Kanodia \n\nThis commit introduces a new configuration option,\nremote..prefetchref, which allows users to specify specific\nref patterns to be prefetched during a git fetch --prefetch\noperation.\n\nThe new option accepts a space-separated list of ref patterns.\nWhen the --prefetch option is used with git fetch, only the refs\nmatching these patterns will be prefetched, instead of the\ndefault behavior of prefetching all fetchable refs.\n\nExample usage in .git/config:\n[remote \"origin\"]\n prefetchref = \"refs/heads/main refs/heads/feature/*\"\n\nThis change allows users to optimize their prefetch operations, potentially\nreducing network traffic and improving performance for large repositories\nwith many refs.\n\nSigned-off-by: Shubham Kanodia \n---\n remote: introduce config to set prefetch refs\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1782%2Fpastelsky%2Fsk%2Fremote-prefetchref-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1782/pastelsky/sk/remote-prefetchref-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1782\n\n Documentation/config/remote.txt | 6 +++\n builtin/fetch.c | 29 +++++++++++++-\n remote.c | 8 ++++\n remote.h | 3 ++\n t/t7900-maintenance.sh | 70 +++++++++++++++++++++++++++++++++\n 5 files changed, 115 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\nindex 8efc53e836d..b25d76dd3b1 100644\n--- a/Documentation/config/remote.txt\n+++ b/Documentation/config/remote.txt\n@@ -33,6 +33,12 @@ remote..fetch::\n \tThe default set of \"refspec\" for linkgit:git-fetch[1]. See\n \tlinkgit:git-fetch[1].\n \n+remote..prefetchref::\n+ Specify the refs to be prefetched when fetching from this remote.\n+ The value is a space-separated list of ref patterns (e.g., \"refs/heads/master refs/heads/develop*\").\n+ These patterns are used as the source part of the refspecs for prefetching.\n+ This can be used to optimize fetch operations by specifying exactly which refs should be prefetched.\n+\n remote..push::\n \tThe default set of \"refspec\" for linkgit:git-push[1]. See\n \tlinkgit:git-push[1].\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b2b5aee5bf2..6e584fa2ebb 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -434,6 +434,30 @@ static void find_non_local_tags(const struct ref *refs,\n \toidset_clear(&fetch_oids);\n }\n \n+static void apply_prefetch_refspec(struct remote *remote, struct refspec *rs)\n+{\n+\tif (remote->prefetch_refs.nr > 0) {\n+\t\tint i;\n+\t\tfor (i = 0; i < remote->prefetch_refs.nr; i++) {\n+\t\t\tconst char *src = remote->prefetch_refs.items[i].string;\n+\t\t\tstruct strbuf dst = STRBUF_INIT;\n+\n+\t\t\tstrbuf_addf(&dst, \"refs/prefetch/%s/\", remote->name);\n+\t\t\tif (starts_with(src, \"refs/heads/\")) {\n+\t\t\t\tstrbuf_addstr(&dst, src + 11);\n+\t\t\t} else if (starts_with(src, \"refs/\")) {\n+\t\t\t\tstrbuf_addstr(&dst, src + 5);\n+\t\t\t} else {\n+\t\t\t\tstrbuf_addstr(&dst, src);\n+\t\t\t}\n+\n+\t\t\trefspec_appendf(rs, \"%s:%s\", src, dst.buf);\n+\t\t\tstrbuf_release(&dst);\n+\t\t}\n+\t}\n+}\n+\n+\n static void filter_prefetch_refspec(struct refspec *rs)\n {\n \tint i;\n@@ -502,8 +526,11 @@ static struct ref *get_ref_map(struct remote *remote,\n \tint existing_refs_populated = 0;\n \n \tfilter_prefetch_refspec(rs);\n-\tif (remote)\n+\tif (remote) {\n \t\tfilter_prefetch_refspec(&remote->fetch);\n+\t\tif (prefetch)\n+\t\t\tapply_prefetch_refspec(remote, rs);\n+\t}\n \n \tif (rs->nr) {\n \t\tstruct refspec *fetch_refspec;\ndiff --git a/remote.c b/remote.c\nindex 8f3dee13186..b46d62b2c47 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -141,6 +141,7 @@ static struct remote *make_remote(struct remote_state *remote_state,\n \tret->prune = -1; /* unspecified */\n \tret->prune_tags = -1; /* unspecified */\n \tret->name = xstrndup(name, len);\n+\tstring_list_init_dup(&ret->prefetch_refs);\n \trefspec_init(&ret->push, REFSPEC_PUSH);\n \trefspec_init(&ret->fetch, REFSPEC_FETCH);\n \n@@ -166,6 +167,7 @@ static void remote_clear(struct remote *remote)\n \tfree((char *)remote->uploadpack);\n \tFREE_AND_NULL(remote->http_proxy);\n \tFREE_AND_NULL(remote->http_proxy_authmethod);\n+\tstring_list_clear(&remote->prefetch_refs, 0);\n }\n \n static void add_merge(struct branch *branch, const char *name)\n@@ -456,6 +458,12 @@ static int handle_config(const char *key, const char *value,\n \t\tremote->prune = git_config_bool(key, value);\n \telse if (!strcmp(subkey, \"prunetags\"))\n \t\tremote->prune_tags = git_config_bool(key, value);\n+\telse if (!strcmp(subkey, \"prefetchref\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(key);\n+\t\tstring_list_split(&remote->prefetch_refs, value, ' ', -1);\n+\t\treturn 0;\n+\t}\n \telse if (!strcmp(subkey, \"url\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(key);\ndiff --git a/remote.h b/remote.h\nindex b901b56746d..c18e68e0d8d 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -5,6 +5,7 @@\n #include \"hashmap.h\"\n #include \"refspec.h\"\n #include \"strvec.h\"\n+#include \"string-list.h\"\n \n struct option;\n struct transport_ls_refs_options;\n@@ -77,6 +78,8 @@ struct remote {\n \n \tstruct refspec fetch;\n \n+\tstruct string_list prefetch_refs;\n+\n \t/*\n \t * The setting for whether to fetch tags (as a separate rule from the\n \t * configured refspecs);\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex abae7a97546..2ad5b922d83 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -245,6 +245,76 @@ test_expect_success 'prefetch multiple remotes' '\n \ttest_subcommand git fetch remote2 $fetchargs .prefetchref refs if present' '\n+\ttest_create_repo prefetch-test-mixed-patterns &&\n+\t(\n+\t\tcd prefetch-test-mixed-patterns &&\n+\t\ttest_commit initial &&\n+\t\tgit clone . clone1 &&\n+\t\tgit clone . clone2 &&\n+\n+\t\tgit remote add remote1 \"file://$(pwd)/clone1\" &&\n+\t\tgit remote add remote2 \"file://$(pwd)/clone2\" &&\n+\n+\t\t# Set single prefetchref pattern for remote1 and multiple for remote2\n+\t\tgit config remote.remote1.prefetchref \"refs/heads/foo\" &&\n+\t\tgit config remote.remote2.prefetchref \"refs/heads/feature/* refs/heads/topic\" &&\n+\n+\t\t# Create branches in clone1 and push\n+\t\t(\n+\t\t\tcd clone1 &&\n+\t\t\tgit checkout -b foo &&\n+\t\t\ttest_commit foo-commit &&\n+\t\t\tgit checkout -b feature/a &&\n+\t\t\ttest_commit feature-a-commit &&\n+\t\t\tgit checkout -b other &&\n+\t\t\ttest_commit other-commit &&\n+\t\t\tgit push origin foo feature/a other\n+\t\t) &&\n+\n+\t\t# Create branches in clone2 and push\n+\t\t(\n+\t\t\tcd clone2 &&\n+\t\t\tgit checkout -b topic &&\n+\t\t\ttest_commit master-commit &&\n+\t\t\tgit checkout -b feature/x &&\n+\t\t\ttest_commit feature-x-commit &&\n+\t\t\tgit checkout -b feature/y &&\n+\t\t\ttest_commit feature-y-commit &&\n+\t\t\tgit checkout -b dev &&\n+\t\t\ttest_commit dev-commit &&\n+\t\t\tgit push origin topic feature/x feature/y dev\n+\t\t) &&\n+\n+\t\t# Run maintenance prefetch task\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/prefetch.txt\" git maintenance run --task=prefetch 2>/dev/null &&\n+\n+\t\t# Check that only specified refs were prefetched\n+\t\tfetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head --recurse-submodules=no --quiet\" &&\n+\t\ttest_subcommand git fetch remote1 $fetchargs wrote:\n>\n> From: Shubham Kanodia \n>\n> This commit introduces a new configuration option,\n> remote..prefetchref, which allows users to specify specific\n> ref patterns to be prefetched during a git fetch --prefetch\n> operation.\n>\n> The new option accepts a space-separated list of ref patterns.\n> When the --prefetch option is used with git fetch, only the refs\n> matching these patterns will be prefetched, instead of the\n> default behavior of prefetching all fetchable refs.\n>\n> Example usage in .git/config:\n> [remote \"origin\"]\n> prefetchref = \"refs/heads/main refs/heads/feature/*\"\n>\n> This change allows users to optimize their prefetch operations, potentially\n> reducing network traffic and improving performance for large repositories\n> with many refs.\n>\n> Signed-off-by: Shubham Kanodia \n> ---\n> remote: introduce config to set prefetch refs\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1782%2Fpastelsky%2Fsk%2Fremote-prefetchref-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1782/pastelsky/sk/remote-prefetchref-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1782\n>\n> Documentation/config/remote.txt | 6 +++\n> builtin/fetch.c | 29 +++++++++++++-\n> remote.c | 8 ++++\n> remote.h | 3 ++\n> t/t7900-maintenance.sh | 70 +++++++++++++++++++++++++++++++++\n> 5 files changed, 115 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\n> index 8efc53e836d..b25d76dd3b1 100644\n> --- a/Documentation/config/remote.txt\n> +++ b/Documentation/config/remote.txt\n> @@ -33,6 +33,12 @@ remote..fetch::\n> The default set of \"refspec\" for linkgit:git-fetch[1]. See\n> linkgit:git-fetch[1].\n>\n> +remote..prefetchref::\n> + Specify the refs to be prefetched when fetching from this remote.\n> + The value is a space-separated list of ref patterns (e.g., \"refs/heads/master refs/heads/develop*\").\n> + These patterns are used as the source part of the refspecs for prefetching.\n> + This can be used to optimize fetch operations by specifying exactly which refs should be prefetched.\n> +\n> remote..push::\n> The default set of \"refspec\" for linkgit:git-push[1]. See\n> linkgit:git-push[1].\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index b2b5aee5bf2..6e584fa2ebb 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -434,6 +434,30 @@ static void find_non_local_tags(const struct ref *refs,\n> oidset_clear(&fetch_oids);\n> }\n>\n> +static void apply_prefetch_refspec(struct remote *remote, struct refspec *rs)\n> +{\n> + if (remote->prefetch_refs.nr > 0) {\n> + int i;\n> + for (i = 0; i < remote->prefetch_refs.nr; i++) {\n> + const char *src = remote->prefetch_refs.items[i].string;\n> + struct strbuf dst = STRBUF_INIT;\n> +\n> + strbuf_addf(&dst, \"refs/prefetch/%s/\", remote->name);\n> + if (starts_with(src, \"refs/heads/\")) {\n> + strbuf_addstr(&dst, src + 11);\n> + } else if (starts_with(src, \"refs/\")) {\n> + strbuf_addstr(&dst, src + 5);\n> + } else {\n> + strbuf_addstr(&dst, src);\n> + }\n> +\n> + refspec_appendf(rs, \"%s:%s\", src, dst.buf);\n> + strbuf_release(&dst);\n> + }\n> + }\n> +}\n> +\n> +\n> static void filter_prefetch_refspec(struct refspec *rs)\n> {\n> int i;\n> @@ -502,8 +526,11 @@ static struct ref *get_ref_map(struct remote *remote,\n> int existing_refs_populated = 0;\n>\n> filter_prefetch_refspec(rs);\n> - if (remote)\n> + if (remote) {\n> filter_prefetch_refspec(&remote->fetch);\n> + if (prefetch)\n> + apply_prefetch_refspec(remote, rs);\n> + }\n>\n> if (rs->nr) {\n> struct refspec *fetch_refspec;\n> diff --git a/remote.c b/remote.c\n> index 8f3dee13186..b46d62b2c47 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -141,6 +141,7 @@ static struct remote *make_remote(struct remote_state *remote_state,\n> ret->prune = -1; /* unspecified */\n> ret->prune_tags = -1; /* unspecified */\n> ret->name = xstrndup(name, len);\n> + string_list_init_dup(&ret->prefetch_refs);\n> refspec_init(&ret->push, REFSPEC_PUSH);\n> refspec_init(&ret->fetch, REFSPEC_FETCH);\n>\n> @@ -166,6 +167,7 @@ static void remote_clear(struct remote *remote)\n> free((char *)remote->uploadpack);\n> FREE_AND_NULL(remote->http_proxy);\n> FREE_AND_NULL(remote->http_proxy_authmethod);\n> + string_list_clear(&remote->prefetch_refs, 0);\n> }\n>\n> static void add_merge(struct branch *branch, const char *name)\n> @@ -456,6 +458,12 @@ static int handle_config(const char *key, const char *value,\n> remote->prune = git_config_bool(key, value);\n> else if (!strcmp(subkey, \"prunetags\"))\n> remote->prune_tags = git_config_bool(key, value);\n> + else if (!strcmp(subkey, \"prefetchref\")) {\n> + if (!value)\n> + return config_error_nonbool(key);\n> + string_list_split(&remote->prefetch_refs, value, ' ', -1);\n> + return 0;\n> + }\n> else if (!strcmp(subkey, \"url\")) {\n> if (!value)\n> return config_error_nonbool(key);\n> diff --git a/remote.h b/remote.h\n> index b901b56746d..c18e68e0d8d 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -5,6 +5,7 @@\n> #include \"hashmap.h\"\n> #include \"refspec.h\"\n> #include \"strvec.h\"\n> +#include \"string-list.h\"\n>\n> struct option;\n> struct transport_ls_refs_options;\n> @@ -77,6 +78,8 @@ struct remote {\n>\n> struct refspec fetch;\n>\n> + struct string_list prefetch_refs;\n> +\n> /*\n> * The setting for whether to fetch tags (as a separate rule from the\n> * configured refspecs);\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index abae7a97546..2ad5b922d83 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -245,6 +245,76 @@ test_expect_success 'prefetch multiple remotes' '\n> test_subcommand git fetch remote2 $fetchargs '\n>\n> +test_expect_success 'prefetch only acts on remote..prefetchref refs if present' '\n> + test_create_repo prefetch-test-mixed-patterns &&\n> + (\n> + cd prefetch-test-mixed-patterns &&\n> + test_commit initial &&\n> + git clone . clone1 &&\n> + git clone . clone2 &&\n> +\n> + git remote add remote1 \"file://$(pwd)/clone1\" &&\n> + git remote add remote2 \"file://$(pwd)/clone2\" &&\n> +\n> + # Set single prefetchref pattern for remote1 and multiple for remote2\n> + git config remote.remote1.prefetchref \"refs/heads/foo\" &&\n> + git config remote.remote2.prefetchref \"refs/heads/feature/* refs/heads/topic\" &&\n> +\n> + # Create branches in clone1 and push\n> + (\n> + cd clone1 &&\n> + git checkout -b foo &&\n> + test_commit foo-commit &&\n> + git checkout -b feature/a &&\n> + test_commit feature-a-commit &&\n> + git checkout -b other &&\n> + test_commit other-commit &&\n> + git push origin foo feature/a other\n> + ) &&\n> +\n> + # Create branches in clone2 and push\n> + (\n> + cd clone2 &&\n> + git checkout -b topic &&\n> + test_commit master-commit &&\n> + git checkout -b feature/x &&\n> + test_commit feature-x-commit &&\n> + git checkout -b feature/y &&\n> + test_commit feature-y-commit &&\n> + git checkout -b dev &&\n> + test_commit dev-commit &&\n> + git push origin topic feature/x feature/y dev\n> + ) &&\n> +\n> + # Run maintenance prefetch task\n> + GIT_TRACE2_EVENT=\"$(pwd)/prefetch.txt\" git maintenance run --task=prefetch 2>/dev/null &&\n> +\n> + # Check that only specified refs were prefetched\n> + fetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head --recurse-submodules=no --quiet\" &&\n> + test_subcommand git fetch remote1 $fetchargs + test_subcommand git fetch remote2 $fetchargs + ls -R .git/refs/prefetch &&\n> +\n> + # Verify that only specified refs are in the prefetch refs for remote1\n> + git rev-parse refs/prefetch/remotes/remote1/foo &&\n> + test_must_fail git rev-parse refs/prefetch/remotes/remote1/feature/a &&\n> + test_must_fail git rev-parse refs/prefetch/remotes/remote1/other &&\n> +\n> + # Verify that only specified refs are in the prefetch refs for remote2\n> + git rev-parse refs/prefetch/remotes/remote2/feature/x &&\n> + git rev-parse refs/prefetch/remotes/remote2/feature/y &&\n> + git rev-parse refs/prefetch/remotes/remote2/topic &&\n> + test_must_fail git rev-parse refs/prefetch/remotes/remote2/dev &&\n> +\n> + # Fetch all refs and compare\n> + git fetch --all &&\n> + test_cmp_rev refs/remotes/remote1/foo refs/prefetch/remotes/remote1/foo &&\n> + test_cmp_rev refs/remotes/remote2/feature/x refs/prefetch/remotes/remote2/feature/x &&\n> + test_cmp_rev refs/remotes/remote2/feature/y refs/prefetch/remotes/remote2/feature/y &&\n> + test_cmp_rev refs/remotes/remote2/topic refs/prefetch/remotes/remote2/topic\n> + )\n> +'\n> +\n> test_expect_success 'loose-objects task' '\n> # Repack everything so we know the state of the object dir\n> git repack -adk &&\n>\n> base-commit: 2e7b89e038c0c888acf61f1b4ee5a43d4dd5e94c\n> --\n> gitgitgadget\n\nThis is a continuation of my work, as we deemed that adding a\n`remote..prefetch` was unnecessary given that\nthere were already ways to stop fetching refs from a given remote\nusing `skipFetchAll`.\n\nLooking at getting early feedback on the direction & implementation\nhere since this isn't as straightforward as my last contribution.\nI would appreciate any thoughts! The config's name is tentatively\n`prefetchref,` but I'm open to suggestions.\n"},{"id":"502463","messageId":"xmqqzfogsrqo.fsf@gitster.g","threadId":"62083","inReplyTo":"pull.1782.git.1725875232922.gitgitgadget@gmail.com","subject":"Re: [PATCH] remote: introduce config to set prefetch refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-09T16:42:55Z","receivedAt":"2024-09-09T16:42:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shubham Kanodia via GitGitGadget\" writes:\n\n> +static void apply_prefetch_refspec(struct remote *remote, struct refspec *rs)\n> +{\n> +\tif (remote->prefetch_refs.nr > 0) {\n> +\t\tint i;\n> +\t\tfor (i = 0; i < remote->prefetch_refs.nr; i++) {\n> +\t\t\tconst char *src = remote->prefetch_refs.items[i].string;\n> +\t\t\tstruct strbuf dst = STRBUF_INIT;\n> +\n> +\t\t\tstrbuf_addf(&dst, \"refs/prefetch/%s/\", remote->name);\n> +\t\t\tif (starts_with(src, \"refs/heads/\")) {\n> +\t\t\t\tstrbuf_addstr(&dst, src + 11);\n> +\t\t\t} else if (starts_with(src, \"refs/\")) {\n> +\t\t\t\tstrbuf_addstr(&dst, src + 5);\n> +\t\t\t} else {\n> +\t\t\t\tstrbuf_addstr(&dst, src);\n> +\t\t\t}\n> +\n> +\t\t\trefspec_appendf(rs, \"%s:%s\", src, dst.buf);\n> +\t\t\tstrbuf_release(&dst);\n> +\t\t}\n> +\t}\n> +}\n> static void filter_prefetch_refspec(struct refspec *rs)\n> {\n> \tint i;\n> @@ -502,8 +526,11 @@ static struct ref *get_ref_map(struct remote *remote,\n> \tint existing_refs_populated = 0;\n> \n> \tfilter_prefetch_refspec(rs);\n> -\tif (remote)\n> +\tif (remote) {\n> \t\tfilter_prefetch_refspec(&remote->fetch);\n> +\t\tif (prefetch)\n> +\t\t\tapply_prefetch_refspec(remote, rs);\n> +\t}\n\nHmph, a separate helper function with a separate loop was something\nI did not expect to see. Looking at the filter_prefetch_refspec(),\nit already limits what it prefetched to what we usually fetch from\nthe remote, and filteres out the tag namespace. I was hoping that\nthis will _extend_ that existing mechanism, as if by default we\nhave prefetch refspec \"!refs/tags/*\", which can be replaced by the\nconfiguration to say \"!refs/tags/* !refs/changes/*\", or positive\nones like \"refs/heads/*\". The filtering semantics should be\n\n * a refspec whose src matches negated pattern (like !refs/tags/*)\n is rejected.\n\n * if the prefetch_refs has *only* positive patterns, then a refspec\n whose src does not match *any* of the pattern is rejected.\n\n * a refspec that is not rejected is prefetched.\n\nBut the above still allows what filter_prefetch_refspec() does by\ndefault, without any way to narrow it down, and then adds its own\nentries.\n\nThis is a half-tangent, but while studying for this topic, I noticed\nthat filter_prefetch_refspec() triggers O(n) loop every time a fetch\nrefspec is skipped. \n\nIt all comes from 2e03115d (fetch: add --prefetch option,\n2021-04-16) but rewriting the loop to use two pointers into the\narray seemed trivial and the result seemed a bit more readable.\n\nYour \"further limit the prefetched refs with configuration\" feature\nwould probably replace this part of the updated code:\n\n+\t\t/* skip ones that do not store, or store in refs/tags */\n+\t\tif (!rs->items[scan].dst ||\n+\t\t (rs->items[scan].src &&\n+\t\t starts_with(rs->items[scan].src,\n+\t\t\t\t ref_namespace[NAMESPACE_TAGS].ref))) {\n\nThat is, instead of \"skip ones that do not store, or store in refs/tags\",\nfiltering using the configured value (probably implemented as a helper\nfunction) would be used as the condition of the if statement.\n\nThoughts?\n\n builtin/fetch.c | 46 ++++++++++++++++++++++++----------------------\n 1 file changed, 24 insertions(+), 22 deletions(-)\n\ndiff --git c/builtin/fetch.c w/builtin/fetch.c\nindex 693f02b958..219302ed67 100644\n--- c/builtin/fetch.c\n+++ w/builtin/fetch.c\n@@ -436,37 +436,38 @@ static void find_non_local_tags(const struct ref *refs,\n \n static void filter_prefetch_refspec(struct refspec *rs)\n {\n-\tint i;\n+\tint scan, store;\n \n \tif (!prefetch)\n \t\treturn;\n \n-\tfor (i = 0; i < rs->nr; i++) {\n+\t/*\n+\t * scan refspec items with 'scan', and decide to either\n+\t * mangle and store it in 'store', or omit it from the result.\n+\t */\n+\tfor (scan = store = 0; scan < rs->nr; scan++, store++) {\n \t\tstruct strbuf new_dst = STRBUF_INIT;\n \t\tchar *old_dst;\n \t\tconst char *sub = NULL;\n \n-\t\tif (rs->items[i].negative)\n-\t\t\tcontinue;\n-\t\tif (!rs->items[i].dst ||\n-\t\t (rs->items[i].src &&\n-\t\t starts_with(rs->items[i].src,\n-\t\t\t\t ref_namespace[NAMESPACE_TAGS].ref))) {\n-\t\t\tint j;\n-\n-\t\t\tfree(rs->items[i].src);\n-\t\t\tfree(rs->items[i].dst);\n-\n-\t\t\tfor (j = i + 1; j < rs->nr; j++) {\n-\t\t\t\trs->items[j - 1] = rs->items[j];\n-\t\t\t\trs->raw[j - 1] = rs->raw[j];\n-\t\t\t}\n-\t\t\trs->nr--;\n-\t\t\ti--;\n+\t\t/* negative ones are kept as-is */\n+\t\tif (rs->items[scan].negative) {\n+\t\t\tif (store != scan)\n+\t\t\t\trs->items[store] = rs->items[scan];\n \t\t\tcontinue;\n \t\t}\n \n-\t\told_dst = rs->items[i].dst;\n+\t\t/* skip ones that do not store, or store in refs/tags */\n+\t\tif (!rs->items[scan].dst ||\n+\t\t (rs->items[scan].src &&\n+\t\t starts_with(rs->items[scan].src,\n+\t\t\t\t ref_namespace[NAMESPACE_TAGS].ref))) {\n+\t\t\trefspec_item_clear(&rs->items[scan]);\n+\t\t\tstore--; /* compensate for loop's auto increment */\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\told_dst = rs->items[scan].dst;\n \t\tstrbuf_addstr(&new_dst, ref_namespace[NAMESPACE_PREFETCH].ref);\n \n \t\t/*\n@@ -478,11 +479,12 @@ static void filter_prefetch_refspec(struct refspec *rs)\n \t\t\tsub = old_dst;\n \t\tstrbuf_addstr(&new_dst, sub);\n \n-\t\trs->items[i].dst = strbuf_detach(&new_dst, NULL);\n-\t\trs->items[i].force = 1;\n+\t\trs->items[store].dst = strbuf_detach(&new_dst, NULL);\n+\t\trs->items[store].force = 1;\n \n \t\tfree(old_dst);\n \t}\n+\trs->nr = store;\n }\n \n static struct ref *get_ref_map(struct remote *remote,\n"},{"id":"502469","messageId":"CAG=Um+0GvFzdAZrCgoS52xh9DF2pntQ+7i+vqYMFQf-MWr3H5A@mail.gmail.com","threadId":"62083","inReplyTo":"xmqqzfogsrqo.fsf@gitster.g","subject":"Re: [PATCH] remote: introduce config to set prefetch refs","fromName":"Shubham Kanodia","fromEmail":"shubham.kanodia10@gmail.com","sentAt":"2024-09-09T18:21:39Z","receivedAt":"2024-09-09T18:22:17Z","isPatch":true,"sender":{"key":"shubham.kanodia10@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8946207?v=4"},"body":"On Mon, Sep 9, 2024 at 10:12 PM Junio C Hamano wrote:\n>\n> \"Shubham Kanodia via GitGitGadget\" writes:\n>\n> > +static void apply_prefetch_refspec(struct remote *remote, struct refspec *rs)\n> > +{\n> > + if (remote->prefetch_refs.nr > 0) {\n> > + int i;\n> > + for (i = 0; i < remote->prefetch_refs.nr; i++) {\n> > + const char *src = remote->prefetch_refs.items[i].string;\n> > + struct strbuf dst = STRBUF_INIT;\n> > +\n> > + strbuf_addf(&dst, \"refs/prefetch/%s/\", remote->name);\n> > + if (starts_with(src, \"refs/heads/\")) {\n> > + strbuf_addstr(&dst, src + 11);\n> > + } else if (starts_with(src, \"refs/\")) {\n> > + strbuf_addstr(&dst, src + 5);\n> > + } else {\n> > + strbuf_addstr(&dst, src);\n> > + }\n> > +\n> > + refspec_appendf(rs, \"%s:%s\", src, dst.buf);\n> > + strbuf_release(&dst);\n> > + }\n> > + }\n> > +}\n> > static void filter_prefetch_refspec(struct refspec *rs)\n> > {\n> > int i;\n> > @@ -502,8 +526,11 @@ static struct ref *get_ref_map(struct remote *remote,\n> > int existing_refs_populated = 0;\n> >\n> > filter_prefetch_refspec(rs);\n> > - if (remote)\n> > + if (remote) {\n> > filter_prefetch_refspec(&remote->fetch);\n> > + if (prefetch)\n> > + apply_prefetch_refspec(remote, rs);\n> > + }\n>\n> Hmph, a separate helper function with a separate loop was something\n> I did not expect to see. Looking at the filter_prefetch_refspec(),\n> it already limits what it prefetched to what we usually fetch from\n> the remote, and filteres out the tag namespace. I was hoping that\n> this will _extend_ that existing mechanism, as if by default we\n> have prefetch refspec \"!refs/tags/*\", which can be replaced by the\n> configuration to say \"!refs/tags/* !refs/changes/*\", or positive\n> ones like \"refs/heads/*\". The filtering semantics should be\n>\n> * a refspec whose src matches negated pattern (like !refs/tags/*)\n> is rejected.\n>\n> * if the prefetch_refs has *only* positive patterns, then a refspec\n> whose src does not match *any* of the pattern is rejected.\n>\n> * a refspec that is not rejected is prefetched.\n>\n> But the above still allows what filter_prefetch_refspec() does by\n> default, without any way to narrow it down, and then adds its own\n> entries.\n>\n> This is a half-tangent, but while studying for this topic, I noticed\n> that filter_prefetch_refspec() triggers O(n) loop every time a fetch\n> refspec is skipped.\n>\n> It all comes from 2e03115d (fetch: add --prefetch option,\n> 2021-04-16) but rewriting the loop to use two pointers into the\n> array seemed trivial and the result seemed a bit more readable.\n>\n> Your \"further limit the prefetched refs with configuration\" feature\n> would probably replace this part of the updated code:\n>\n> + /* skip ones that do not store, or store in refs/tags */\n> + if (!rs->items[scan].dst ||\n> + (rs->items[scan].src &&\n> + starts_with(rs->items[scan].src,\n> + ref_namespace[NAMESPACE_TAGS].ref))) {\n>\n> That is, instead of \"skip ones that do not store, or store in refs/tags\",\n> filtering using the configured value (probably implemented as a helper\n> function) would be used as the condition of the if statement.\n>\n> Thoughts?\n>\n> builtin/fetch.c | 46 ++++++++++++++++++++++++----------------------\n> 1 file changed, 24 insertions(+), 22 deletions(-)\n>\n> diff --git c/builtin/fetch.c w/builtin/fetch.c\n> index 693f02b958..219302ed67 100644\n> --- c/builtin/fetch.c\n> +++ w/builtin/fetch.c\n> @@ -436,37 +436,38 @@ static void find_non_local_tags(const struct ref *refs,\n>\n> static void filter_prefetch_refspec(struct refspec *rs)\n> {\n> - int i;\n> + int scan, store;\n>\n> if (!prefetch)\n> return;\n>\n> - for (i = 0; i < rs->nr; i++) {\n> + /*\n> + * scan refspec items with 'scan', and decide to either\n> + * mangle and store it in 'store', or omit it from the result.\n> + */\n> + for (scan = store = 0; scan < rs->nr; scan++, store++) {\n> struct strbuf new_dst = STRBUF_INIT;\n> char *old_dst;\n> const char *sub = NULL;\n>\n> - if (rs->items[i].negative)\n> - continue;\n> - if (!rs->items[i].dst ||\n> - (rs->items[i].src &&\n> - starts_with(rs->items[i].src,\n> - ref_namespace[NAMESPACE_TAGS].ref))) {\n> - int j;\n> -\n> - free(rs->items[i].src);\n> - free(rs->items[i].dst);\n> -\n> - for (j = i + 1; j < rs->nr; j++) {\n> - rs->items[j - 1] = rs->items[j];\n> - rs->raw[j - 1] = rs->raw[j];\n> - }\n> - rs->nr--;\n> - i--;\n> + /* negative ones are kept as-is */\n> + if (rs->items[scan].negative) {\n> + if (store != scan)\n> + rs->items[store] = rs->items[scan];\n> continue;\n> }\n>\n> - old_dst = rs->items[i].dst;\n> + /* skip ones that do not store, or store in refs/tags */\n> + if (!rs->items[scan].dst ||\n> + (rs->items[scan].src &&\n> + starts_with(rs->items[scan].src,\n> + ref_namespace[NAMESPACE_TAGS].ref))) {\n> + refspec_item_clear(&rs->items[scan]);\n> + store--; /* compensate for loop's auto increment */\n> + continue;\n> + }\n> +\n> + old_dst = rs->items[scan].dst;\n> strbuf_addstr(&new_dst, ref_namespace[NAMESPACE_PREFETCH].ref);\n>\n> /*\n> @@ -478,11 +479,12 @@ static void filter_prefetch_refspec(struct refspec *rs)\n> sub = old_dst;\n> strbuf_addstr(&new_dst, sub);\n>\n> - rs->items[i].dst = strbuf_detach(&new_dst, NULL);\n> - rs->items[i].force = 1;\n> + rs->items[store].dst = strbuf_detach(&new_dst, NULL);\n> + rs->items[store].force = 1;\n>\n> free(old_dst);\n> }\n> + rs->nr = store;\n> }\n>\n> static struct ref *get_ref_map(struct remote *remote,\n\nHow should we handle the related `remote..fetch` config?\nIn an earlier discussion, it was discussed that —\n`.prefetchref` should override `.fetch` completely (instead of\npatching it) which makes sense to me.\nAt the moment my reading is that `filter_prefetch_refspec` still\nfilters / modifies `remote->fetch`.\n\n```\nif (remote) {\n filter_prefetch_refspec(&remote->fetch);\n}\n```\n\nSo we'll need to clear refspecs and re-populate perhaps?\n"},{"id":"502470","messageId":"xmqq5xr4r818.fsf@gitster.g","threadId":"62083","inReplyTo":"CAG=Um+0GvFzdAZrCgoS52xh9DF2pntQ+7i+vqYMFQf-MWr3H5A@mail.gmail.com","subject":"Re: [PATCH] remote: introduce config to set prefetch refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-09T18:33:55Z","receivedAt":"2024-09-09T18:33:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shubham Kanodia writes:\n\n> How should we handle the related `remote..fetch` config?\n\nThe get_ref_map() helper is what the rest of the code interacts with\nconfigured refspec. remote->fetch is handled there and goes through\nthe same filter_prefetch_refspec().\n\n> In an earlier discussion, it was discussed that —\n> `.prefetchref` should override `.fetch` completely (instead of\n> patching it) which makes sense to me.\n\nMaybe it made sense to you back when it was discussed, but after\nseeing the current code (before applying this patch), specifically\nwhat happens in filter_prefetch_refspec(), it no longer makes much\nsense to me.\n\nEspecially it is nonsense to allow .prefetchref to widen the set of\nrefs that are being prefetched beyond what is normally fetched, so\nwe should look at a design that easily allows such a configuration\nwith strong suspicion, I would think.\n\n"},{"id":"502736","messageId":"CAG=Um+3WSckyZ2P2o2igQr4hbMyMNTDZ_kqjrfdufvL6hUhMjA@mail.gmail.com","threadId":"62083","inReplyTo":"xmqq5xr4r818.fsf@gitster.g","subject":"Re: [PATCH] remote: introduce config to set prefetch refs","fromName":"Shubham Kanodia","fromEmail":"shubham.kanodia10@gmail.com","sentAt":"2024-09-13T06:16:24Z","receivedAt":"2024-09-13T06:17:01Z","isPatch":true,"sender":{"key":"shubham.kanodia10@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8946207?v=4"},"body":"On Tue, Sep 10, 2024 at 12:03 AM Junio C Hamano wrote:\n>\n> Shubham Kanodia writes:\n>\n> > How should we handle the related `remote..fetch` config?\n>\n> The get_ref_map() helper is what the rest of the code interacts with\n> configured refspec. remote->fetch is handled there and goes through\n> the same filter_prefetch_refspec().\n>\n> > In an earlier discussion, it was discussed that —\n> > `.prefetchref` should override `.fetch` completely (instead of\n> > patching it) which makes sense to me.\n>\n> Maybe it made sense to you back when it was discussed, but after\n> seeing the current code (before applying this patch), specifically\n> what happens in filter_prefetch_refspec(), it no longer makes much\n> sense to me.\n>\n> Especially it is nonsense to allow .prefetchref to widen the set of\n> refs that are being prefetched beyond what is normally fetched, so\n> we should look at a design that easily allows such a configuration\n> with strong suspicion, I would think.\n\nIdeally, a repository should be able to specify (say):\n\nremote.origin.fetch=+refs/heads/*:refs/remotes/origin/*\nremote.origin.prefetchref=refs/heads/main\n\nThis configuration would maintain the normal behavior for fetches, but\nonly prefetch the main branch.\nThe rationale for this is that the main branch typically serves as the\nHEAD from which future branches will be forked in an active\nrepository.\n\nRegarding:\n\n> If prefetch_refs contains only positive patterns, then a refspec whose source\n> doesn't match any of these patterns is rejected.\n\nSimply rejecting a source refspec pattern in `remote..fetch`\nwouldn't achieve our goal here.\nIdeally, we'd need to create a subset of it.\n\nWhat we're looking for is essentially a pattern intersection between\nthe (fetch) `refs/heads/*` and the (prefetchref) `refs/heads/main`,\nwhich in this case would result in `refs/heads/main`.\nHowever, if I understand correctly, performing such pattern\nintersections isn't straightforward in the `filter_prefetch_refspec`\nfunction (let me know if there' prior art for pattern intersection)\n\nI also believe that allowing negative refs might complicate things\nwithout providing a clear use case.\nFor instance, how would we handle the intersection of `fetch` and\n`prefetchref` if `prefetchref` contained both positive and negative\npatterns?\n\nGiven that both `fetch` and `prefetchref` could have multiple\npatterns, it might be simpler and more intuitive for users if we adopt\nan \"A wins over B\" approach.\nHowever, I'm interested in hearing your thoughts on this matter.\n\nPerhaps I should link to the earlier discussion here —\nMessage ID (CAG=Um+1wTbXn_RN+LOCrpZpSNR_QF582PszxNyhz5anVHtBp+w@mail.gmail.com)\n"},{"id":"502758","messageId":"xmqqplp7ze0h.fsf@gitster.g","threadId":"62083","inReplyTo":"CAG=Um+3WSckyZ2P2o2igQr4hbMyMNTDZ_kqjrfdufvL6hUhMjA@mail.gmail.com","subject":"Re: [PATCH] remote: introduce config to set prefetch refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-13T16:58:54Z","receivedAt":"2024-09-13T16:58:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shubham Kanodia writes:\n\n> Ideally, a repository should be able to specify (say):\n>\n> remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*\n> remote.origin.prefetchref=refs/heads/main\n>\n> This configuration would maintain the normal behavior for fetches, but\n> only prefetch the main branch.\n> The rationale for this is that the main branch typically serves as the\n> HEAD from which future branches will be forked in an active\n> repository.\n\nOh, that is 100% agreeable. All I wanted to caution you about was\nwhat should happen when remote.origin.prefetchref in the above is\nreplaced to something like:\n\n [remote \"origin\"]\n\tfetch = +refs/heads/*:refs/remotes/origin/*\n prefetchref = refs/notes/*\n\nThat is, if your refspec used for your real fetch (i.e. \"git fetch\"\nwithout the \"--prefetch\" option) does not fetch anything from\n\"refs/notes/\" hierarchy, prefetching from the hierarchy does not\nhelp the real fetch. I do not have a strong preference between\nmarking it as an error and silently ignoring the prefetch but\nleaning towards the latter, and that is why my suggestion to\nimplement this new \"prefetchref\" as something that extends the\nexisting filter_prefetch_refspec(), which already filters out\nrefspec that fetches from the refs/tags/ namespace (and the ones\nthat do not store by having NULL in the .dst side).\n\n> Regarding:\n>\n>> If prefetch_refs contains only positive patterns, then a refspec whose source\n>> doesn't match any of these patterns is rejected.\n>\n> Simply rejecting a source refspec pattern in `remote..fetch`\n> wouldn't achieve our goal here.\n\nI used the verb \"reject\" to mean \"filter out\", just like a refspec\nwith left-hand-side that begins with \"refs/tags/\" is filtered out\nin the current filter_prefetch_refspec(). And that is exactly what\nwe want to achieve our goal here.\n\nIOW, you would\n\n * read their ref advertisement, and pick only the ones that have a\n matching pattern in the left-hand-side of a remote.$name.fetch\n element. With a more recent protocol, remote.$name.fetch may\n have already participated in narrowing what they advertise to\n begin with, but the end result is the same.\n\n * give it to filter_prefetch_refspec().\n\n * filter_prefetch_refspec() inspects the refspec elements, and\n rejects ones with no right-hand-side, and ones with\n left-hand-side that begin with refs/tags/. The current code\n without your patch already works this way up to this point.\n\n * We extend the above filtering so that in addition to the two\n kinds we currently reject, reject the ones that do not match the\n prefetchref criteria. This is what is needed to implement\n \"prefetchref configuration limits the set of refs that get\n prefetched\".\n\nAnd what you quoted is a beginning of how \"prefetchref configuration\nlimits\". It cannot be \"add to what filter_prefetch_refspec() did\",\nlike done by the implementation in the patch we are discussing.\n\nIf your configuration were this:\n\n [remote \"origin\"]\n fetch = +refs/heads/*:refs/remotes/origin/*\n\nyou would want a way to say things like\n\n (1) I want to prefetch everything I usually fetch\n\n (2) Among the ones I usually fetch, I only want to prefetch master\n and next branches.\n\n (3) I want to prefetch only refs/heads/jk/* branches, but not\n refs/heads/jk/wip/* branches.\n\n (4) I want to prefetch everything I usually fetch, except for\n refs/heads/wip/* branches.\n\nThe case (1) is the simplest. You will leave .prefetchref empty.\n\nFor the case (2), you would write something like\n\n [remote \"origin\"]\n\tprefetchref = refs/heads/master\n\tprefetchref = refs/heads/next\n\nSo, when your prefetchref has all positive patterns, after the\nexisting conditional in filter_prefetch_refspec() passes a refspec\nwhose right-hand-side (i.e., .dst) is not NULL and whose\nleft-hand-side (i.e., .src) does not begin with \"refs/tags/\", you\nfurther inspect and make sure it matches one of these prefetchref\npatterns. In example (2), if they advertised master, next, and seen\nbranches, refs/heads/seen would be filtered out because it matches\nneither of the two patterns, so we would end up prefetching master\nand next branches.\n\nFor the case (3), you would want to say something like\n\n [remote \"origin\"]\n\tprefetchref = refs/heads/jk/*\n\tprefetchref = !refs/heads/jk/wip/*\n\nNow your prefetchref has some negative pattern. When filtering what\nthe existing conditional in filter_prefetch_refspec() passed, you'd\ninspect the refspec element and see if it matches any of the\npositive patterns, and also if it does not match any of the negative\nones. refs/heads/next does not match any positive ones and gets\nrejected. refs/heads/jk/main does match the positive pattern\n'refs/heads/jk/*', and it does not match the negative pattern\n'refs/heads/jk/wip/*', so it passes and will get prefetched.\n\nFor the case (4), you would write something like\n\n [remote \"origin\"]\n\tprefetchref = !refs/heads/wip/*\n\nThere is no positive pattern, so if you blindly apply the rule you\nused for (3) above, everything will get rejected, which is not what\nyou want. refs/heads/main does not match any positive patterns\n(because there are no positive patterns given), but it does not\nmatch any negative ones, so it passes and will get prefetched.\n\nThe condition to implement the above four cases (which I think\ncovers all the cases we care about, but I won't guarantee it is\nexhaustive---you'd need to sanity check) would be\n\n - If there is 1 or more positive prefetchref patterns, the refspec\n element must match one of them to be considered for the next\n rule. Otherwise, it will not be prefetched.\n\n - If the refspec element matches any of negative prefetchref\n patterns, it will not be prefetched.\n\n"},{"id":"502795","messageId":"CAG=Um+2e7kSL8wGFJcJtFAJt8AxyNwpemJNnxDNfsrm1SVVw1Q@mail.gmail.com","threadId":"62083","inReplyTo":"xmqqplp7ze0h.fsf@gitster.g","subject":"Re: [PATCH] remote: introduce config to set prefetch refs","fromName":"Shubham Kanodia","fromEmail":"shubham.kanodia10@gmail.com","sentAt":"2024-09-14T19:35:48Z","receivedAt":"2024-09-14T19:36:26Z","isPatch":true,"sender":{"key":"shubham.kanodia10@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8946207?v=4"},"body":"On Fri, Sep 13, 2024 at 10:28 PM Junio C Hamano wrote:\n>\n> Shubham Kanodia writes:\n>\n> > Ideally, a repository should be able to specify (say):\n> >\n> > remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*\n> > remote.origin.prefetchref=refs/heads/main\n> >\n> > This configuration would maintain the normal behavior for fetches, but\n> > only prefetch the main branch.\n> > The rationale for this is that the main branch typically serves as the\n> > HEAD from which future branches will be forked in an active\n> > repository.\n>\n> Oh, that is 100% agreeable. All I wanted to caution you about was\n> what should happen when remote.origin.prefetchref in the above is\n> replaced to something like:\n>\n> [remote \"origin\"]\n> fetch = +refs/heads/*:refs/remotes/origin/*\n> prefetchref = refs/notes/*\n>\n> That is, if your refspec used for your real fetch (i.e. \"git fetch\"\n> without the \"--prefetch\" option) does not fetch anything from\n> \"refs/notes/\" hierarchy, prefetching from the hierarchy does not\n> help the real fetch. I do not have a strong preference between\n> marking it as an error and silently ignoring the prefetch but\n> leaning towards the latter, and that is why my suggestion to\n> implement this new \"prefetchref\" as something that extends the\n> existing filter_prefetch_refspec(), which already filters out\n> refspec that fetches from the refs/tags/ namespace (and the ones\n> that do not store by having NULL in the .dst side).\n>\n> > Regarding:\n> >\n> >> If prefetch_refs contains only positive patterns, then a refspec whose source\n> >> doesn't match any of these patterns is rejected.\n> >\n> > Simply rejecting a source refspec pattern in `remote..fetch`\n> > wouldn't achieve our goal here.\n>\n> I used the verb \"reject\" to mean \"filter out\", just like a refspec\n> with left-hand-side that begins with \"refs/tags/\" is filtered out\n> in the current filter_prefetch_refspec(). And that is exactly what\n> we want to achieve our goal here.\n>\n> IOW, you would\n>\n> * read their ref advertisement, and pick only the ones that have a\n> matching pattern in the left-hand-side of a remote.$name.fetch\n> element. With a more recent protocol, remote.$name.fetch may\n> have already participated in narrowing what they advertise to\n> begin with, but the end result is the same.\n>\n> * give it to filter_prefetch_refspec().\n>\n> * filter_prefetch_refspec() inspects the refspec elements, and\n> rejects ones with no right-hand-side, and ones with\n> left-hand-side that begin with refs/tags/. The current code\n> without your patch already works this way up to this point.\n>\n> * We extend the above filtering so that in addition to the two\n> kinds we currently reject, reject the ones that do not match the\n> prefetchref criteria. This is what is needed to implement\n> \"prefetchref configuration limits the set of refs that get\n> prefetched\".\n>\n> And what you quoted is a beginning of how \"prefetchref configuration\n> limits\". It cannot be \"add to what filter_prefetch_refspec() did\",\n> like done by the implementation in the patch we are discussing.\n>\n> If your configuration were this:\n>\n> [remote \"origin\"]\n> fetch = +refs/heads/*:refs/remotes/origin/*\n>\n> you would want a way to say things like\n>\n> (1) I want to prefetch everything I usually fetch\n>\n> (2) Among the ones I usually fetch, I only want to prefetch master\n> and next branches.\n>\n> (3) I want to prefetch only refs/heads/jk/* branches, but not\n> refs/heads/jk/wip/* branches.\n>\n> (4) I want to prefetch everything I usually fetch, except for\n> refs/heads/wip/* branches.\n>\n> The case (1) is the simplest. You will leave .prefetchref empty.\n>\n> For the case (2), you would write something like\n>\n> [remote \"origin\"]\n> prefetchref = refs/heads/master\n> prefetchref = refs/heads/next\n>\n> So, when your prefetchref has all positive patterns, after the\n> existing conditional in filter_prefetch_refspec() passes a refspec\n> whose right-hand-side (i.e., .dst) is not NULL and whose\n> left-hand-side (i.e., .src) does not begin with \"refs/tags/\", you\n> further inspect and make sure it matches one of these prefetchref\n> patterns. In example (2), if they advertised master, next, and seen\n> branches, refs/heads/seen would be filtered out because it matches\n> neither of the two patterns, so we would end up prefetching master\n> and next branches.\n>\n> For the case (3), you would want to say something like\n>\n> [remote \"origin\"]\n> prefetchref = refs/heads/jk/*\n> prefetchref = !refs/heads/jk/wip/*\n>\n> Now your prefetchref has some negative pattern. When filtering what\n> the existing conditional in filter_prefetch_refspec() passed, you'd\n> inspect the refspec element and see if it matches any of the\n> positive patterns, and also if it does not match any of the negative\n> ones. refs/heads/next does not match any positive ones and gets\n> rejected. refs/heads/jk/main does match the positive pattern\n> 'refs/heads/jk/*', and it does not match the negative pattern\n> 'refs/heads/jk/wip/*', so it passes and will get prefetched.\n>\n> For the case (4), you would write something like\n>\n> [remote \"origin\"]\n> prefetchref = !refs/heads/wip/*\n>\n> There is no positive pattern, so if you blindly apply the rule you\n> used for (3) above, everything will get rejected, which is not what\n> you want. refs/heads/main does not match any positive patterns\n> (because there are no positive patterns given), but it does not\n> match any negative ones, so it passes and will get prefetched.\n>\n> The condition to implement the above four cases (which I think\n> covers all the cases we care about, but I won't guarantee it is\n> exhaustive---you'd need to sanity check) would be\n>\n> - If there is 1 or more positive prefetchref patterns, the refspec\n> element must match one of them to be considered for the next\n> rule. Otherwise, it will not be prefetched.\n>\n> - If the refspec element matches any of negative prefetchref\n> patterns, it will not be prefetched.\n>\n\nIf we're trying to determine if a pattern\n(remote..prefetchref) is a subset of another or not\n(remote..fetch) (to not accidentally expand the scope beyond\n`fetch`),\nwe'd need a function that does that pattern-to-pattern. Are you aware\nof any existing functions that do so?\n\nI see `wildmatch`, but that is only used for pattern to full-text comparisons.\n"},{"id":"502796","messageId":"xmqqo74qro6f.fsf@gitster.g","threadId":"62083","inReplyTo":"CAG=Um+2e7kSL8wGFJcJtFAJt8AxyNwpemJNnxDNfsrm1SVVw1Q@mail.gmail.com","subject":"Re: [PATCH] remote: introduce config to set prefetch refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-14T20:11:04Z","receivedAt":"2024-09-14T20:11:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shubham Kanodia writes:\n\n> If we're trying to determine if a pattern\n> (remote..prefetchref) is a subset of another or not\n> (remote..fetch) (to not accidentally expand the scope beyond\n> `fetch`),\n> we'd need a function that does that pattern-to-pattern. Are you aware\n> of any existing functions that do so?\n\nThere is no such computation for this application. Such a\ncomputation might become needed if you wanted to complain that the\nuser gave .prefetchref pattern that would never match what .fetch\npatterns would allow to pass. But there is no such need.\n\nYou will first get the advertised refs from the remote. \n\nExisting logic filteres them down to what matches configured\nremote.$name.fetch variable. filter_prefetch_refspec() may further\nreduces the result by removing those whose .src side begins with\n\"refs/tags/\".\n\nNow you only look at what survived the above existing filtering, and\nfurther narrow it down by picking only ones that match the prefetch\ncondition. If the refspec that survived the filtering by the fetch\nrefspec (and existing logic in filter_prefetch_refspec()) does not\nsatisfy the prefetch condition, it won't be prefetched.\n\nSince you are using .prefetch ONLY TO narrow the result down, by\ndefinition, you are not adding anything what .fetch configuration\nwould not have fetched.\n\n\n"},{"id":"502806","messageId":"CAG=Um+2DiQswQr86zhT3_quO3eYH8EBpwVNQir_SvMWwAsPk5g@mail.gmail.com","threadId":"62083","inReplyTo":"xmqqo74qro6f.fsf@gitster.g","subject":"Re: [PATCH] remote: introduce config to set prefetch refs","fromName":"Shubham Kanodia","fromEmail":"shubham.kanodia10@gmail.com","sentAt":"2024-09-15T14:06:11Z","receivedAt":"2024-09-15T14:06:48Z","isPatch":true,"sender":{"key":"shubham.kanodia10@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8946207?v=4"},"body":"On Sun, Sep 15, 2024 at 1:41 AM Junio C Hamano wrote:\n>\n> Shubham Kanodia writes:\n>\n> > If we're trying to determine if a pattern\n> > (remote..prefetchref) is a subset of another or not\n> > (remote..fetch) (to not accidentally expand the scope beyond\n> > `fetch`),\n> > we'd need a function that does that pattern-to-pattern. Are you aware\n> > of any existing functions that do so?\n>\n> There is no such computation for this application. Such a\n> computation might become needed if you wanted to complain that the\n> user gave .prefetchref pattern that would never match what .fetch\n> patterns would allow to pass. But there is no such need.\n>\n> You will first get the advertised refs from the remote.\n>\n> Existing logic filteres them down to what matches configured\n> remote.$name.fetch variable. filter_prefetch_refspec() may further\n> reduces the result by removing those whose .src side begins with\n> \"refs/tags/\".\n>\n> Now you only look at what survived the above existing filtering, and\n> further narrow it down by picking only ones that match the prefetch\n> condition. If the refspec that survived the filtering by the fetch\n> refspec (and existing logic in filter_prefetch_refspec()) does not\n> satisfy the prefetch condition, it won't be prefetched.\n>\n> Since you are using .prefetch ONLY TO narrow the result down, by\n> definition, you are not adding anything what .fetch configuration\n> would not have fetched.\n>\n>\n\nAh I see — I assumed you expected all filtering for `prefetch`\n(existing & new) to happen inside `filter_prefetch_refspec`.\nBut that threw me off, because `filter_prefetch_refspec` doesn't deal\nwith advertised refs from remote, and only patterns.\n\nLet me know if the diff in the following mail is closer to what you\nwere expecting?\n"},{"id":"502807","messageId":"pull.1782.v2.git.1726409329993.gitgitgadget@gmail.com","threadId":"62083","inReplyTo":"pull.1782.git.1725875232922.gitgitgadget@gmail.com","subject":"[PATCH v2] remote: introduce config to set prefetch refs","fromName":"Shubham Kanodia via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-15T14:08:49Z","receivedAt":"2024-09-15T14:08:54Z","isPatch":true,"sender":{"key":"shubham.kanodia10@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8946207?v=4"},"body":"From: Shubham Kanodia \n\nThis commit introduces a new configuration option,\nremote..prefetchref, which allows users to specify specific\nref patterns to be prefetched during a git fetch --prefetch\noperation.\n\nThe new option accepts a space-separated list of ref patterns.\nWhen the --prefetch option is used with git fetch, only the refs\nmatching these patterns will be prefetched, instead of the\ndefault behavior of prefetching all fetchable refs.\n\nExample usage in .git/config:\n[remote \"origin\"]\n prefetchref = \"refs/heads/main refs/heads/feature/*\"\n\nThis change allows users to optimize their prefetch operations, potentially\nreducing network traffic and improving performance for large repositories\nwith many refs.\n\nSigned-off-by: Shubham Kanodia \n---\n remote: introduce config to set prefetch refs\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1782%2Fpastelsky%2Fsk%2Fremote-prefetchref-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1782/pastelsky/sk/remote-prefetchref-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1782\n\nRange-diff vs v1:\n\n 1: 3113d8b8635 ! 1: f9f9e637bfa remote: introduce config to set prefetch refs\n @@ Documentation/config/remote.txt: remote..fetch::\n \tlinkgit:git-push[1].\n \n ## builtin/fetch.c ##\n -@@ builtin/fetch.c: static void find_non_local_tags(const struct ref *refs,\n - \toidset_clear(&fetch_oids);\n +@@\n + #include \"trace.h\"\n + #include \"trace2.h\"\n + #include \"bundle-uri.h\"\n ++#include \"wildmatch.h\"\n + \n + #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n + \n +@@ builtin/fetch.c: static void filter_prefetch_refspec(struct refspec *rs)\n + \t}\n }\n \n -+static void apply_prefetch_refspec(struct remote *remote, struct refspec *rs)\n ++static int matches_prefetch_refs(const char *refname, const struct string_list *prefetch_refs)\n +{\n -+\tif (remote->prefetch_refs.nr > 0) {\n -+\t\tint i;\n -+\t\tfor (i = 0; i < remote->prefetch_refs.nr; i++) {\n -+\t\t\tconst char *src = remote->prefetch_refs.items[i].string;\n -+\t\t\tstruct strbuf dst = STRBUF_INIT;\n -+\n -+\t\t\tstrbuf_addf(&dst, \"refs/prefetch/%s/\", remote->name);\n -+\t\t\tif (starts_with(src, \"refs/heads/\")) {\n -+\t\t\t\tstrbuf_addstr(&dst, src + 11);\n -+\t\t\t} else if (starts_with(src, \"refs/\")) {\n -+\t\t\t\tstrbuf_addstr(&dst, src + 5);\n -+\t\t\t} else {\n -+\t\t\t\tstrbuf_addstr(&dst, src);\n -+\t\t\t}\n ++\tint i;\n ++\tint has_positive = 0;\n ++\tint matched_positive = 0;\n ++\tint matched_negative = 0;\n ++\n ++\tfor (i = 0; i < prefetch_refs->nr; i++) {\n ++\t\tconst char *pattern = prefetch_refs->items[i].string;\n ++\t\tint is_negative = (*pattern == '!');\n ++\n ++\t\tif (is_negative)\n ++\t\t\tpattern++;\n ++\t\telse\n ++\t\t\thas_positive = 1;\n +\n -+\t\t\trefspec_appendf(rs, \"%s:%s\", src, dst.buf);\n -+\t\t\tstrbuf_release(&dst);\n ++\t\tif (wildmatch(pattern, refname, 0) == 0) {\n ++\t\t\tif (is_negative)\n ++\t\t\t\tmatched_negative = 1;\n ++\t\t\telse\n ++\t\t\t\tmatched_positive = 1;\n +\t\t}\n +\t}\n ++\n ++\tif (!has_positive)\n ++\t\treturn !matched_negative;\n ++\n ++\treturn matched_positive && !matched_negative;\n +}\n +\n +\n - static void filter_prefetch_refspec(struct refspec *rs)\n - {\n - \tint i;\n ++static void ref_remove(struct ref **head, struct ref *to_remove)\n ++{\n ++\tstruct ref **pp, *p;\n ++\n ++\tfor (pp = head; (p = *pp) != NULL; pp = &p->next) {\n ++\t\tif (p == to_remove) {\n ++\t\t\t*pp = p->next;\n ++\t\t\treturn;\n ++\t\t}\n ++\t}\n ++}\n ++\n + static struct ref *get_ref_map(struct remote *remote,\n + \t\t\t const struct ref *remote_refs,\n + \t\t\t struct refspec *rs,\n @@ builtin/fetch.c: static struct ref *get_ref_map(struct remote *remote,\n \tint existing_refs_populated = 0;\n \n \tfilter_prefetch_refspec(rs);\n --\tif (remote)\n -+\tif (remote) {\n ++\n + \tif (remote)\n \t\tfilter_prefetch_refspec(&remote->fetch);\n -+\t\tif (prefetch)\n -+\t\t\tapply_prefetch_refspec(remote, rs);\n + \n +@@ builtin/fetch.c: static struct ref *get_ref_map(struct remote *remote,\n + \telse\n + \t\tref_map = apply_negative_refspecs(ref_map, &remote->fetch);\n + \n ++\t/**\n ++\t * Filter out advertised refs that we don't want to fetch during\n ++\t * prefetch if a prefetchref config is set\n ++\t */\n ++\tif (prefetch && remote->prefetch_refs.nr) {\n ++\t\tstruct ref *ref, *next;\n ++\t\tfor (ref = ref_map; ref; ref = next) {\n ++\t\t\tnext = ref->next;\n ++\n ++\t\t\tif (!matches_prefetch_refs(ref->name, &remote->prefetch_refs)) {\n ++\t\t\t\t\tref_remove(&ref_map, ref);\n ++\t\t\t\t\tfree_one_ref(ref);\n ++\t\t\t}\n ++\t\t}\n +\t}\n ++\n + \tref_map = ref_remove_duplicates(ref_map);\n \n - \tif (rs->nr) {\n - \t\tstruct refspec *fetch_refspec;\n + \tfor (rm = ref_map; rm; rm = rm->next) {\n \n ## remote.c ##\n @@ remote.c: static struct remote *make_remote(struct remote_state *remote_state,\n @@ t/t7900-maintenance.sh: test_expect_success 'prefetch multiple remotes' '\n \ttest_subcommand git fetch remote2 $fetchargs .prefetchref refs if present' '\n -+\ttest_create_repo prefetch-test-mixed-patterns &&\n ++test_expect_success 'prefetch with positive prefetch ref patterns' '\n ++\ttest_create_repo filter-prefetch-positive &&\n +\t(\n -+\t\tcd prefetch-test-mixed-patterns &&\n ++\t\tcd filter-prefetch-positive &&\n +\t\ttest_commit initial &&\n -+\t\tgit clone . clone1 &&\n +\t\tgit clone . clone2 &&\n -+\n -+\t\tgit remote add remote1 \"file://$(pwd)/clone1\" &&\n +\t\tgit remote add remote2 \"file://$(pwd)/clone2\" &&\n +\n -+\t\t# Set single prefetchref pattern for remote1 and multiple for remote2\n -+\t\tgit config remote.remote1.prefetchref \"refs/heads/foo\" &&\n -+\t\tgit config remote.remote2.prefetchref \"refs/heads/feature/* refs/heads/topic\" &&\n -+\n -+\t\t# Create branches in clone1 and push\n -+\t\t(\n -+\t\t\tcd clone1 &&\n -+\t\t\tgit checkout -b foo &&\n -+\t\t\ttest_commit foo-commit &&\n -+\t\t\tgit checkout -b feature/a &&\n -+\t\t\ttest_commit feature-a-commit &&\n -+\t\t\tgit checkout -b other &&\n -+\t\t\ttest_commit other-commit &&\n -+\t\t\tgit push origin foo feature/a other\n -+\t\t) &&\n -+\n -+\t\t# Create branches in clone2 and push\n -+\t\t(\n -+\t\t\tcd clone2 &&\n -+\t\t\tgit checkout -b topic &&\n -+\t\t\ttest_commit master-commit &&\n -+\t\t\tgit checkout -b feature/x &&\n -+\t\t\ttest_commit feature-x-commit &&\n -+\t\t\tgit checkout -b feature/y &&\n -+\t\t\ttest_commit feature-y-commit &&\n -+\t\t\tgit checkout -b dev &&\n -+\t\t\ttest_commit dev-commit &&\n -+\t\t\tgit push origin topic feature/x feature/y dev\n -+\t\t) &&\n -+\n -+\t\t# Run maintenance prefetch task\n -+\t\tGIT_TRACE2_EVENT=\"$(pwd)/prefetch.txt\" git maintenance run --task=prefetch 2>/dev/null &&\n -+\n -+\t\t# Check that only specified refs were prefetched\n ++\t\tcd clone2 &&\n ++\t\tgit checkout -b feature && test_commit feature-commit-2 &&\n ++\t\tgit checkout -b wip/test && test_commit wip-test-commit-2 &&\n ++\t\tgit checkout -b topic/x && test_commit topic-x-commit-2 &&\n ++\t\tgit push -f origin feature wip/test topic/x&&\n ++\t\tcd .. &&\n ++\n ++\t\tgit config remote.remote2.prefetchref \"refs/heads/feature\" &&\n +\t\tfetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head --recurse-submodules=no --quiet\" &&\n -+\t\ttest_subcommand git fetch remote1 $fetchargs /dev/null &&\n ++\t\ttest_subcommand git fetch remote2 $fetchargs /dev/null &&\n ++\t\ttest_subcommand git fetch remote3 $fetchargs /dev/null &&\n ++\t\ttest_subcommand git fetch remote4 $fetchargs .fetch::\n \tThe default set of \"refspec\" for linkgit:git-fetch[1]. See\n \tlinkgit:git-fetch[1].\n \n+remote..prefetchref::\n+ Specify the refs to be prefetched when fetching from this remote.\n+ The value is a space-separated list of ref patterns (e.g., \"refs/heads/master refs/heads/develop*\").\n+ These patterns are used as the source part of the refspecs for prefetching.\n+ This can be used to optimize fetch operations by specifying exactly which refs should be prefetched.\n+\n remote..push::\n \tThe default set of \"refspec\" for linkgit:git-push[1]. See\n \tlinkgit:git-push[1].\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b2b5aee5bf2..16c8a31c2e1 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -38,6 +38,7 @@\n #include \"trace.h\"\n #include \"trace2.h\"\n #include \"bundle-uri.h\"\n+#include \"wildmatch.h\"\n \n #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n \n@@ -485,6 +486,49 @@ static void filter_prefetch_refspec(struct refspec *rs)\n \t}\n }\n \n+static int matches_prefetch_refs(const char *refname, const struct string_list *prefetch_refs)\n+{\n+\tint i;\n+\tint has_positive = 0;\n+\tint matched_positive = 0;\n+\tint matched_negative = 0;\n+\n+\tfor (i = 0; i < prefetch_refs->nr; i++) {\n+\t\tconst char *pattern = prefetch_refs->items[i].string;\n+\t\tint is_negative = (*pattern == '!');\n+\n+\t\tif (is_negative)\n+\t\t\tpattern++;\n+\t\telse\n+\t\t\thas_positive = 1;\n+\n+\t\tif (wildmatch(pattern, refname, 0) == 0) {\n+\t\t\tif (is_negative)\n+\t\t\t\tmatched_negative = 1;\n+\t\t\telse\n+\t\t\t\tmatched_positive = 1;\n+\t\t}\n+\t}\n+\n+\tif (!has_positive)\n+\t\treturn !matched_negative;\n+\n+\treturn matched_positive && !matched_negative;\n+}\n+\n+\n+static void ref_remove(struct ref **head, struct ref *to_remove)\n+{\n+\tstruct ref **pp, *p;\n+\n+\tfor (pp = head; (p = *pp) != NULL; pp = &p->next) {\n+\t\tif (p == to_remove) {\n+\t\t\t*pp = p->next;\n+\t\t\treturn;\n+\t\t}\n+\t}\n+}\n+\n static struct ref *get_ref_map(struct remote *remote,\n \t\t\t const struct ref *remote_refs,\n \t\t\t struct refspec *rs,\n@@ -502,6 +546,7 @@ static struct ref *get_ref_map(struct remote *remote,\n \tint existing_refs_populated = 0;\n \n \tfilter_prefetch_refspec(rs);\n+\n \tif (remote)\n \t\tfilter_prefetch_refspec(&remote->fetch);\n \n@@ -610,6 +655,22 @@ static struct ref *get_ref_map(struct remote *remote,\n \telse\n \t\tref_map = apply_negative_refspecs(ref_map, &remote->fetch);\n \n+\t/**\n+\t * Filter out advertised refs that we don't want to fetch during\n+\t * prefetch if a prefetchref config is set\n+\t */\n+\tif (prefetch && remote->prefetch_refs.nr) {\n+\t\tstruct ref *ref, *next;\n+\t\tfor (ref = ref_map; ref; ref = next) {\n+\t\t\tnext = ref->next;\n+\n+\t\t\tif (!matches_prefetch_refs(ref->name, &remote->prefetch_refs)) {\n+\t\t\t\t\tref_remove(&ref_map, ref);\n+\t\t\t\t\tfree_one_ref(ref);\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \tref_map = ref_remove_duplicates(ref_map);\n \n \tfor (rm = ref_map; rm; rm = rm->next) {\ndiff --git a/remote.c b/remote.c\nindex 8f3dee13186..b46d62b2c47 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -141,6 +141,7 @@ static struct remote *make_remote(struct remote_state *remote_state,\n \tret->prune = -1; /* unspecified */\n \tret->prune_tags = -1; /* unspecified */\n \tret->name = xstrndup(name, len);\n+\tstring_list_init_dup(&ret->prefetch_refs);\n \trefspec_init(&ret->push, REFSPEC_PUSH);\n \trefspec_init(&ret->fetch, REFSPEC_FETCH);\n \n@@ -166,6 +167,7 @@ static void remote_clear(struct remote *remote)\n \tfree((char *)remote->uploadpack);\n \tFREE_AND_NULL(remote->http_proxy);\n \tFREE_AND_NULL(remote->http_proxy_authmethod);\n+\tstring_list_clear(&remote->prefetch_refs, 0);\n }\n \n static void add_merge(struct branch *branch, const char *name)\n@@ -456,6 +458,12 @@ static int handle_config(const char *key, const char *value,\n \t\tremote->prune = git_config_bool(key, value);\n \telse if (!strcmp(subkey, \"prunetags\"))\n \t\tremote->prune_tags = git_config_bool(key, value);\n+\telse if (!strcmp(subkey, \"prefetchref\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(key);\n+\t\tstring_list_split(&remote->prefetch_refs, value, ' ', -1);\n+\t\treturn 0;\n+\t}\n \telse if (!strcmp(subkey, \"url\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(key);\ndiff --git a/remote.h b/remote.h\nindex b901b56746d..c18e68e0d8d 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -5,6 +5,7 @@\n #include \"hashmap.h\"\n #include \"refspec.h\"\n #include \"strvec.h\"\n+#include \"string-list.h\"\n \n struct option;\n struct transport_ls_refs_options;\n@@ -77,6 +78,8 @@ struct remote {\n \n \tstruct refspec fetch;\n \n+\tstruct string_list prefetch_refs;\n+\n \t/*\n \t * The setting for whether to fetch tags (as a separate rule from the\n \t * configured refspecs);\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex abae7a97546..5b64257eb7d 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -245,6 +245,86 @@ test_expect_success 'prefetch multiple remotes' '\n \ttest_subcommand git fetch remote2 $fetchargs /dev/null &&\n+\t\ttest_subcommand git fetch remote2 $fetchargs /dev/null &&\n+\t\ttest_subcommand git fetch remote3 $fetchargs /dev/null &&\n+\t\ttest_subcommand git fetch remote4 $fetchargs writes:\n\n> Existing logic filteres them down to what matches configured\n> remote.$name.fetch variable. filter_prefetch_refspec() may further\n> reduces the result by removing those whose .src side begins with\n> \"refs/tags/\".\n>\n> Now you only look at what survived the above existing filtering, and\n> further narrow it down by picking only ones that match the prefetch\n> condition. If the refspec that survived the filtering by the fetch\n> refspec (and existing logic in filter_prefetch_refspec()) does not\n> satisfy the prefetch condition, it won't be prefetched.\n\nSorry, but I misread the code.\n\nBy the time filter_prefetch_refspec() is called by get_ref_map(),\nthis caller has \"remote_refs\" linked list that describes each ref it\nis going to fetch, so conceptually what is left for the prefetch\nlogic to do is to selectively discard the elements on this list that\nare not worth asking the remote to send new object data for and use\nthe remainder of the list in remote_refs list, and the logic to\nfurther limit this list with the prefetchref configuration would fit\nwell here, but filter_prefetch_refspec() does not work on this list\nat all X-<. So the prefetchref limitation needs to come outside the\nfunction.\n"},{"id":"502819","messageId":"CAG=Um+1OQFUuHS0yEV65=D4eW3Ezr8f==WN4umppqS6pFAQRSw@mail.gmail.com","threadId":"62083","inReplyTo":"xmqqbk0osxp0.fsf@gitster.g","subject":"Re: [PATCH] remote: introduce config to set prefetch refs","fromName":"Shubham Kanodia","fromEmail":"shubham.kanodia10@gmail.com","sentAt":"2024-09-16T04:34:56Z","receivedAt":"2024-09-16T04:35:34Z","isPatch":true,"sender":{"key":"shubham.kanodia10@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8946207?v=4"},"body":"On Sun, Sep 15, 2024 at 9:42 PM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > Existing logic filteres them down to what matches configured\n> > remote.$name.fetch variable. filter_prefetch_refspec() may further\n> > reduces the result by removing those whose .src side begins with\n> > \"refs/tags/\".\n> >\n> > Now you only look at what survived the above existing filtering, and\n> > further narrow it down by picking only ones that match the prefetch\n> > condition. If the refspec that survived the filtering by the fetch\n> > refspec (and existing logic in filter_prefetch_refspec()) does not\n> > satisfy the prefetch condition, it won't be prefetched.\n>\n> Sorry, but I misread the code.\n>\n> By the time filter_prefetch_refspec() is called by get_ref_map(),\n> this caller has \"remote_refs\" linked list that describes each ref it\n> is going to fetch, so conceptually what is left for the prefetch\n> logic to do is to selectively discard the elements on this list that\n> are not worth asking the remote to send new object data for and use\n> the remainder of the list in remote_refs list, and the logic to\n> further limit this list with the prefetchref configuration would fit\n> well here, but filter_prefetch_refspec() does not work on this list\n> at all X-<. So the prefetchref limitation needs to come outside the\n> function.\n\nSure, can you take a look at the latest diff (shared in the last reply)?\nMoved the prefetchref filtering to `get_ref_map` where advertised refs\nare available.\n"},{"id":"503052","messageId":"pull.1782.v3.git.1726741439445.gitgitgadget@gmail.com","threadId":"62083","inReplyTo":"pull.1782.v2.git.1726409329993.gitgitgadget@gmail.com","subject":"[PATCH v3] remote: introduce config to set prefetch refs","fromName":"Shubham Kanodia via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-19T10:23:59Z","receivedAt":"2024-09-19T10:24:03Z","isPatch":true,"sender":{"key":"shubham.kanodia10@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8946207?v=4"},"body":"From: Shubham Kanodia \n\nThis commit introduces a new configuration option,\nremote..prefetchref, which allows users to specify specific\nref patterns to be prefetched during a git fetch --prefetch\noperation.\n\nThe new option accepts a space-separated list of ref patterns.\nWhen the --prefetch option is used with git fetch, only the refs\nmatching these patterns will be prefetched, instead of the\ndefault behavior of prefetching all fetchable refs.\n\nExample usage in .git/config:\n[remote \"origin\"]\n prefetchref = \"refs/heads/main refs/heads/feature/*\"\n\nThis change allows users to optimize their prefetch operations, potentially\nreducing network traffic and improving performance for large repositories\nwith many refs.\n\nSigned-off-by: Shubham Kanodia \n---\n remote: introduce config to set prefetch refs\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1782%2Fpastelsky%2Fsk%2Fremote-prefetchref-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1782/pastelsky/sk/remote-prefetchref-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1782\n\nRange-diff vs v2:\n\n 1: f9f9e637bfa ! 1: 717d5957c47 remote: introduce config to set prefetch refs\n @@ Documentation/config/remote.txt: remote..fetch::\n \n +remote..prefetchref::\n + Specify the refs to be prefetched when fetching from this remote.\n -+ The value is a space-separated list of ref patterns (e.g., \"refs/heads/master refs/heads/develop*\").\n -+ These patterns are used as the source part of the refspecs for prefetching.\n ++ The value is a space-separated list of ref patterns (e.g., \"refs/heads/master !refs/heads/develop*\").\n + This can be used to optimize fetch operations by specifying exactly which refs should be prefetched.\n +\n remote..push::\n @@ t/t7900-maintenance.sh: test_expect_success 'prefetch multiple remotes' '\n +\t\tcd .. &&\n +\n +\t\tgit config remote.remote4.prefetchref \"refs/heads/topic/* !refs/heads/topic/y\" &&\n -+\t\t# git config --add remote.remote4.prefetchref \"!refs/topic/y\" &&\n -+\t\tcat .git/config &&\n +\t\tfetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head --recurse-submodules=no --quiet\" &&\n +\t\tGIT_TRACE2_EVENT=\"$(pwd)/prefetch-mixed.txt\" git maintenance run --task=prefetch 2>/dev/null &&\n +\t\ttest_subcommand git fetch remote4 $fetchargs .fetch::\n \tThe default set of \"refspec\" for linkgit:git-fetch[1]. See\n \tlinkgit:git-fetch[1].\n \n+remote..prefetchref::\n+ Specify the refs to be prefetched when fetching from this remote.\n+ The value is a space-separated list of ref patterns (e.g., \"refs/heads/master !refs/heads/develop*\").\n+ This can be used to optimize fetch operations by specifying exactly which refs should be prefetched.\n+\n remote..push::\n \tThe default set of \"refspec\" for linkgit:git-push[1]. See\n \tlinkgit:git-push[1].\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b2b5aee5bf2..16c8a31c2e1 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -38,6 +38,7 @@\n #include \"trace.h\"\n #include \"trace2.h\"\n #include \"bundle-uri.h\"\n+#include \"wildmatch.h\"\n \n #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n \n@@ -485,6 +486,49 @@ static void filter_prefetch_refspec(struct refspec *rs)\n \t}\n }\n \n+static int matches_prefetch_refs(const char *refname, const struct string_list *prefetch_refs)\n+{\n+\tint i;\n+\tint has_positive = 0;\n+\tint matched_positive = 0;\n+\tint matched_negative = 0;\n+\n+\tfor (i = 0; i < prefetch_refs->nr; i++) {\n+\t\tconst char *pattern = prefetch_refs->items[i].string;\n+\t\tint is_negative = (*pattern == '!');\n+\n+\t\tif (is_negative)\n+\t\t\tpattern++;\n+\t\telse\n+\t\t\thas_positive = 1;\n+\n+\t\tif (wildmatch(pattern, refname, 0) == 0) {\n+\t\t\tif (is_negative)\n+\t\t\t\tmatched_negative = 1;\n+\t\t\telse\n+\t\t\t\tmatched_positive = 1;\n+\t\t}\n+\t}\n+\n+\tif (!has_positive)\n+\t\treturn !matched_negative;\n+\n+\treturn matched_positive && !matched_negative;\n+}\n+\n+\n+static void ref_remove(struct ref **head, struct ref *to_remove)\n+{\n+\tstruct ref **pp, *p;\n+\n+\tfor (pp = head; (p = *pp) != NULL; pp = &p->next) {\n+\t\tif (p == to_remove) {\n+\t\t\t*pp = p->next;\n+\t\t\treturn;\n+\t\t}\n+\t}\n+}\n+\n static struct ref *get_ref_map(struct remote *remote,\n \t\t\t const struct ref *remote_refs,\n \t\t\t struct refspec *rs,\n@@ -502,6 +546,7 @@ static struct ref *get_ref_map(struct remote *remote,\n \tint existing_refs_populated = 0;\n \n \tfilter_prefetch_refspec(rs);\n+\n \tif (remote)\n \t\tfilter_prefetch_refspec(&remote->fetch);\n \n@@ -610,6 +655,22 @@ static struct ref *get_ref_map(struct remote *remote,\n \telse\n \t\tref_map = apply_negative_refspecs(ref_map, &remote->fetch);\n \n+\t/**\n+\t * Filter out advertised refs that we don't want to fetch during\n+\t * prefetch if a prefetchref config is set\n+\t */\n+\tif (prefetch && remote->prefetch_refs.nr) {\n+\t\tstruct ref *ref, *next;\n+\t\tfor (ref = ref_map; ref; ref = next) {\n+\t\t\tnext = ref->next;\n+\n+\t\t\tif (!matches_prefetch_refs(ref->name, &remote->prefetch_refs)) {\n+\t\t\t\t\tref_remove(&ref_map, ref);\n+\t\t\t\t\tfree_one_ref(ref);\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \tref_map = ref_remove_duplicates(ref_map);\n \n \tfor (rm = ref_map; rm; rm = rm->next) {\ndiff --git a/remote.c b/remote.c\nindex 8f3dee13186..b46d62b2c47 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -141,6 +141,7 @@ static struct remote *make_remote(struct remote_state *remote_state,\n \tret->prune = -1; /* unspecified */\n \tret->prune_tags = -1; /* unspecified */\n \tret->name = xstrndup(name, len);\n+\tstring_list_init_dup(&ret->prefetch_refs);\n \trefspec_init(&ret->push, REFSPEC_PUSH);\n \trefspec_init(&ret->fetch, REFSPEC_FETCH);\n \n@@ -166,6 +167,7 @@ static void remote_clear(struct remote *remote)\n \tfree((char *)remote->uploadpack);\n \tFREE_AND_NULL(remote->http_proxy);\n \tFREE_AND_NULL(remote->http_proxy_authmethod);\n+\tstring_list_clear(&remote->prefetch_refs, 0);\n }\n \n static void add_merge(struct branch *branch, const char *name)\n@@ -456,6 +458,12 @@ static int handle_config(const char *key, const char *value,\n \t\tremote->prune = git_config_bool(key, value);\n \telse if (!strcmp(subkey, \"prunetags\"))\n \t\tremote->prune_tags = git_config_bool(key, value);\n+\telse if (!strcmp(subkey, \"prefetchref\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(key);\n+\t\tstring_list_split(&remote->prefetch_refs, value, ' ', -1);\n+\t\treturn 0;\n+\t}\n \telse if (!strcmp(subkey, \"url\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(key);\ndiff --git a/remote.h b/remote.h\nindex b901b56746d..c18e68e0d8d 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -5,6 +5,7 @@\n #include \"hashmap.h\"\n #include \"refspec.h\"\n #include \"strvec.h\"\n+#include \"string-list.h\"\n \n struct option;\n struct transport_ls_refs_options;\n@@ -77,6 +78,8 @@ struct remote {\n \n \tstruct refspec fetch;\n \n+\tstruct string_list prefetch_refs;\n+\n \t/*\n \t * The setting for whether to fetch tags (as a separate rule from the\n \t * configured refspecs);\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex abae7a97546..054f1f06f95 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -245,6 +245,84 @@ test_expect_success 'prefetch multiple remotes' '\n \ttest_subcommand git fetch remote2 $fetchargs /dev/null &&\n+\t\ttest_subcommand git fetch remote2 $fetchargs /dev/null &&\n+\t\ttest_subcommand git fetch remote3 $fetchargs /dev/null &&\n+\t\ttest_subcommand git fetch remote4 $fetchargs writes:\n\n> From: Shubham Kanodia \n>\n> This commit introduces a new configuration option, ...\n\nThe usual way to compose a log message of this project is to\n\n - Give an observation on how the current system work in the present\n tense (so no need to say \"Currently X is Y\", just \"X is Y\"), and\n discuss what you perceive as a problem in it.\n\n - Propose a solution (optional---often, problem description\n trivially leads to an obvious solution in reader's minds).\n\n - Give commands to the codebase to \"become like so\".\n\nin this order.\n\nTo those who have been intimately following the discussion, it often\nis understandable without some of the above, but we are not writing\nfor those who review the patches. We are primarily writing for future\nreaders of \"git log\" who are not aware of the review discussion we\nhave on list, so we should give something to prepare them by setting\nthe stage and stating the objective first, before going into how the\npatch solved it.\n\n> diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\n> index 8efc53e836d..b04ee0c4c22 100644\n> --- a/Documentation/config/remote.txt\n> +++ b/Documentation/config/remote.txt\n> @@ -33,6 +33,11 @@ remote..fetch::\n> \tThe default set of \"refspec\" for linkgit:git-fetch[1]. See\n> \tlinkgit:git-fetch[1].\n> \n> +remote..prefetchref::\n> + Specify the refs to be prefetched when fetching from this remote.\n> + The value is a space-separated list of ref patterns (e.g., \"refs/heads/master !refs/heads/develop*\").\n> + This can be used to optimize fetch operations by specifying exactly which refs should be prefetched.\n\nOverly long lines.\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index b2b5aee5bf2..16c8a31c2e1 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -38,6 +38,7 @@\n> #include \"trace.h\"\n> #include \"trace2.h\"\n> #include \"bundle-uri.h\"\n> +#include \"wildmatch.h\"\n\nThe match done by the wildmatch function is a way too loose to be\nused in the context of parsing and matching the src half of an\nrefspec_item (e.g. a globbing refspec element can have at most one\nasterisk in it, but wildmatch does not have any sanity check to\nensure it). The refspec logic to accept a fetch refspec\n\"refs/heads/*:refs/remotes/origin/*\" and rewrite a ref the remote\nside advertised as refs/heads/foo into refs/remotes/origin/foo, the\nremote-tracking ref on the local side, is in\nremote.c:match_name_with_pattern(), and it does not use wildmatch.\n\n> +static int matches_prefetch_refs(const char *refname, const struct string_list *prefetch_refs)\n> +{\n> +\tint i;\n> +\tint has_positive = 0;\n> +\tint matched_positive = 0;\n> +\tint matched_negative = 0;\n> +\n> +\tfor (i = 0; i < prefetch_refs->nr; i++) {\n\nJust FYI, in modern Git codebase, loop control variable 'i' can be\ndeclared in the initialization part of the for() statement, i.e.\n\n for (int i = 0; i < ...) {\n\nwhich might make it easier to see that there are three variables of\nimportance to determine the outcome of the computation (and 'i' is\nmerely a loop counter).\n\n> +\t\tconst char *pattern = prefetch_refs->items[i].string;\n> +\t\tint is_negative = (*pattern == '!');\n> +\n> +\t\tif (is_negative)\n> +\t\t\tpattern++;\n> +\t\telse\n> +\t\t\thas_positive = 1;\n> +\n> +\t\tif (wildmatch(pattern, refname, 0) == 0) {\n> +\t\t\tif (is_negative)\n> +\t\t\t\tmatched_negative = 1;\n> +\t\t\telse\n> +\t\t\t\tmatched_positive = 1;\n> +\t\t}\n> +\t}\n\nI suspect that the function can be exposed to external callers as-is,\npossibly after renaming it to make it clear that the helper function\nis about refspec. Then you can call it from the code below in stead\nof wildmatch(), by passing the pattern as \"key\", and the refname as\n\"name\", with NULL for both \"value\" and \"result\", I think.\n\n> +\n> +\tif (!has_positive)\n> +\t\treturn !matched_negative;\n> +\n> +\treturn matched_positive && !matched_negative;\n> +}\n\nAnd the implementation of the logic looks correct. If the pattern\nsays \"!A !B !C\" (I don't want any of these), we only care that none\nof these negative patterns trigger, but if the pattern has even one\npositive one \"X Y !A !B !C\" (it must match X or Y, but it must not\nmatch any of the A B C), we also make sure it matched at least one\npositive pattern.\n\n> +static void ref_remove(struct ref **head, struct ref *to_remove)\n> +{\n> +\tstruct ref **pp, *p;\n> +\n> +\tfor (pp = head; (p = *pp) != NULL; pp = &p->next) {\n> +\t\tif (p == to_remove) {\n> +\t\t\t*pp = p->next;\n> +\t\t\treturn;\n> +\t\t}\n> +\t}\n> +}\n\nWe remote from a linked list of \"struct ref\" one such element\nto_remove. OK.\n\n> @@ -610,6 +655,22 @@ static struct ref *get_ref_map(struct remote *remote,\n> \telse\n> \t\tref_map = apply_negative_refspecs(ref_map, &remote->fetch);\n> \n> +\t/**\n> +\t * Filter out advertised refs that we don't want to fetch during\n> +\t * prefetch if a prefetchref config is set\n> +\t */\n> +\tif (prefetch && remote->prefetch_refs.nr) {\n> +\t\tstruct ref *ref, *next;\n> +\t\tfor (ref = ref_map; ref; ref = next) {\n> +\t\t\tnext = ref->next;\n> +\n> +\t\t\tif (!matches_prefetch_refs(ref->name, &remote->prefetch_refs)) {\n> +\t\t\t\t\tref_remove(&ref_map, ref);\n> +\t\t\t\t\tfree_one_ref(ref);\n\nIt is curious why such an overly deep indentation is used only in\nthis if() statement.\n\nIt is good that you made sure we do not leak the ref that we\nremoved, but it looks verly inefficient to repeatedly call\nref_remove (the function becomes more inefficient as the loop\nprogresses as it is more costly to remove later elements on a\nlinearly linked list).\n\nWouldn't it be more efficient to iterate over the original list, sift\neach element into \"surviving\" and \"discarded\" bin as we go? Something\nalong the lines of ...\n\n\tstruct ref *new_list = NULL, **tail = &new_list;\n\n\twhile (ref_map) {\n\t\tstruct ref *ref = ref_map;\n\n\t\tref_map = ref_map->next;\n\t\tref->next = NULL;\n\n \tif (matches(...)) {\n\t\t\t*tail = ref;\n\t\t\ttail = &ref->next;\n\t\t} else {\n\t\t\tfree_one_ref(ref);\n\t\t}\n\t}\n ref_map = new_list;\n\n..., which I imitated code structure of ref_remove_duplicates().\n\n> \tref_map = ref_remove_duplicates(ref_map);\n\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index abae7a97546..054f1f06f95 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n\nAvoid overly long lines in the test script, too.\n\nIn any case, I very much agree with you that get_ref_map().is the\nbest place to add this logic to. Looking much better.\n\nThanks.\n"},{"id":"504113","messageId":"pull.1782.v4.git.1728073292874.gitgitgadget@gmail.com","threadId":"62083","inReplyTo":"pull.1782.v3.git.1726741439445.gitgitgadget@gmail.com","subject":"[PATCH v4] remote: allow specifying refs to prefetch","fromName":"Shubham Kanodia via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-04T20:21:32Z","receivedAt":"2024-10-04T20:21:37Z","isPatch":true,"sender":{"key":"shubham.kanodia10@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8946207?v=4"},"body":"From: Shubham Kanodia \n\nWhen using 'git fetch --prefetch', all fetchable refs are prefetched\nby default. In large repositories with many refs, this can lead to\nunnecessary network traffic and increased disk space use.\n\nIntroduce a new configuration option 'remote..prefetchref' that\nallows users to specify specific ref patterns to be prefetched during\na 'git fetch --prefetch' operation.\n\nThe 'prefetchref' option accepts a space-separated list of ref\npatterns (e.g., 'refs/heads/main !refs/heads/feature/*'). When the\n'--prefetch' option is used with 'git fetch', only the refs matching\nthese patterns will be prefetched.\n\nSigned-off-by: Shubham Kanodia \n---\n remote: introduce config to set prefetch refs\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1782%2Fpastelsky%2Fsk%2Fremote-prefetchref-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1782/pastelsky/sk/remote-prefetchref-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1782\n\nRange-diff vs v3:\n\n 1: 717d5957c47 ! 1: e3d8aee1ea8 remote: introduce config to set prefetch refs\n @@ Metadata\n Author: Shubham Kanodia \n \n ## Commit message ##\n - remote: introduce config to set prefetch refs\n + remote: allow specifying refs to prefetch\n \n - This commit introduces a new configuration option,\n - remote..prefetchref, which allows users to specify specific\n - ref patterns to be prefetched during a git fetch --prefetch\n - operation.\n + When using 'git fetch --prefetch', all fetchable refs are prefetched\n + by default. In large repositories with many refs, this can lead to\n + unnecessary network traffic and increased disk space use.\n \n - The new option accepts a space-separated list of ref patterns.\n - When the --prefetch option is used with git fetch, only the refs\n - matching these patterns will be prefetched, instead of the\n - default behavior of prefetching all fetchable refs.\n + Introduce a new configuration option 'remote..prefetchref' that\n + allows users to specify specific ref patterns to be prefetched during\n + a 'git fetch --prefetch' operation.\n \n - Example usage in .git/config:\n - [remote \"origin\"]\n - prefetchref = \"refs/heads/main refs/heads/feature/*\"\n -\n - This change allows users to optimize their prefetch operations, potentially\n - reducing network traffic and improving performance for large repositories\n - with many refs.\n + The 'prefetchref' option accepts a space-separated list of ref\n + patterns (e.g., 'refs/heads/main !refs/heads/feature/*'). When the\n + '--prefetch' option is used with 'git fetch', only the refs matching\n + these patterns will be prefetched.\n \n Signed-off-by: Shubham Kanodia \n \n @@ Documentation/config/remote.txt: remote..fetch::\n \tlinkgit:git-fetch[1].\n \n +remote..prefetchref::\n -+ Specify the refs to be prefetched when fetching from this remote.\n -+ The value is a space-separated list of ref patterns (e.g., \"refs/heads/master !refs/heads/develop*\").\n -+ This can be used to optimize fetch operations by specifying exactly which refs should be prefetched.\n ++\tSpecify the refs to be prefetched when fetching from this\n ++\tremote. The value is a space-separated list of ref patterns\n ++\t(e.g., \"refs/heads/main !refs/heads/develop*\"). This can be\n ++\tused to optimize fetch operations by specifying exactly which\n ++\trefs should be prefetched.\n +\n remote..push::\n \tThe default set of \"refspec\" for linkgit:git-push[1]. See\n \tlinkgit:git-push[1].\n \n ## builtin/fetch.c ##\n -@@\n - #include \"trace.h\"\n - #include \"trace2.h\"\n - #include \"bundle-uri.h\"\n -+#include \"wildmatch.h\"\n - \n - #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n - \n @@ builtin/fetch.c: static void filter_prefetch_refspec(struct refspec *rs)\n \t}\n }\n \n ++static int pattern_matches_ref(const char *pattern, const char *refname)\n ++{\n ++\tif (strchr(pattern, '*'))\n ++\t\treturn match_refspec_name_with_pattern(pattern, refname, NULL, NULL) != 0;\n ++\treturn strcmp(pattern, refname) == 0;\n ++}\n ++\n +static int matches_prefetch_refs(const char *refname, const struct string_list *prefetch_refs)\n +{\n -+\tint i;\n -+\tint has_positive = 0;\n -+\tint matched_positive = 0;\n -+\tint matched_negative = 0;\n ++\tint has_positive = 0, matched_positive = 0, matched_negative = 0;\n +\n -+\tfor (i = 0; i < prefetch_refs->nr; i++) {\n ++\tfor (int i = 0; i < prefetch_refs->nr; i++) {\n +\t\tconst char *pattern = prefetch_refs->items[i].string;\n +\t\tint is_negative = (*pattern == '!');\n ++\t\tif (is_negative) pattern++;\n ++\t\telse has_positive = 1;\n +\n -+\t\tif (is_negative)\n -+\t\t\tpattern++;\n -+\t\telse\n -+\t\t\thas_positive = 1;\n -+\n -+\t\tif (wildmatch(pattern, refname, 0) == 0) {\n -+\t\t\tif (is_negative)\n -+\t\t\t\tmatched_negative = 1;\n -+\t\t\telse\n -+\t\t\t\tmatched_positive = 1;\n ++\t\tif (pattern_matches_ref(pattern, refname)) {\n ++\t\t\tif (is_negative) matched_negative = 1;\n ++\t\t\telse matched_positive = 1;\n +\t\t}\n +\t}\n +\n -+\tif (!has_positive)\n -+\t\treturn !matched_negative;\n -+\n -+\treturn matched_positive && !matched_negative;\n -+}\n -+\n -+\n -+static void ref_remove(struct ref **head, struct ref *to_remove)\n -+{\n -+\tstruct ref **pp, *p;\n -+\n -+\tfor (pp = head; (p = *pp) != NULL; pp = &p->next) {\n -+\t\tif (p == to_remove) {\n -+\t\t\t*pp = p->next;\n -+\t\t\treturn;\n -+\t\t}\n -+\t}\n ++\treturn has_positive ? (matched_positive && !matched_negative) : !matched_negative;\n +}\n +\n static struct ref *get_ref_map(struct remote *remote,\n \t\t\t const struct ref *remote_refs,\n \t\t\t struct refspec *rs,\n @@ builtin/fetch.c: static struct ref *get_ref_map(struct remote *remote,\n + \tstruct hashmap existing_refs;\n \tint existing_refs_populated = 0;\n \n ++\tstruct ref *prefetch_filtered_ref_map = NULL, **ref_map_tail = &prefetch_filtered_ref_map;\n ++\tstruct ref *next;\n ++\n \tfilter_prefetch_refspec(rs);\n +\n \tif (remote)\n @@ builtin/fetch.c: static struct ref *get_ref_map(struct remote *remote,\n +\t * Filter out advertised refs that we don't want to fetch during\n +\t * prefetch if a prefetchref config is set\n +\t */\n ++\n +\tif (prefetch && remote->prefetch_refs.nr) {\n -+\t\tstruct ref *ref, *next;\n -+\t\tfor (ref = ref_map; ref; ref = next) {\n -+\t\t\tnext = ref->next;\n ++\t\tprefetch_filtered_ref_map = NULL;\n ++\t\tref_map_tail = &prefetch_filtered_ref_map;\n ++\n ++\t\tfor (rm = ref_map; rm; rm = next) {\n ++\t\t\tnext = rm->next;\n ++\t\t\trm->next = NULL;\n +\n -+\t\t\tif (!matches_prefetch_refs(ref->name, &remote->prefetch_refs)) {\n -+\t\t\t\t\tref_remove(&ref_map, ref);\n -+\t\t\t\t\tfree_one_ref(ref);\n ++\t\t\tif (matches_prefetch_refs(rm->name, &remote->prefetch_refs)) {\n ++\t\t\t\t*ref_map_tail = rm;\n ++\t\t\t\tref_map_tail = &rm->next;\n ++\t\t\t} else {\n ++\t\t\t\tfree_one_ref(rm);\n +\t\t\t}\n +\t\t}\n ++\t\tref_map = prefetch_filtered_ref_map;\n +\t}\n +\n \tref_map = ref_remove_duplicates(ref_map);\n @@ remote.c: static int handle_config(const char *key, const char *value,\n \telse if (!strcmp(subkey, \"url\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(key);\n +@@ remote.c: struct strvec *push_url_of_remote(struct remote *remote)\n + \treturn remote->pushurl.nr ? &remote->pushurl : &remote->url;\n + }\n + \n +-static int match_name_with_pattern(const char *key, const char *name,\n ++int match_refspec_name_with_pattern(const char *key, const char *name,\n + \t\t\t\t const char *value, char **result)\n + {\n + \tconst char *kstar = strchr(key, '*');\n +@@ remote.c: static int refspec_match(const struct refspec_item *refspec,\n + \t\t\t const char *name)\n + {\n + \tif (refspec->pattern)\n +-\t\treturn match_name_with_pattern(refspec->src, name, NULL, NULL);\n ++\t\treturn match_refspec_name_with_pattern(refspec->src, name, NULL, NULL);\n + \n + \treturn !strcmp(refspec->src, name);\n + }\n +@@ remote.c: static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n + \t\t\tconst char *key = refspec->dst ? refspec->dst : refspec->src;\n + \t\t\tconst char *value = refspec->src;\n + \n +-\t\t\tif (match_name_with_pattern(key, needle, value, &expn_name))\n ++\t\t\tif (match_refspec_name_with_pattern(key, needle, value, &expn_name))\n + \t\t\t\tstring_list_append_nodup(&reversed, expn_name);\n + \t\t} else if (refspec->matching) {\n + \t\t\t/* For the special matching refspec, any query should match */\n +@@ remote.c: static void query_refspecs_multiple(struct refspec *rs,\n + \t\tif (!refspec->dst || refspec->negative)\n + \t\t\tcontinue;\n + \t\tif (refspec->pattern) {\n +-\t\t\tif (match_name_with_pattern(key, needle, value, result))\n ++\t\t\tif (match_refspec_name_with_pattern(key, needle, value, result))\n + \t\t\t\tstring_list_append_nodup(results, *result);\n + \t\t} else if (!strcmp(needle, key)) {\n + \t\t\tstring_list_append(results, value);\n +@@ remote.c: int query_refspecs(struct refspec *rs, struct refspec_item *query)\n + \t\tif (!refspec->dst || refspec->negative)\n + \t\t\tcontinue;\n + \t\tif (refspec->pattern) {\n +-\t\t\tif (match_name_with_pattern(key, needle, value, result)) {\n ++\t\t\tif (match_refspec_name_with_pattern(key, needle, value, result)) {\n + \t\t\t\tquery->force = refspec->force;\n + \t\t\t\treturn 0;\n + \t\t\t}\n +@@ remote.c: static char *get_ref_match(const struct refspec *rs, const struct ref *ref,\n + \t\t\tconst char *dst_side = item->dst ? item->dst : item->src;\n + \t\t\tint match;\n + \t\t\tif (direction == FROM_SRC)\n +-\t\t\t\tmatch = match_name_with_pattern(item->src, ref->name, dst_side, &name);\n ++\t\t\t\tmatch = match_refspec_name_with_pattern(item->src, ref->name, dst_side, &name);\n + \t\t\telse\n +-\t\t\t\tmatch = match_name_with_pattern(dst_side, ref->name, item->src, &name);\n ++\t\t\t\tmatch = match_refspec_name_with_pattern(dst_side, ref->name, item->src, &name);\n + \t\t\tif (match) {\n + \t\t\t\tmatching_refs = i;\n + \t\t\t\tbreak;\n +@@ remote.c: static struct ref *get_expanded_map(const struct ref *remote_refs,\n + \n + \t\tif (strchr(ref->name, '^'))\n + \t\t\tcontinue; /* a dereference item */\n +-\t\tif (match_name_with_pattern(refspec->src, ref->name,\n ++\t\tif (match_refspec_name_with_pattern(refspec->src, ref->name,\n + \t\t\t\t\t refspec->dst, &expn_name) &&\n + \t\t !ignore_symref_update(expn_name, &scratch)) {\n + \t\t\tstruct ref *cpy = copy_ref(ref);\n \n ## remote.h ##\n @@\n @@ remote.h: struct remote {\n \t/*\n \t * The setting for whether to fetch tags (as a separate rule from the\n \t * configured refspecs);\n +@@ remote.h: int count_refspec_match(const char *, struct ref *refs, struct ref **matched_ref\n + \n + int check_ref_type(const struct ref *ref, int flags);\n + \n ++int match_refspec_name_with_pattern(const char *key, const char *name,\n ++\t\t\t\t\tconst char *value, char **result);\n ++\n + /*\n + * Free a single ref and its peer, or an entire list of refs and their peers,\n + * respectively.\n \n ## t/t7900-maintenance.sh ##\n @@ t/t7900-maintenance.sh: test_expect_success 'prefetch multiple remotes' '\n @@ t/t7900-maintenance.sh: test_expect_success 'prefetch multiple remotes' '\n +\t\tgit checkout -b feature && test_commit feature-commit-2 &&\n +\t\tgit checkout -b wip/test && test_commit wip-test-commit-2 &&\n +\t\tgit checkout -b topic/x && test_commit topic-x-commit-2 &&\n -+\t\tgit push -f origin feature wip/test topic/x&&\n ++\t\tgit push -f origin feature wip/test topic/x &&\n +\t\tcd .. &&\n +\n +\t\tgit config remote.remote2.prefetchref \"refs/heads/feature\" &&\n -+\t\tfetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head --recurse-submodules=no --quiet\" &&\n -+\t\tGIT_TRACE2_EVENT=\"$(pwd)/prefetch-positive.txt\" git maintenance run --task=prefetch 2>/dev/null &&\n ++\t\tfetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head \\\n ++\t\t\t--recurse-submodules=no --quiet\" &&\n ++\t\tGIT_TRACE2_EVENT=\"$(pwd)/prefetch-positive.txt\" \\\n ++\t\t\tgit maintenance run --task=prefetch 2>/dev/null &&\n +\t\ttest_subcommand git fetch remote2 $fetchargs /dev/null &&\n ++\t\tfetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head \\\n ++\t\t\t--recurse-submodules=no --quiet\" &&\n ++\t\tGIT_TRACE2_EVENT=\"$(pwd)/prefetch-negative.txt\" \\\n ++\t\t\tgit maintenance run --task=prefetch 2>/dev/null &&\n +\t\ttest_subcommand git fetch remote3 $fetchargs /dev/null &&\n ++\t\tgit config remote.remote4.prefetchref \\\n ++\t\t\t\"refs/heads/topic/* !refs/heads/topic/y\" &&\n ++\t\tfetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head \\\n ++\t\t\t--recurse-submodules=no --quiet\" &&\n ++\t\tGIT_TRACE2_EVENT=\"$(pwd)/prefetch-mixed.txt\" \\\n ++\t\t\tgit maintenance run --task=prefetch 2>/dev/null &&\n +\t\ttest_subcommand git fetch remote4 $fetchargs .fetch::\n \tThe default set of \"refspec\" for linkgit:git-fetch[1]. See\n \tlinkgit:git-fetch[1].\n \n+remote..prefetchref::\n+\tSpecify the refs to be prefetched when fetching from this\n+\tremote. The value is a space-separated list of ref patterns\n+\t(e.g., \"refs/heads/main !refs/heads/develop*\"). This can be\n+\tused to optimize fetch operations by specifying exactly which\n+\trefs should be prefetched.\n+\n remote..push::\n \tThe default set of \"refspec\" for linkgit:git-push[1]. See\n \tlinkgit:git-push[1].\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b2b5aee5bf2..74603cfabe0 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -485,6 +485,32 @@ static void filter_prefetch_refspec(struct refspec *rs)\n \t}\n }\n \n+static int pattern_matches_ref(const char *pattern, const char *refname)\n+{\n+\tif (strchr(pattern, '*'))\n+\t\treturn match_refspec_name_with_pattern(pattern, refname, NULL, NULL) != 0;\n+\treturn strcmp(pattern, refname) == 0;\n+}\n+\n+static int matches_prefetch_refs(const char *refname, const struct string_list *prefetch_refs)\n+{\n+\tint has_positive = 0, matched_positive = 0, matched_negative = 0;\n+\n+\tfor (int i = 0; i < prefetch_refs->nr; i++) {\n+\t\tconst char *pattern = prefetch_refs->items[i].string;\n+\t\tint is_negative = (*pattern == '!');\n+\t\tif (is_negative) pattern++;\n+\t\telse has_positive = 1;\n+\n+\t\tif (pattern_matches_ref(pattern, refname)) {\n+\t\t\tif (is_negative) matched_negative = 1;\n+\t\t\telse matched_positive = 1;\n+\t\t}\n+\t}\n+\n+\treturn has_positive ? (matched_positive && !matched_negative) : !matched_negative;\n+}\n+\n static struct ref *get_ref_map(struct remote *remote,\n \t\t\t const struct ref *remote_refs,\n \t\t\t struct refspec *rs,\n@@ -501,7 +527,11 @@ static struct ref *get_ref_map(struct remote *remote,\n \tstruct hashmap existing_refs;\n \tint existing_refs_populated = 0;\n \n+\tstruct ref *prefetch_filtered_ref_map = NULL, **ref_map_tail = &prefetch_filtered_ref_map;\n+\tstruct ref *next;\n+\n \tfilter_prefetch_refspec(rs);\n+\n \tif (remote)\n \t\tfilter_prefetch_refspec(&remote->fetch);\n \n@@ -610,6 +640,29 @@ static struct ref *get_ref_map(struct remote *remote,\n \telse\n \t\tref_map = apply_negative_refspecs(ref_map, &remote->fetch);\n \n+\t/**\n+\t * Filter out advertised refs that we don't want to fetch during\n+\t * prefetch if a prefetchref config is set\n+\t */\n+\n+\tif (prefetch && remote->prefetch_refs.nr) {\n+\t\tprefetch_filtered_ref_map = NULL;\n+\t\tref_map_tail = &prefetch_filtered_ref_map;\n+\n+\t\tfor (rm = ref_map; rm; rm = next) {\n+\t\t\tnext = rm->next;\n+\t\t\trm->next = NULL;\n+\n+\t\t\tif (matches_prefetch_refs(rm->name, &remote->prefetch_refs)) {\n+\t\t\t\t*ref_map_tail = rm;\n+\t\t\t\tref_map_tail = &rm->next;\n+\t\t\t} else {\n+\t\t\t\tfree_one_ref(rm);\n+\t\t\t}\n+\t\t}\n+\t\tref_map = prefetch_filtered_ref_map;\n+\t}\n+\n \tref_map = ref_remove_duplicates(ref_map);\n \n \tfor (rm = ref_map; rm; rm = rm->next) {\ndiff --git a/remote.c b/remote.c\nindex 8f3dee13186..6752c73370f 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -141,6 +141,7 @@ static struct remote *make_remote(struct remote_state *remote_state,\n \tret->prune = -1; /* unspecified */\n \tret->prune_tags = -1; /* unspecified */\n \tret->name = xstrndup(name, len);\n+\tstring_list_init_dup(&ret->prefetch_refs);\n \trefspec_init(&ret->push, REFSPEC_PUSH);\n \trefspec_init(&ret->fetch, REFSPEC_FETCH);\n \n@@ -166,6 +167,7 @@ static void remote_clear(struct remote *remote)\n \tfree((char *)remote->uploadpack);\n \tFREE_AND_NULL(remote->http_proxy);\n \tFREE_AND_NULL(remote->http_proxy_authmethod);\n+\tstring_list_clear(&remote->prefetch_refs, 0);\n }\n \n static void add_merge(struct branch *branch, const char *name)\n@@ -456,6 +458,12 @@ static int handle_config(const char *key, const char *value,\n \t\tremote->prune = git_config_bool(key, value);\n \telse if (!strcmp(subkey, \"prunetags\"))\n \t\tremote->prune_tags = git_config_bool(key, value);\n+\telse if (!strcmp(subkey, \"prefetchref\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(key);\n+\t\tstring_list_split(&remote->prefetch_refs, value, ' ', -1);\n+\t\treturn 0;\n+\t}\n \telse if (!strcmp(subkey, \"url\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(key);\n@@ -868,7 +876,7 @@ struct strvec *push_url_of_remote(struct remote *remote)\n \treturn remote->pushurl.nr ? &remote->pushurl : &remote->url;\n }\n \n-static int match_name_with_pattern(const char *key, const char *name,\n+int match_refspec_name_with_pattern(const char *key, const char *name,\n \t\t\t\t const char *value, char **result)\n {\n \tconst char *kstar = strchr(key, '*');\n@@ -900,7 +908,7 @@ static int refspec_match(const struct refspec_item *refspec,\n \t\t\t const char *name)\n {\n \tif (refspec->pattern)\n-\t\treturn match_name_with_pattern(refspec->src, name, NULL, NULL);\n+\t\treturn match_refspec_name_with_pattern(refspec->src, name, NULL, NULL);\n \n \treturn !strcmp(refspec->src, name);\n }\n@@ -969,7 +977,7 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n \t\t\tconst char *key = refspec->dst ? refspec->dst : refspec->src;\n \t\t\tconst char *value = refspec->src;\n \n-\t\t\tif (match_name_with_pattern(key, needle, value, &expn_name))\n+\t\t\tif (match_refspec_name_with_pattern(key, needle, value, &expn_name))\n \t\t\t\tstring_list_append_nodup(&reversed, expn_name);\n \t\t} else if (refspec->matching) {\n \t\t\t/* For the special matching refspec, any query should match */\n@@ -1014,7 +1022,7 @@ static void query_refspecs_multiple(struct refspec *rs,\n \t\tif (!refspec->dst || refspec->negative)\n \t\t\tcontinue;\n \t\tif (refspec->pattern) {\n-\t\t\tif (match_name_with_pattern(key, needle, value, result))\n+\t\t\tif (match_refspec_name_with_pattern(key, needle, value, result))\n \t\t\t\tstring_list_append_nodup(results, *result);\n \t\t} else if (!strcmp(needle, key)) {\n \t\t\tstring_list_append(results, value);\n@@ -1043,7 +1051,7 @@ int query_refspecs(struct refspec *rs, struct refspec_item *query)\n \t\tif (!refspec->dst || refspec->negative)\n \t\t\tcontinue;\n \t\tif (refspec->pattern) {\n-\t\t\tif (match_name_with_pattern(key, needle, value, result)) {\n+\t\t\tif (match_refspec_name_with_pattern(key, needle, value, result)) {\n \t\t\t\tquery->force = refspec->force;\n \t\t\t\treturn 0;\n \t\t\t}\n@@ -1456,9 +1464,9 @@ static char *get_ref_match(const struct refspec *rs, const struct ref *ref,\n \t\t\tconst char *dst_side = item->dst ? item->dst : item->src;\n \t\t\tint match;\n \t\t\tif (direction == FROM_SRC)\n-\t\t\t\tmatch = match_name_with_pattern(item->src, ref->name, dst_side, &name);\n+\t\t\t\tmatch = match_refspec_name_with_pattern(item->src, ref->name, dst_side, &name);\n \t\t\telse\n-\t\t\t\tmatch = match_name_with_pattern(dst_side, ref->name, item->src, &name);\n+\t\t\t\tmatch = match_refspec_name_with_pattern(dst_side, ref->name, item->src, &name);\n \t\t\tif (match) {\n \t\t\t\tmatching_refs = i;\n \t\t\t\tbreak;\n@@ -2076,7 +2084,7 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,\n \n \t\tif (strchr(ref->name, '^'))\n \t\t\tcontinue; /* a dereference item */\n-\t\tif (match_name_with_pattern(refspec->src, ref->name,\n+\t\tif (match_refspec_name_with_pattern(refspec->src, ref->name,\n \t\t\t\t\t refspec->dst, &expn_name) &&\n \t\t !ignore_symref_update(expn_name, &scratch)) {\n \t\t\tstruct ref *cpy = copy_ref(ref);\ndiff --git a/remote.h b/remote.h\nindex b901b56746d..9ffef206f23 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -5,6 +5,7 @@\n #include \"hashmap.h\"\n #include \"refspec.h\"\n #include \"strvec.h\"\n+#include \"string-list.h\"\n \n struct option;\n struct transport_ls_refs_options;\n@@ -77,6 +78,8 @@ struct remote {\n \n \tstruct refspec fetch;\n \n+\tstruct string_list prefetch_refs;\n+\n \t/*\n \t * The setting for whether to fetch tags (as a separate rule from the\n \t * configured refspecs);\n@@ -207,6 +210,9 @@ int count_refspec_match(const char *, struct ref *refs, struct ref **matched_ref\n \n int check_ref_type(const struct ref *ref, int flags);\n \n+int match_refspec_name_with_pattern(const char *key, const char *name,\n+\t\t\t\t\tconst char *value, char **result);\n+\n /*\n * Free a single ref and its peer, or an entire list of refs and their peers,\n * respectively.\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex abae7a97546..fc1b5d14e75 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -245,6 +245,91 @@ test_expect_success 'prefetch multiple remotes' '\n \ttest_subcommand git fetch remote2 $fetchargs /dev/null &&\n+\t\ttest_subcommand git fetch remote2 $fetchargs /dev/null &&\n+\t\ttest_subcommand git fetch remote3 $fetchargs /dev/null &&\n+\t\ttest_subcommand git fetch remote4 $fetchargs wrote:\n>\n> \"Shubham Kanodia via GitGitGadget\" writes:\n>\n> > From: Shubham Kanodia \n> >\n> > This commit introduces a new configuration option, ...\n>\n> The usual way to compose a log message of this project is to\n>\n> - Give an observation on how the current system work in the present\n> tense (so no need to say \"Currently X is Y\", just \"X is Y\"), and\n> discuss what you perceive as a problem in it.\n>\n> - Propose a solution (optional---often, problem description\n> trivially leads to an obvious solution in reader's minds).\n>\n> - Give commands to the codebase to \"become like so\".\n>\n> in this order.\n>\n> To those who have been intimately following the discussion, it often\n> is understandable without some of the above, but we are not writing\n> for those who review the patches. We are primarily writing for future\n> readers of \"git log\" who are not aware of the review discussion we\n> have on list, so we should give something to prepare them by setting\n> the stage and stating the objective first, before going into how the\n> patch solved it.\n>\n> > diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\n> > index 8efc53e836d..b04ee0c4c22 100644\n> > --- a/Documentation/config/remote.txt\n> > +++ b/Documentation/config/remote.txt\n> > @@ -33,6 +33,11 @@ remote..fetch::\n> > The default set of \"refspec\" for linkgit:git-fetch[1]. See\n> > linkgit:git-fetch[1].\n> >\n> > +remote..prefetchref::\n> > + Specify the refs to be prefetched when fetching from this remote.\n> > + The value is a space-separated list of ref patterns (e.g., \"refs/heads/master !refs/heads/develop*\").\n> > + This can be used to optimize fetch operations by specifying exactly which refs should be prefetched.\n>\n> Overly long lines.\n>\n> > diff --git a/builtin/fetch.c b/builtin/fetch.c\n> > index b2b5aee5bf2..16c8a31c2e1 100644\n> > --- a/builtin/fetch.c\n> > +++ b/builtin/fetch.c\n> > @@ -38,6 +38,7 @@\n> > #include \"trace.h\"\n> > #include \"trace2.h\"\n> > #include \"bundle-uri.h\"\n> > +#include \"wildmatch.h\"\n>\n> The match done by the wildmatch function is a way too loose to be\n> used in the context of parsing and matching the src half of an\n> refspec_item (e.g. a globbing refspec element can have at most one\n> asterisk in it, but wildmatch does not have any sanity check to\n> ensure it). The refspec logic to accept a fetch refspec\n> \"refs/heads/*:refs/remotes/origin/*\" and rewrite a ref the remote\n> side advertised as refs/heads/foo into refs/remotes/origin/foo, the\n> remote-tracking ref on the local side, is in\n> remote.c:match_name_with_pattern(), and it does not use wildmatch.\n>\n> > +static int matches_prefetch_refs(const char *refname, const struct string_list *prefetch_refs)\n> > +{\n> > + int i;\n> > + int has_positive = 0;\n> > + int matched_positive = 0;\n> > + int matched_negative = 0;\n> > +\n> > + for (i = 0; i < prefetch_refs->nr; i++) {\n>\n> Just FYI, in modern Git codebase, loop control variable 'i' can be\n> declared in the initialization part of the for() statement, i.e.\n>\n> for (int i = 0; i < ...) {\n>\n> which might make it easier to see that there are three variables of\n> importance to determine the outcome of the computation (and 'i' is\n> merely a loop counter).\n>\n> > + const char *pattern = prefetch_refs->items[i].string;\n> > + int is_negative = (*pattern == '!');\n> > +\n> > + if (is_negative)\n> > + pattern++;\n> > + else\n> > + has_positive = 1;\n> > +\n> > + if (wildmatch(pattern, refname, 0) == 0) {\n> > + if (is_negative)\n> > + matched_negative = 1;\n> > + else\n> > + matched_positive = 1;\n> > + }\n> > + }\n>\n> I suspect that the function can be exposed to external callers as-is,\n> possibly after renaming it to make it clear that the helper function\n> is about refspec. Then you can call it from the code below in stead\n> of wildmatch(), by passing the pattern as \"key\", and the refname as\n> \"name\", with NULL for both \"value\" and \"result\", I think.\n>\n> > +\n> > + if (!has_positive)\n> > + return !matched_negative;\n> > +\n> > + return matched_positive && !matched_negative;\n> > +}\n>\n> And the implementation of the logic looks correct. If the pattern\n> says \"!A !B !C\" (I don't want any of these), we only care that none\n> of these negative patterns trigger, but if the pattern has even one\n> positive one \"X Y !A !B !C\" (it must match X or Y, but it must not\n> match any of the A B C), we also make sure it matched at least one\n> positive pattern.\n>\n> > +static void ref_remove(struct ref **head, struct ref *to_remove)\n> > +{\n> > + struct ref **pp, *p;\n> > +\n> > + for (pp = head; (p = *pp) != NULL; pp = &p->next) {\n> > + if (p == to_remove) {\n> > + *pp = p->next;\n> > + return;\n> > + }\n> > + }\n> > +}\n>\n> We remote from a linked list of \"struct ref\" one such element\n> to_remove. OK.\n>\n> > @@ -610,6 +655,22 @@ static struct ref *get_ref_map(struct remote *remote,\n> > else\n> > ref_map = apply_negative_refspecs(ref_map, &remote->fetch);\n> >\n> > + /**\n> > + * Filter out advertised refs that we don't want to fetch during\n> > + * prefetch if a prefetchref config is set\n> > + */\n> > + if (prefetch && remote->prefetch_refs.nr) {\n> > + struct ref *ref, *next;\n> > + for (ref = ref_map; ref; ref = next) {\n> > + next = ref->next;\n> > +\n> > + if (!matches_prefetch_refs(ref->name, &remote->prefetch_refs)) {\n> > + ref_remove(&ref_map, ref);\n> > + free_one_ref(ref);\n>\n> It is curious why such an overly deep indentation is used only in\n> this if() statement.\n>\n> It is good that you made sure we do not leak the ref that we\n> removed, but it looks verly inefficient to repeatedly call\n> ref_remove (the function becomes more inefficient as the loop\n> progresses as it is more costly to remove later elements on a\n> linearly linked list).\n>\n> Wouldn't it be more efficient to iterate over the original list, sift\n> each element into \"surviving\" and \"discarded\" bin as we go? Something\n> along the lines of ...\n>\n> struct ref *new_list = NULL, **tail = &new_list;\n>\n> while (ref_map) {\n> struct ref *ref = ref_map;\n>\n> ref_map = ref_map->next;\n> ref->next = NULL;\n>\n> if (matches(...)) {\n> *tail = ref;\n> tail = &ref->next;\n> } else {\n> free_one_ref(ref);\n> }\n> }\n> ref_map = new_list;\n>\n> ..., which I imitated code structure of ref_remove_duplicates().\n>\n> > ref_map = ref_remove_duplicates(ref_map);\n>\n> > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> > index abae7a97546..054f1f06f95 100755\n> > --- a/t/t7900-maintenance.sh\n> > +++ b/t/t7900-maintenance.sh\n>\n> Avoid overly long lines in the test script, too.\n>\n> In any case, I very much agree with you that get_ref_map().is the\n> best place to add this logic to. Looking much better.\n>\n> Thanks.\n"},{"id":"506503","messageId":"CAG=Um+1Ox9wDJixK+9dugVO4m37TSqjqYEc7eLQZK0rUuWzwrA@mail.gmail.com","threadId":"62083","inReplyTo":"pull.1782.v4.git.1728073292874.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] remote: allow specifying refs to prefetch","fromName":"Shubham Kanodia","fromEmail":"shubham.kanodia10@gmail.com","sentAt":"2024-11-04T08:47:30Z","receivedAt":"2024-11-04T08:48:08Z","isPatch":true,"sender":{"key":"shubham.kanodia10@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8946207?v=4"},"body":"On Sat, Oct 5, 2024 at 1:51 AM Shubham Kanodia via GitGitGadget\n wrote:\n>\n> From: Shubham Kanodia \n>\n> When using 'git fetch --prefetch', all fetchable refs are prefetched\n> by default. In large repositories with many refs, this can lead to\n> unnecessary network traffic and increased disk space use.\n>\n> Introduce a new configuration option 'remote..prefetchref' that\n> allows users to specify specific ref patterns to be prefetched during\n> a 'git fetch --prefetch' operation.\n>\n> The 'prefetchref' option accepts a space-separated list of ref\n> patterns (e.g., 'refs/heads/main !refs/heads/feature/*'). When the\n> '--prefetch' option is used with 'git fetch', only the refs matching\n> these patterns will be prefetched.\n>\n> Signed-off-by: Shubham Kanodia \n> ---\n> remote: introduce config to set prefetch refs\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1782%2Fpastelsky%2Fsk%2Fremote-prefetchref-v4\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1782/pastelsky/sk/remote-prefetchref-v4\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1782\n>\n> Range-diff vs v3:\n>\n> 1: 717d5957c47 ! 1: e3d8aee1ea8 remote: introduce config to set prefetch refs\n> @@ Metadata\n> Author: Shubham Kanodia \n>\n> ## Commit message ##\n> - remote: introduce config to set prefetch refs\n> + remote: allow specifying refs to prefetch\n>\n> - This commit introduces a new configuration option,\n> - remote..prefetchref, which allows users to specify specific\n> - ref patterns to be prefetched during a git fetch --prefetch\n> - operation.\n> + When using 'git fetch --prefetch', all fetchable refs are prefetched\n> + by default. In large repositories with many refs, this can lead to\n> + unnecessary network traffic and increased disk space use.\n>\n> - The new option accepts a space-separated list of ref patterns.\n> - When the --prefetch option is used with git fetch, only the refs\n> - matching these patterns will be prefetched, instead of the\n> - default behavior of prefetching all fetchable refs.\n> + Introduce a new configuration option 'remote..prefetchref' that\n> + allows users to specify specific ref patterns to be prefetched during\n> + a 'git fetch --prefetch' operation.\n>\n> - Example usage in .git/config:\n> - [remote \"origin\"]\n> - prefetchref = \"refs/heads/main refs/heads/feature/*\"\n> -\n> - This change allows users to optimize their prefetch operations, potentially\n> - reducing network traffic and improving performance for large repositories\n> - with many refs.\n> + The 'prefetchref' option accepts a space-separated list of ref\n> + patterns (e.g., 'refs/heads/main !refs/heads/feature/*'). When the\n> + '--prefetch' option is used with 'git fetch', only the refs matching\n> + these patterns will be prefetched.\n>\n> Signed-off-by: Shubham Kanodia \n>\n> @@ Documentation/config/remote.txt: remote..fetch::\n> linkgit:git-fetch[1].\n>\n> +remote..prefetchref::\n> -+ Specify the refs to be prefetched when fetching from this remote.\n> -+ The value is a space-separated list of ref patterns (e.g., \"refs/heads/master !refs/heads/develop*\").\n> -+ This can be used to optimize fetch operations by specifying exactly which refs should be prefetched.\n> ++ Specify the refs to be prefetched when fetching from this\n> ++ remote. The value is a space-separated list of ref patterns\n> ++ (e.g., \"refs/heads/main !refs/heads/develop*\"). This can be\n> ++ used to optimize fetch operations by specifying exactly which\n> ++ refs should be prefetched.\n> +\n> remote..push::\n> The default set of \"refspec\" for linkgit:git-push[1]. See\n> linkgit:git-push[1].\n>\n> ## builtin/fetch.c ##\n> -@@\n> - #include \"trace.h\"\n> - #include \"trace2.h\"\n> - #include \"bundle-uri.h\"\n> -+#include \"wildmatch.h\"\n> -\n> - #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n> -\n> @@ builtin/fetch.c: static void filter_prefetch_refspec(struct refspec *rs)\n> }\n> }\n>\n> ++static int pattern_matches_ref(const char *pattern, const char *refname)\n> ++{\n> ++ if (strchr(pattern, '*'))\n> ++ return match_refspec_name_with_pattern(pattern, refname, NULL, NULL) != 0;\n> ++ return strcmp(pattern, refname) == 0;\n> ++}\n> ++\n> +static int matches_prefetch_refs(const char *refname, const struct string_list *prefetch_refs)\n> +{\n> -+ int i;\n> -+ int has_positive = 0;\n> -+ int matched_positive = 0;\n> -+ int matched_negative = 0;\n> ++ int has_positive = 0, matched_positive = 0, matched_negative = 0;\n> +\n> -+ for (i = 0; i < prefetch_refs->nr; i++) {\n> ++ for (int i = 0; i < prefetch_refs->nr; i++) {\n> + const char *pattern = prefetch_refs->items[i].string;\n> + int is_negative = (*pattern == '!');\n> ++ if (is_negative) pattern++;\n> ++ else has_positive = 1;\n> +\n> -+ if (is_negative)\n> -+ pattern++;\n> -+ else\n> -+ has_positive = 1;\n> -+\n> -+ if (wildmatch(pattern, refname, 0) == 0) {\n> -+ if (is_negative)\n> -+ matched_negative = 1;\n> -+ else\n> -+ matched_positive = 1;\n> ++ if (pattern_matches_ref(pattern, refname)) {\n> ++ if (is_negative) matched_negative = 1;\n> ++ else matched_positive = 1;\n> + }\n> + }\n> +\n> -+ if (!has_positive)\n> -+ return !matched_negative;\n> -+\n> -+ return matched_positive && !matched_negative;\n> -+}\n> -+\n> -+\n> -+static void ref_remove(struct ref **head, struct ref *to_remove)\n> -+{\n> -+ struct ref **pp, *p;\n> -+\n> -+ for (pp = head; (p = *pp) != NULL; pp = &p->next) {\n> -+ if (p == to_remove) {\n> -+ *pp = p->next;\n> -+ return;\n> -+ }\n> -+ }\n> ++ return has_positive ? (matched_positive && !matched_negative) : !matched_negative;\n> +}\n> +\n> static struct ref *get_ref_map(struct remote *remote,\n> const struct ref *remote_refs,\n> struct refspec *rs,\n> @@ builtin/fetch.c: static struct ref *get_ref_map(struct remote *remote,\n> + struct hashmap existing_refs;\n> int existing_refs_populated = 0;\n>\n> ++ struct ref *prefetch_filtered_ref_map = NULL, **ref_map_tail = &prefetch_filtered_ref_map;\n> ++ struct ref *next;\n> ++\n> filter_prefetch_refspec(rs);\n> +\n> if (remote)\n> @@ builtin/fetch.c: static struct ref *get_ref_map(struct remote *remote,\n> + * Filter out advertised refs that we don't want to fetch during\n> + * prefetch if a prefetchref config is set\n> + */\n> ++\n> + if (prefetch && remote->prefetch_refs.nr) {\n> -+ struct ref *ref, *next;\n> -+ for (ref = ref_map; ref; ref = next) {\n> -+ next = ref->next;\n> ++ prefetch_filtered_ref_map = NULL;\n> ++ ref_map_tail = &prefetch_filtered_ref_map;\n> ++\n> ++ for (rm = ref_map; rm; rm = next) {\n> ++ next = rm->next;\n> ++ rm->next = NULL;\n> +\n> -+ if (!matches_prefetch_refs(ref->name, &remote->prefetch_refs)) {\n> -+ ref_remove(&ref_map, ref);\n> -+ free_one_ref(ref);\n> ++ if (matches_prefetch_refs(rm->name, &remote->prefetch_refs)) {\n> ++ *ref_map_tail = rm;\n> ++ ref_map_tail = &rm->next;\n> ++ } else {\n> ++ free_one_ref(rm);\n> + }\n> + }\n> ++ ref_map = prefetch_filtered_ref_map;\n> + }\n> +\n> ref_map = ref_remove_duplicates(ref_map);\n> @@ remote.c: static int handle_config(const char *key, const char *value,\n> else if (!strcmp(subkey, \"url\")) {\n> if (!value)\n> return config_error_nonbool(key);\n> +@@ remote.c: struct strvec *push_url_of_remote(struct remote *remote)\n> + return remote->pushurl.nr ? &remote->pushurl : &remote->url;\n> + }\n> +\n> +-static int match_name_with_pattern(const char *key, const char *name,\n> ++int match_refspec_name_with_pattern(const char *key, const char *name,\n> + const char *value, char **result)\n> + {\n> + const char *kstar = strchr(key, '*');\n> +@@ remote.c: static int refspec_match(const struct refspec_item *refspec,\n> + const char *name)\n> + {\n> + if (refspec->pattern)\n> +- return match_name_with_pattern(refspec->src, name, NULL, NULL);\n> ++ return match_refspec_name_with_pattern(refspec->src, name, NULL, NULL);\n> +\n> + return !strcmp(refspec->src, name);\n> + }\n> +@@ remote.c: static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n> + const char *key = refspec->dst ? refspec->dst : refspec->src;\n> + const char *value = refspec->src;\n> +\n> +- if (match_name_with_pattern(key, needle, value, &expn_name))\n> ++ if (match_refspec_name_with_pattern(key, needle, value, &expn_name))\n> + string_list_append_nodup(&reversed, expn_name);\n> + } else if (refspec->matching) {\n> + /* For the special matching refspec, any query should match */\n> +@@ remote.c: static void query_refspecs_multiple(struct refspec *rs,\n> + if (!refspec->dst || refspec->negative)\n> + continue;\n> + if (refspec->pattern) {\n> +- if (match_name_with_pattern(key, needle, value, result))\n> ++ if (match_refspec_name_with_pattern(key, needle, value, result))\n> + string_list_append_nodup(results, *result);\n> + } else if (!strcmp(needle, key)) {\n> + string_list_append(results, value);\n> +@@ remote.c: int query_refspecs(struct refspec *rs, struct refspec_item *query)\n> + if (!refspec->dst || refspec->negative)\n> + continue;\n> + if (refspec->pattern) {\n> +- if (match_name_with_pattern(key, needle, value, result)) {\n> ++ if (match_refspec_name_with_pattern(key, needle, value, result)) {\n> + query->force = refspec->force;\n> + return 0;\n> + }\n> +@@ remote.c: static char *get_ref_match(const struct refspec *rs, const struct ref *ref,\n> + const char *dst_side = item->dst ? item->dst : item->src;\n> + int match;\n> + if (direction == FROM_SRC)\n> +- match = match_name_with_pattern(item->src, ref->name, dst_side, &name);\n> ++ match = match_refspec_name_with_pattern(item->src, ref->name, dst_side, &name);\n> + else\n> +- match = match_name_with_pattern(dst_side, ref->name, item->src, &name);\n> ++ match = match_refspec_name_with_pattern(dst_side, ref->name, item->src, &name);\n> + if (match) {\n> + matching_refs = i;\n> + break;\n> +@@ remote.c: static struct ref *get_expanded_map(const struct ref *remote_refs,\n> +\n> + if (strchr(ref->name, '^'))\n> + continue; /* a dereference item */\n> +- if (match_name_with_pattern(refspec->src, ref->name,\n> ++ if (match_refspec_name_with_pattern(refspec->src, ref->name,\n> + refspec->dst, &expn_name) &&\n> + !ignore_symref_update(expn_name, &scratch)) {\n> + struct ref *cpy = copy_ref(ref);\n>\n> ## remote.h ##\n> @@\n> @@ remote.h: struct remote {\n> /*\n> * The setting for whether to fetch tags (as a separate rule from the\n> * configured refspecs);\n> +@@ remote.h: int count_refspec_match(const char *, struct ref *refs, struct ref **matched_ref\n> +\n> + int check_ref_type(const struct ref *ref, int flags);\n> +\n> ++int match_refspec_name_with_pattern(const char *key, const char *name,\n> ++ const char *value, char **result);\n> ++\n> + /*\n> + * Free a single ref and its peer, or an entire list of refs and their peers,\n> + * respectively.\n>\n> ## t/t7900-maintenance.sh ##\n> @@ t/t7900-maintenance.sh: test_expect_success 'prefetch multiple remotes' '\n> @@ t/t7900-maintenance.sh: test_expect_success 'prefetch multiple remotes' '\n> + git checkout -b feature && test_commit feature-commit-2 &&\n> + git checkout -b wip/test && test_commit wip-test-commit-2 &&\n> + git checkout -b topic/x && test_commit topic-x-commit-2 &&\n> -+ git push -f origin feature wip/test topic/x&&\n> ++ git push -f origin feature wip/test topic/x &&\n> + cd .. &&\n> +\n> + git config remote.remote2.prefetchref \"refs/heads/feature\" &&\n> -+ fetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head --recurse-submodules=no --quiet\" &&\n> -+ GIT_TRACE2_EVENT=\"$(pwd)/prefetch-positive.txt\" git maintenance run --task=prefetch 2>/dev/null &&\n> ++ fetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head \\\n> ++ --recurse-submodules=no --quiet\" &&\n> ++ GIT_TRACE2_EVENT=\"$(pwd)/prefetch-positive.txt\" \\\n> ++ git maintenance run --task=prefetch 2>/dev/null &&\n> + test_subcommand git fetch remote2 $fetchargs +\n> + git rev-parse refs/prefetch/remotes/remote2/feature &&\n> @@ t/t7900-maintenance.sh: test_expect_success 'prefetch multiple remotes' '\n> + cd .. &&\n> +\n> + git config remote.remote3.prefetchref \"!refs/heads/wip/*\" &&\n> -+ fetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head --recurse-submodules=no --quiet\" &&\n> -+ GIT_TRACE2_EVENT=\"$(pwd)/prefetch-negative.txt\" git maintenance run --task=prefetch 2>/dev/null &&\n> ++ fetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head \\\n> ++ --recurse-submodules=no --quiet\" &&\n> ++ GIT_TRACE2_EVENT=\"$(pwd)/prefetch-negative.txt\" \\\n> ++ git maintenance run --task=prefetch 2>/dev/null &&\n> + test_subcommand git fetch remote3 $fetchargs + git rev-parse refs/prefetch/remotes/remote3/feature &&\n> + git rev-parse refs/prefetch/remotes/remote3/topic/x &&\n> @@ t/t7900-maintenance.sh: test_expect_success 'prefetch multiple remotes' '\n> + git push -f origin feature topic/x topic/y &&\n> + cd .. &&\n> +\n> -+ git config remote.remote4.prefetchref \"refs/heads/topic/* !refs/heads/topic/y\" &&\n> -+ fetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head --recurse-submodules=no --quiet\" &&\n> -+ GIT_TRACE2_EVENT=\"$(pwd)/prefetch-mixed.txt\" git maintenance run --task=prefetch 2>/dev/null &&\n> ++ git config remote.remote4.prefetchref \\\n> ++ \"refs/heads/topic/* !refs/heads/topic/y\" &&\n> ++ fetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head \\\n> ++ --recurse-submodules=no --quiet\" &&\n> ++ GIT_TRACE2_EVENT=\"$(pwd)/prefetch-mixed.txt\" \\\n> ++ git maintenance run --task=prefetch 2>/dev/null &&\n> + test_subcommand git fetch remote4 $fetchargs +\n> + test_must_fail git rev-parse refs/prefetch/remotes/remote4/feature &&\n>\n>\n> Documentation/config/remote.txt | 7 +++\n> builtin/fetch.c | 53 ++++++++++++++++++++\n> remote.c | 24 ++++++----\n> remote.h | 6 +++\n> t/t7900-maintenance.sh | 85 +++++++++++++++++++++++++++++++++\n> 5 files changed, 167 insertions(+), 8 deletions(-)\n>\n> diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\n> index 8efc53e836d..186f439ed7b 100644\n> --- a/Documentation/config/remote.txt\n> +++ b/Documentation/config/remote.txt\n> @@ -33,6 +33,13 @@ remote..fetch::\n> The default set of \"refspec\" for linkgit:git-fetch[1]. See\n> linkgit:git-fetch[1].\n>\n> +remote..prefetchref::\n> + Specify the refs to be prefetched when fetching from this\n> + remote. The value is a space-separated list of ref patterns\n> + (e.g., \"refs/heads/main !refs/heads/develop*\"). This can be\n> + used to optimize fetch operations by specifying exactly which\n> + refs should be prefetched.\n> +\n> remote..push::\n> The default set of \"refspec\" for linkgit:git-push[1]. See\n> linkgit:git-push[1].\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index b2b5aee5bf2..74603cfabe0 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -485,6 +485,32 @@ static void filter_prefetch_refspec(struct refspec *rs)\n> }\n> }\n>\n> +static int pattern_matches_ref(const char *pattern, const char *refname)\n> +{\n> + if (strchr(pattern, '*'))\n> + return match_refspec_name_with_pattern(pattern, refname, NULL, NULL) != 0;\n> + return strcmp(pattern, refname) == 0;\n> +}\n> +\n> +static int matches_prefetch_refs(const char *refname, const struct string_list *prefetch_refs)\n> +{\n> + int has_positive = 0, matched_positive = 0, matched_negative = 0;\n> +\n> + for (int i = 0; i < prefetch_refs->nr; i++) {\n> + const char *pattern = prefetch_refs->items[i].string;\n> + int is_negative = (*pattern == '!');\n> + if (is_negative) pattern++;\n> + else has_positive = 1;\n> +\n> + if (pattern_matches_ref(pattern, refname)) {\n> + if (is_negative) matched_negative = 1;\n> + else matched_positive = 1;\n> + }\n> + }\n> +\n> + return has_positive ? (matched_positive && !matched_negative) : !matched_negative;\n> +}\n> +\n> static struct ref *get_ref_map(struct remote *remote,\n> const struct ref *remote_refs,\n> struct refspec *rs,\n> @@ -501,7 +527,11 @@ static struct ref *get_ref_map(struct remote *remote,\n> struct hashmap existing_refs;\n> int existing_refs_populated = 0;\n>\n> + struct ref *prefetch_filtered_ref_map = NULL, **ref_map_tail = &prefetch_filtered_ref_map;\n> + struct ref *next;\n> +\n> filter_prefetch_refspec(rs);\n> +\n> if (remote)\n> filter_prefetch_refspec(&remote->fetch);\n>\n> @@ -610,6 +640,29 @@ static struct ref *get_ref_map(struct remote *remote,\n> else\n> ref_map = apply_negative_refspecs(ref_map, &remote->fetch);\n>\n> + /**\n> + * Filter out advertised refs that we don't want to fetch during\n> + * prefetch if a prefetchref config is set\n> + */\n> +\n> + if (prefetch && remote->prefetch_refs.nr) {\n> + prefetch_filtered_ref_map = NULL;\n> + ref_map_tail = &prefetch_filtered_ref_map;\n> +\n> + for (rm = ref_map; rm; rm = next) {\n> + next = rm->next;\n> + rm->next = NULL;\n> +\n> + if (matches_prefetch_refs(rm->name, &remote->prefetch_refs)) {\n> + *ref_map_tail = rm;\n> + ref_map_tail = &rm->next;\n> + } else {\n> + free_one_ref(rm);\n> + }\n> + }\n> + ref_map = prefetch_filtered_ref_map;\n> + }\n> +\n> ref_map = ref_remove_duplicates(ref_map);\n>\n> for (rm = ref_map; rm; rm = rm->next) {\n> diff --git a/remote.c b/remote.c\n> index 8f3dee13186..6752c73370f 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -141,6 +141,7 @@ static struct remote *make_remote(struct remote_state *remote_state,\n> ret->prune = -1; /* unspecified */\n> ret->prune_tags = -1; /* unspecified */\n> ret->name = xstrndup(name, len);\n> + string_list_init_dup(&ret->prefetch_refs);\n> refspec_init(&ret->push, REFSPEC_PUSH);\n> refspec_init(&ret->fetch, REFSPEC_FETCH);\n>\n> @@ -166,6 +167,7 @@ static void remote_clear(struct remote *remote)\n> free((char *)remote->uploadpack);\n> FREE_AND_NULL(remote->http_proxy);\n> FREE_AND_NULL(remote->http_proxy_authmethod);\n> + string_list_clear(&remote->prefetch_refs, 0);\n> }\n>\n> static void add_merge(struct branch *branch, const char *name)\n> @@ -456,6 +458,12 @@ static int handle_config(const char *key, const char *value,\n> remote->prune = git_config_bool(key, value);\n> else if (!strcmp(subkey, \"prunetags\"))\n> remote->prune_tags = git_config_bool(key, value);\n> + else if (!strcmp(subkey, \"prefetchref\")) {\n> + if (!value)\n> + return config_error_nonbool(key);\n> + string_list_split(&remote->prefetch_refs, value, ' ', -1);\n> + return 0;\n> + }\n> else if (!strcmp(subkey, \"url\")) {\n> if (!value)\n> return config_error_nonbool(key);\n> @@ -868,7 +876,7 @@ struct strvec *push_url_of_remote(struct remote *remote)\n> return remote->pushurl.nr ? &remote->pushurl : &remote->url;\n> }\n>\n> -static int match_name_with_pattern(const char *key, const char *name,\n> +int match_refspec_name_with_pattern(const char *key, const char *name,\n> const char *value, char **result)\n> {\n> const char *kstar = strchr(key, '*');\n> @@ -900,7 +908,7 @@ static int refspec_match(const struct refspec_item *refspec,\n> const char *name)\n> {\n> if (refspec->pattern)\n> - return match_name_with_pattern(refspec->src, name, NULL, NULL);\n> + return match_refspec_name_with_pattern(refspec->src, name, NULL, NULL);\n>\n> return !strcmp(refspec->src, name);\n> }\n> @@ -969,7 +977,7 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n> const char *key = refspec->dst ? refspec->dst : refspec->src;\n> const char *value = refspec->src;\n>\n> - if (match_name_with_pattern(key, needle, value, &expn_name))\n> + if (match_refspec_name_with_pattern(key, needle, value, &expn_name))\n> string_list_append_nodup(&reversed, expn_name);\n> } else if (refspec->matching) {\n> /* For the special matching refspec, any query should match */\n> @@ -1014,7 +1022,7 @@ static void query_refspecs_multiple(struct refspec *rs,\n> if (!refspec->dst || refspec->negative)\n> continue;\n> if (refspec->pattern) {\n> - if (match_name_with_pattern(key, needle, value, result))\n> + if (match_refspec_name_with_pattern(key, needle, value, result))\n> string_list_append_nodup(results, *result);\n> } else if (!strcmp(needle, key)) {\n> string_list_append(results, value);\n> @@ -1043,7 +1051,7 @@ int query_refspecs(struct refspec *rs, struct refspec_item *query)\n> if (!refspec->dst || refspec->negative)\n> continue;\n> if (refspec->pattern) {\n> - if (match_name_with_pattern(key, needle, value, result)) {\n> + if (match_refspec_name_with_pattern(key, needle, value, result)) {\n> query->force = refspec->force;\n> return 0;\n> }\n> @@ -1456,9 +1464,9 @@ static char *get_ref_match(const struct refspec *rs, const struct ref *ref,\n> const char *dst_side = item->dst ? item->dst : item->src;\n> int match;\n> if (direction == FROM_SRC)\n> - match = match_name_with_pattern(item->src, ref->name, dst_side, &name);\n> + match = match_refspec_name_with_pattern(item->src, ref->name, dst_side, &name);\n> else\n> - match = match_name_with_pattern(dst_side, ref->name, item->src, &name);\n> + match = match_refspec_name_with_pattern(dst_side, ref->name, item->src, &name);\n> if (match) {\n> matching_refs = i;\n> break;\n> @@ -2076,7 +2084,7 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,\n>\n> if (strchr(ref->name, '^'))\n> continue; /* a dereference item */\n> - if (match_name_with_pattern(refspec->src, ref->name,\n> + if (match_refspec_name_with_pattern(refspec->src, ref->name,\n> refspec->dst, &expn_name) &&\n> !ignore_symref_update(expn_name, &scratch)) {\n> struct ref *cpy = copy_ref(ref);\n> diff --git a/remote.h b/remote.h\n> index b901b56746d..9ffef206f23 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -5,6 +5,7 @@\n> #include \"hashmap.h\"\n> #include \"refspec.h\"\n> #include \"strvec.h\"\n> +#include \"string-list.h\"\n>\n> struct option;\n> struct transport_ls_refs_options;\n> @@ -77,6 +78,8 @@ struct remote {\n>\n> struct refspec fetch;\n>\n> + struct string_list prefetch_refs;\n> +\n> /*\n> * The setting for whether to fetch tags (as a separate rule from the\n> * configured refspecs);\n> @@ -207,6 +210,9 @@ int count_refspec_match(const char *, struct ref *refs, struct ref **matched_ref\n>\n> int check_ref_type(const struct ref *ref, int flags);\n>\n> +int match_refspec_name_with_pattern(const char *key, const char *name,\n> + const char *value, char **result);\n> +\n> /*\n> * Free a single ref and its peer, or an entire list of refs and their peers,\n> * respectively.\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index abae7a97546..fc1b5d14e75 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -245,6 +245,91 @@ test_expect_success 'prefetch multiple remotes' '\n> test_subcommand git fetch remote2 $fetchargs '\n>\n> +test_expect_success 'prefetch with positive prefetch ref patterns' '\n> + test_create_repo filter-prefetch-positive &&\n> + (\n> + cd filter-prefetch-positive &&\n> + test_commit initial &&\n> + git clone . clone2 &&\n> + git remote add remote2 \"file://$(pwd)/clone2\" &&\n> +\n> + cd clone2 &&\n> + git checkout -b feature && test_commit feature-commit-2 &&\n> + git checkout -b wip/test && test_commit wip-test-commit-2 &&\n> + git checkout -b topic/x && test_commit topic-x-commit-2 &&\n> + git push -f origin feature wip/test topic/x &&\n> + cd .. &&\n> +\n> + git config remote.remote2.prefetchref \"refs/heads/feature\" &&\n> + fetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head \\\n> + --recurse-submodules=no --quiet\" &&\n> + GIT_TRACE2_EVENT=\"$(pwd)/prefetch-positive.txt\" \\\n> + git maintenance run --task=prefetch 2>/dev/null &&\n> + test_subcommand git fetch remote2 $fetchargs +\n> + git rev-parse refs/prefetch/remotes/remote2/feature &&\n> + test_must_fail git rev-parse refs/prefetch/remotes/remote2/wip/test &&\n> + test_must_fail git rev-parse refs/prefetch/remotes/remote2/topic/x\n> + )\n> +'\n> +\n> +test_expect_success 'prefetch with negative prefetch ref patterns' '\n> + test_create_repo filter-prefetch-negative &&\n> + (\n> + cd filter-prefetch-negative &&\n> + test_commit initial &&\n> + git clone . clone3 &&\n> + git remote add remote3 \"file://$(pwd)/clone3\" &&\n> + cat .git/config &&\n> +\n> + cd clone3 &&\n> + git checkout -b feature && test_commit feature-commit-3 &&\n> + git checkout -b wip/test && test_commit wip-test-commit-3 &&\n> + git checkout -b topic/x && test_commit topic-x-commit-3 &&\n> + git push -f origin feature wip/test topic/x &&\n> + cd .. &&\n> +\n> + git config remote.remote3.prefetchref \"!refs/heads/wip/*\" &&\n> + fetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head \\\n> + --recurse-submodules=no --quiet\" &&\n> + GIT_TRACE2_EVENT=\"$(pwd)/prefetch-negative.txt\" \\\n> + git maintenance run --task=prefetch 2>/dev/null &&\n> + test_subcommand git fetch remote3 $fetchargs + git rev-parse refs/prefetch/remotes/remote3/feature &&\n> + git rev-parse refs/prefetch/remotes/remote3/topic/x &&\n> + test_must_fail git rev-parse refs/prefetch/remotes/remote3/wip/test\n> + )\n> +'\n> +\n> +test_expect_success 'prefetch with positive & negative prefetch ref patterns' '\n> + test_create_repo filter-prefetch-mixed &&\n> + (\n> + cd filter-prefetch-mixed &&\n> + test_commit initial &&\n> + git clone . clone4 &&\n> + git remote add remote4 \"file://$(pwd)/clone4\" &&\n> +\n> + cd clone4 &&\n> + git checkout -b feature && test_commit feature-commit-4 &&\n> + git checkout -b topic/x && test_commit topic-x-commit-4 &&\n> + git checkout -b topic/y && test_commit topic-y-commit-4 &&\n> + git push -f origin feature topic/x topic/y &&\n> + cd .. &&\n> +\n> + git config remote.remote4.prefetchref \\\n> + \"refs/heads/topic/* !refs/heads/topic/y\" &&\n> + fetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head \\\n> + --recurse-submodules=no --quiet\" &&\n> + GIT_TRACE2_EVENT=\"$(pwd)/prefetch-mixed.txt\" \\\n> + git maintenance run --task=prefetch 2>/dev/null &&\n> + test_subcommand git fetch remote4 $fetchargs +\n> + test_must_fail git rev-parse refs/prefetch/remotes/remote4/feature &&\n> + test_must_fail git rev-parse refs/prefetch/remotes/remote4/topic/y &&\n> + git rev-parse refs/prefetch/remotes/remote4/topic/x\n> + )\n> +'\n> +\n> test_expect_success 'loose-objects task' '\n> # Repack everything so we know the state of the object dir\n> git repack -adk &&\n>\n> base-commit: 2e7b89e038c0c888acf61f1b4ee5a43d4dd5e94c\n> --\n> gitgitgadget\n\nA friendly bump to revive this thread to see if the code looks good to\nyou Junio.\n"},{"id":"506632","messageId":"Zym--GVNJt_lsQEz@pks.im","threadId":"62083","inReplyTo":"pull.1782.v4.git.1728073292874.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] remote: allow specifying refs to prefetch","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-05T06:45:17Z","receivedAt":"2024-11-05T06:45:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 04, 2024 at 08:21:32PM +0000, Shubham Kanodia via GitGitGadget wrote:\n\nI'm coming rather late to the party and simply want to review this so\nthat the thread gets revived. So my context may be lacking, please\nforgive me if I am reopening things that were already discussed.\n\n> diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\n> index 8efc53e836d..186f439ed7b 100644\n> --- a/Documentation/config/remote.txt\n> +++ b/Documentation/config/remote.txt\n> @@ -33,6 +33,13 @@ remote..fetch::\n> \tThe default set of \"refspec\" for linkgit:git-fetch[1]. See\n> \tlinkgit:git-fetch[1].\n> \n> +remote..prefetchref::\n> +\tSpecify the refs to be prefetched when fetching from this\n> +\tremote. The value is a space-separated list of ref patterns\n> +\t(e.g., \"refs/heads/main !refs/heads/develop*\"). This can be\n> +\tused to optimize fetch operations by specifying exactly which\n> +\trefs should be prefetched.\n\nI'm a bit surprised that we use a space-separated list here instead of\naccepting a multi-valued config like we do for \"remote..fetch\".\nShouldn't we use the format here to make things more consistent?\n\n> remote..push::\n> \tThe default set of \"refspec\" for linkgit:git-push[1]. See\n> \tlinkgit:git-push[1].\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index b2b5aee5bf2..74603cfabe0 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -485,6 +485,32 @@ static void filter_prefetch_refspec(struct refspec *rs)\n> \t}\n> }\n> \n> +static int pattern_matches_ref(const char *pattern, const char *refname)\n> +{\n> +\tif (strchr(pattern, '*'))\n> +\t\treturn match_refspec_name_with_pattern(pattern, refname, NULL, NULL) != 0;\n> +\treturn strcmp(pattern, refname) == 0;\n> +}\n> +\n> +static int matches_prefetch_refs(const char *refname, const struct string_list *prefetch_refs)\n> +{\n> +\tint has_positive = 0, matched_positive = 0, matched_negative = 0;\n> +\n> +\tfor (int i = 0; i < prefetch_refs->nr; i++) {\n> +\t\tconst char *pattern = prefetch_refs->items[i].string;\n> +\t\tint is_negative = (*pattern == '!');\n> +\t\tif (is_negative) pattern++;\n> +\t\telse has_positive = 1;\n> +\n> +\t\tif (pattern_matches_ref(pattern, refname)) {\n> +\t\t\tif (is_negative) matched_negative = 1;\n> +\t\t\telse matched_positive = 1;\n> +\t\t}\n> +\t}\n> +\n> +\treturn has_positive ? (matched_positive && !matched_negative) : !matched_negative;\n> +}\n\nThis is essentially open-coding a bunch of logic around how we parse\nrefspecs. I'd propose to instead use the APIs we already have in this\narea, namely those in \"refspec.h\".\n\n> static struct ref *get_ref_map(struct remote *remote,\n> \t\t\t const struct ref *remote_refs,\n> \t\t\t struct refspec *rs,\n> @@ -501,7 +527,11 @@ static struct ref *get_ref_map(struct remote *remote,\n> \tstruct hashmap existing_refs;\n> \tint existing_refs_populated = 0;\n> \n> +\tstruct ref *prefetch_filtered_ref_map = NULL, **ref_map_tail = &prefetch_filtered_ref_map;\n> +\tstruct ref *next;\n> +\n\nWe don't typically have empty lines between variable declarations.\n\n> \tfilter_prefetch_refspec(rs);\n> +\n> \tif (remote)\n> \t\tfilter_prefetch_refspec(&remote->fetch);\n> \n> @@ -610,6 +640,29 @@ static struct ref *get_ref_map(struct remote *remote,\n> \telse\n> \t\tref_map = apply_negative_refspecs(ref_map, &remote->fetch);\n> \n> +\t/**\n> +\t * Filter out advertised refs that we don't want to fetch during\n> +\t * prefetch if a prefetchref config is set\n> +\t */\n\nOur comments typically start with `/*`, not `/**`.\n\n> +\tif (prefetch && remote->prefetch_refs.nr) {\n> +\t\tprefetch_filtered_ref_map = NULL;\n> +\t\tref_map_tail = &prefetch_filtered_ref_map;\n\nAs far as I can see, both of these variables have already been\ninitialized beforehand.\n\n> +\t\tfor (rm = ref_map; rm; rm = next) {\n> +\t\t\tnext = rm->next;\n> +\t\t\trm->next = NULL;\n> +\n> +\t\t\tif (matches_prefetch_refs(rm->name, &remote->prefetch_refs)) {\n> +\t\t\t\t*ref_map_tail = rm;\n> +\t\t\t\tref_map_tail = &rm->next;\n> +\t\t\t} else {\n> +\t\t\t\tfree_one_ref(rm);\n> +\t\t\t}\n> +\t\t}\n> +\t\tref_map = prefetch_filtered_ref_map;\n> +\t}\n> +\n> \tref_map = ref_remove_duplicates(ref_map);\n> \n> \tfor (rm = ref_map; rm; rm = rm->next) {\n> diff --git a/remote.c b/remote.c\n> index 8f3dee13186..6752c73370f 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -141,6 +141,7 @@ static struct remote *make_remote(struct remote_state *remote_state,\n> \tret->prune = -1; /* unspecified */\n> \tret->prune_tags = -1; /* unspecified */\n> \tret->name = xstrndup(name, len);\n> +\tstring_list_init_dup(&ret->prefetch_refs);\n> \trefspec_init(&ret->push, REFSPEC_PUSH);\n> \trefspec_init(&ret->fetch, REFSPEC_FETCH);\n> \n> @@ -166,6 +167,7 @@ static void remote_clear(struct remote *remote)\n> \tfree((char *)remote->uploadpack);\n> \tFREE_AND_NULL(remote->http_proxy);\n> \tFREE_AND_NULL(remote->http_proxy_authmethod);\n> +\tstring_list_clear(&remote->prefetch_refs, 0);\n> }\n> \n> static void add_merge(struct branch *branch, const char *name)\n> @@ -456,6 +458,12 @@ static int handle_config(const char *key, const char *value,\n> \t\tremote->prune = git_config_bool(key, value);\n> \telse if (!strcmp(subkey, \"prunetags\"))\n> \t\tremote->prune_tags = git_config_bool(key, value);\n> +\telse if (!strcmp(subkey, \"prefetchref\")) {\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(key);\n> +\t\tstring_list_split(&remote->prefetch_refs, value, ' ', -1);\n> +\t\treturn 0;\n> +\t}\n> \telse if (!strcmp(subkey, \"url\")) {\n> \t\tif (!value)\n> \t\t\treturn config_error_nonbool(key);\n> @@ -868,7 +876,7 @@ struct strvec *push_url_of_remote(struct remote *remote)\n> \treturn remote->pushurl.nr ? &remote->pushurl : &remote->url;\n> }\n> \n> -static int match_name_with_pattern(const char *key, const char *name,\n> +int match_refspec_name_with_pattern(const char *key, const char *name,\n> \t\t\t\t const char *value, char **result)\n> {\n> \tconst char *kstar = strchr(key, '*');\n\nIs this rename really necessary? It is not mentioned in the commit\nmessage, so this is surprising to me. If it really was necessary it\nshould be split out into a separate commit that also explains why you\nthink that this is a good idea.\n\n> diff --git a/remote.h b/remote.h\n> index b901b56746d..9ffef206f23 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -5,6 +5,7 @@\n> #include \"hashmap.h\"\n> #include \"refspec.h\"\n> #include \"strvec.h\"\n> +#include \"string-list.h\"\n> \n> struct option;\n> struct transport_ls_refs_options;\n> @@ -77,6 +78,8 @@ struct remote {\n> \n> \tstruct refspec fetch;\n> \n> +\tstruct string_list prefetch_refs;\n> +\n> \t/*\n> \t * The setting for whether to fetch tags (as a separate rule from the\n> \t * configured refspecs);\n> @@ -207,6 +210,9 @@ int count_refspec_match(const char *, struct ref *refs, struct ref **matched_ref\n> \n> int check_ref_type(const struct ref *ref, int flags);\n> \n> +int match_refspec_name_with_pattern(const char *key, const char *name,\n> +\t\t\t\t\tconst char *value, char **result);\n\nI think instead of exposing this function we should rather expose\n`refspec_match()`, which is at a higher level and knows to handle the\ncases for us where the refspec is a pattern and when it's not.\n\nPatrick\n"},{"id":"506661","messageId":"8c4e5911-d908-437a-8eec-385e272c8847@gmail.com","threadId":"62083","inReplyTo":"pull.1782.v4.git.1728073292874.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] remote: allow specifying refs to prefetch","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-11-05T14:45:17Z","receivedAt":"2024-11-05T14:45:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Shubham\n\nOn 04/10/2024 21:21, Shubham Kanodia via GitGitGadget wrote:\n> From: Shubham Kanodia \n> \n\nI agree with the Patrick's comments on the implementation, I've left a \ncouple of test comments below.\n\n> +test_expect_success 'prefetch with positive prefetch ref patterns' '\n> +\ttest_create_repo filter-prefetch-positive &&\n> +\t(\n> +\t\tcd filter-prefetch-positive &&\n> +\t\ttest_commit initial &&\n> +\t\tgit clone . clone2 &&\n> +\t\tgit remote add remote2 \"file://$(pwd)/clone2\" &&\n> +\n> +\t\tcd clone2 &&\n> +\t\tgit checkout -b feature && test_commit feature-commit-2 &&\n> +\t\tgit checkout -b wip/test && test_commit wip-test-commit-2 &&\n> +\t\tgit checkout -b topic/x && test_commit topic-x-commit-2 &&\n> +\t\tgit push -f origin feature wip/test topic/x &&\n> +\t\tcd .. &&\n\nI think it would make sense to have a blank line before this rather than \nafter it so \"cd\" is grouped with the commands executed in that directory.\n\n> +\t\tgit config remote.remote2.prefetchref \"refs/heads/feature\" &&\n> +\t\tfetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head \\\n> +\t\t\t--recurse-submodules=no --quiet\" &&\n> +\t\tGIT_TRACE2_EVENT=\"$(pwd)/prefetch-positive.txt\" \\\n> +\t\t\tgit maintenance run --task=prefetch 2>/dev/null &&\n> +\t\ttest_subcommand git fetch remote2 $fetchargs +\t\tgit rev-parse refs/prefetch/remotes/remote2/feature &&\n> +\t\ttest_must_fail git rev-parse refs/prefetch/remotes/remote2/wip/test &&\n> +\t\ttest_must_fail git rev-parse refs/prefetch/remotes/remote2/topic/x\n\nthese are the important tests for checking the prefetch pattern \nmatching. We should perhaps be using \"git rev-parse --verify\"\n\nThe test coverage looks good\n\nBest Wishes\n\nPhillip\n\n> +\t)\n> +'\n> +\n> +test_expect_success 'prefetch with negative prefetch ref patterns' '\n> +\ttest_create_repo filter-prefetch-negative &&\n> +\t(\n> +\t\tcd filter-prefetch-negative &&\n> +\t\ttest_commit initial &&\n> +\t\tgit clone . clone3 &&\n> +\t\tgit remote add remote3 \"file://$(pwd)/clone3\" &&\n> +\t\tcat .git/config &&\n> +\n> +\t\tcd clone3 &&\n> +\t\tgit checkout -b feature && test_commit feature-commit-3 &&\n> +\t\tgit checkout -b wip/test && test_commit wip-test-commit-3 &&\n> +\t\tgit checkout -b topic/x && test_commit topic-x-commit-3 &&\n> +\t\tgit push -f origin feature wip/test topic/x &&\n> +\t\tcd .. &&\n> +\n> +\t\tgit config remote.remote3.prefetchref \"!refs/heads/wip/*\" &&\n> +\t\tfetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head \\\n> +\t\t\t--recurse-submodules=no --quiet\" &&\n> +\t\tGIT_TRACE2_EVENT=\"$(pwd)/prefetch-negative.txt\" \\\n> +\t\t\tgit maintenance run --task=prefetch 2>/dev/null &&\n> +\t\ttest_subcommand git fetch remote3 $fetchargs +\t\tgit rev-parse refs/prefetch/remotes/remote3/feature &&\n> +\t\tgit rev-parse refs/prefetch/remotes/remote3/topic/x &&\n> +\t\ttest_must_fail git rev-parse refs/prefetch/remotes/remote3/wip/test\n> +\t)\n> +'\n> +\n> +test_expect_success 'prefetch with positive & negative prefetch ref patterns' '\n> +\ttest_create_repo filter-prefetch-mixed &&\n> +\t(\n> +\t\tcd filter-prefetch-mixed &&\n> +\t\ttest_commit initial &&\n> +\t\tgit clone . clone4 &&\n> +\t\tgit remote add remote4 \"file://$(pwd)/clone4\" &&\n> +\n> +\t\tcd clone4 &&\n> +\t\tgit checkout -b feature && test_commit feature-commit-4 &&\n> +\t\tgit checkout -b topic/x && test_commit topic-x-commit-4 &&\n> +\t\tgit checkout -b topic/y && test_commit topic-y-commit-4 &&\n> +\t\tgit push -f origin feature topic/x topic/y &&\n> +\t\tcd .. &&\n> +\n> +\t\tgit config remote.remote4.prefetchref \\\n> +\t\t\t\"refs/heads/topic/* !refs/heads/topic/y\" &&\n> +\t\tfetchargs=\"--prefetch --prune --no-tags --no-write-fetch-head \\\n> +\t\t\t--recurse-submodules=no --quiet\" &&\n> +\t\tGIT_TRACE2_EVENT=\"$(pwd)/prefetch-mixed.txt\" \\\n> +\t\t\tgit maintenance run --task=prefetch 2>/dev/null &&\n> +\t\ttest_subcommand git fetch remote4 $fetchargs +\n> +\t\ttest_must_fail git rev-parse refs/prefetch/remotes/remote4/feature &&\n> +\t\ttest_must_fail git rev-parse refs/prefetch/remotes/remote4/topic/y &&\n> +\t\tgit rev-parse refs/prefetch/remotes/remote4/topic/x\n> +\t)\n> +'\n> +\n> test_expect_success 'loose-objects task' '\n> \t# Repack everything so we know the state of the object dir\n> \tgit repack -adk &&\n> \n> base-commit: 2e7b89e038c0c888acf61f1b4ee5a43d4dd5e94c\n\n"},{"id":"506662","messageId":"b548c532-e54a-4edc-94aa-f3c2925602e6@gmail.com","threadId":"62083","inReplyTo":"Zym--GVNJt_lsQEz@pks.im","subject":"Re: [PATCH v4] remote: allow specifying refs to prefetch","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-11-05T14:47:11Z","receivedAt":"2024-11-05T14:47:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 05/11/2024 06:45, Patrick Steinhardt wrote:\n> On Fri, Oct 04, 2024 at 08:21:32PM +0000, Shubham Kanodia via GitGitGadget wrote:\n> \n>> diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\n>> index 8efc53e836d..186f439ed7b 100644\n>> --- a/Documentation/config/remote.txt\n>> +++ b/Documentation/config/remote.txt\n>> @@ -33,6 +33,13 @@ remote..fetch::\n>> \tThe default set of \"refspec\" for linkgit:git-fetch[1]. See\n>> \tlinkgit:git-fetch[1].\n>> \n>> +remote..prefetchref::\n>> +\tSpecify the refs to be prefetched when fetching from this\n>> +\tremote. The value is a space-separated list of ref patterns\n>> +\t(e.g., \"refs/heads/main !refs/heads/develop*\"). This can be\n>> +\tused to optimize fetch operations by specifying exactly which\n>> +\trefs should be prefetched.\n> \n> I'm a bit surprised that we use a space-separated list here instead of\n> accepting a multi-valued config like we do for \"remote..fetch\".\n> Shouldn't we use the format here to make things more consistent?\n\nI agree that would be a good idea. I also think that it would be helpful \nto expand the documentation to explain exactly how the patterns are \nmatched. I think \"remote..prefetch\" would better match the \nexisting \"remote..fetch\" and \"remote..name\" config key names.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"506667","messageId":"CAG=Um+3D3iGPRv4t4DcCP=Bh7gbAQ6cSJ79jmg2h-qbSiQRkUA@mail.gmail.com","threadId":"62083","inReplyTo":"b548c532-e54a-4edc-94aa-f3c2925602e6@gmail.com","subject":"Re: [PATCH v4] remote: allow specifying refs to prefetch","fromName":"Shubham Kanodia","fromEmail":"shubham.kanodia10@gmail.com","sentAt":"2024-11-05T16:26:52Z","receivedAt":"2024-11-05T16:27:30Z","isPatch":true,"sender":{"key":"shubham.kanodia10@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8946207?v=4"},"body":"On Tue, Nov 5, 2024 at 8:17 PM Phillip Wood wrote:\n>\n> On 05/11/2024 06:45, Patrick Steinhardt wrote:\n> > On Fri, Oct 04, 2024 at 08:21:32PM +0000, Shubham Kanodia via GitGitGadget wrote:\n> >\n> >> diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\n> >> index 8efc53e836d..186f439ed7b 100644\n> >> --- a/Documentation/config/remote.txt\n> >> +++ b/Documentation/config/remote.txt\n> >> @@ -33,6 +33,13 @@ remote..fetch::\n> >> The default set of \"refspec\" for linkgit:git-fetch[1]. See\n> >> linkgit:git-fetch[1].\n> >>\n> >> +remote..prefetchref::\n> >> + Specify the refs to be prefetched when fetching from this\n> >> + remote. The value is a space-separated list of ref patterns\n> >> + (e.g., \"refs/heads/main !refs/heads/develop*\"). This can be\n> >> + used to optimize fetch operations by specifying exactly which\n> >> + refs should be prefetched.\n> >\n> > I'm a bit surprised that we use a space-separated list here instead of\n> > accepting a multi-valued config like we do for \"remote..fetch\".\n> > Shouldn't we use the format here to make things more consistent?\n>\n> I agree that would be a good idea. I also think that it would be helpful\n> to expand the documentation to explain exactly how the patterns are\n> matched. I think \"remote..prefetch\" would better match the\n> existing \"remote..fetch\" and \"remote..name\" config key names.\n>\n> Best Wishes\n>\n> Phillip\n>\n\nThanks for taking the time to look at this!\n\nLet me add context here based on a few things that have already been discussed\nas a part of this thread or an earlier discussion on\nhttps://lore.kernel.org/git/CAG=Um+1wTbXn_RN+LOCrpZpSNR_QF582PszxNyhz5anVHtBp+w@mail.gmail.com/\n\n> I'm coming rather late to the party and simply want to review this so\n> that the thread gets revived. So my context may be lacking, please\n> forgive me if I am reopening things that were already discussed.\n\nI don't have a particular preference here, and this was discussed in\nan earlier thread\nwhere Junio opined (https://lore.kernel.org/git/xmqq5xrcn2k1.fsf@gitster.g/—\n\n> I agree that it is the right place to configure this as attributes\n> to remotes. It would make it handy if we could give a catch-all\n> configuration, though. For example:\n>\n> [remote \"origin\"]\n> prefetch = true\n> prefetchref = refs/heads/* refs/tags/*\n> [remote \"*\"]\n> prefetch = false\n>\n> may toggle prefetch off for all remotes, except that the tags and\n> the local branches of the remote \"origin\" are prefetched. Instead\n> of a multi-value configuration variable (like remote.*.fetch) where\n> we need to worry about clearing convention, we can use a regular\n> \"last one wins\" variable that is whitespace separated patterns, as\n> such a pattern can never have a whitespace in it.\n\nwhich is what my implementation is based on.\n\n> This is essentially open-coding a bunch of logic around how we parse\n> refspecs. I'd propose to instead use the APIs we already have in this\n> area, namely those in \"refspec.h\".\n\nIs there any that you would suggest? I don't see anything in `refspec.h` that\nmight cover a similar logic. I'd previously used `wildmatch.h`, but\nthat is really\nonly suitable for glob-style wildcards.\n\n> Is this rename really necessary? It is not mentioned in the commit\n> message, so this is surprising to me\n\nWhat was previously `match_name_with_pattern` is now exposed out\nfrom `remote.c` to be used outside. I thought given that its now applicable\noutside `remote.c` it might be clearer to rename it to be more explicit\n(match name of what? remote? refspec? etc)\n\nHowever I agree that the higher level `refspec_match()` is a better\nfunction to expose\nanyway, and am happy to make that change.\n"},{"id":"506688","messageId":"xmqqy11xqjpx.fsf@gitster.g","threadId":"62083","inReplyTo":"CAG=Um+3D3iGPRv4t4DcCP=Bh7gbAQ6cSJ79jmg2h-qbSiQRkUA@mail.gmail.com","subject":"Re: [PATCH v4] remote: allow specifying refs to prefetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-06T00:39:06Z","receivedAt":"2024-11-06T00:39:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shubham Kanodia writes:\n\n> I don't have a particular preference here, and this was discussed in\n> an earlier thread\n> where Junio opined (https://lore.kernel.org/git/xmqq5xrcn2k1.fsf@gitster.g/—\n>\n>> I agree that it is the right place to configure this as attributes\n>> to remotes. It would make it handy if we could give a catch-all\n>> configuration, though. For example:\n>>\n>> [remote \"origin\"]\n>> prefetch = true\n>> prefetchref = refs/heads/* refs/tags/*\n>> [remote \"*\"]\n>> prefetch = false\n>>\n>> may toggle prefetch off for all remotes, except that the tags and\n>> the local branches of the remote \"origin\" are prefetched. Instead\n>> of a multi-value configuration variable (like remote.*.fetch) where\n>> we need to worry about clearing convention, we can use a regular\n>> \"last one wins\" variable that is whitespace separated patterns, as\n>> such a pattern can never have a whitespace in it.\n> which is what my implementation is based on.\n\nI am fine with space separated list or multi-valued variable. The\nonly difference is that with multi-valued list, we'd need to worry\nabout ensuring that we have a way to \"clear\" the values we have seen\nso far. It has plenty of precedence and is not a rocket science.\nThe above, if I recall correctly, was solely about the need for\n\"catch-all default\" (aka \"*\" remote) and not about multi-value vs\nspace separated last-one-wins value at all. IOW, the above could\nhave been\n\n [remote \"origin\"]\n\tprefetch = true\n\tprefetchref = refs/heads/*\n\tprefetchref = refs/tags/*\n [remote \"*\"]\n\tprefetch = false\n\nand conveyed exactly what I wanted to say in the message you quoted.\n\nIn any case, I somehow thought that we discarded the arrangement\nwith \"*\" wildcard as unworkable. If I remember the discussion\nbefore I left correctly, didn't it turn out to be troublesome to\nhave [remote \"*\"] section because existing code would need to\nenumerate configured remotes, and we do not want to see \"*\" listed?\nIf we found a workable solution to that while I was away, that would\nbe great, but I haven't looked at what this latest round of the\nseries does to solve it (yet). Perhaps teaching \"git remote\" and\n\"git fetch --all\" to skip \"*\" while enumerating remotes was\nsufficient? I dunno.\n\nThanks.\n"},{"id":"506703","messageId":"ZysQvUyxgdRqjvj2@pks.im","threadId":"62083","inReplyTo":"CAG=Um+3D3iGPRv4t4DcCP=Bh7gbAQ6cSJ79jmg2h-qbSiQRkUA@mail.gmail.com","subject":"Re: [PATCH v4] remote: allow specifying refs to prefetch","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-06T06:46:27Z","receivedAt":"2024-11-06T06:46:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Nov 05, 2024 at 09:56:52PM +0530, Shubham Kanodia wrote:\n> On Tue, Nov 5, 2024 at 8:17 PM Phillip Wood wrote:\n> > On 05/11/2024 06:45, Patrick Steinhardt wrote:\n> > > On Fri, Oct 04, 2024 at 08:21:32PM +0000, Shubham Kanodia via GitGitGadget wrote:\n> > >> diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\n> > >> index 8efc53e836d..186f439ed7b 100644\n> > >> --- a/Documentation/config/remote.txt\n> > >> +++ b/Documentation/config/remote.txt\n> > >> @@ -33,6 +33,13 @@ remote..fetch::\n> > >> The default set of \"refspec\" for linkgit:git-fetch[1]. See\n> > >> linkgit:git-fetch[1].\n> > >>\n> > >> +remote..prefetchref::\n> > >> + Specify the refs to be prefetched when fetching from this\n> > >> + remote. The value is a space-separated list of ref patterns\n> > >> + (e.g., \"refs/heads/main !refs/heads/develop*\"). This can be\n> > >> + used to optimize fetch operations by specifying exactly which\n> > >> + refs should be prefetched.\n> > >\n> > > I'm a bit surprised that we use a space-separated list here instead of\n> > > accepting a multi-valued config like we do for \"remote..fetch\".\n> > > Shouldn't we use the format here to make things more consistent?\n> >\n> > I agree that would be a good idea. I also think that it would be helpful\n> > to expand the documentation to explain exactly how the patterns are\n> > matched. I think \"remote..prefetch\" would better match the\n> > existing \"remote..fetch\" and \"remote..name\" config key names.\n> >\n> > Best Wishes\n> >\n> > Phillip\n> >\n> \n> Thanks for taking the time to look at this!\n> \n> Let me add context here based on a few things that have already been discussed\n> as a part of this thread or an earlier discussion on\n> https://lore.kernel.org/git/CAG=Um+1wTbXn_RN+LOCrpZpSNR_QF582PszxNyhz5anVHtBp+w@mail.gmail.com/\n> \n> > I'm coming rather late to the party and simply want to review this so\n> > that the thread gets revived. So my context may be lacking, please\n> > forgive me if I am reopening things that were already discussed.\n> \n> I don't have a particular preference here, and this was discussed in\n> an earlier thread\n> where Junio opined (https://lore.kernel.org/git/xmqq5xrcn2k1.fsf@gitster.g/—\n\nFair enough, thanks for the pointer! The reason stated by Junio is that\nhaving a space-separated list of refspecs makes it easier to reset the\nrefspec again, as we can simply use a \"last-one-wins\" pattern. And while\nI understand that, I would claim that we already have a properly\nestablished way to reset multi-valued lists by first assigning an empty\nvalue:\n\n [remote \"origin\"]\n prefetchref = refs/heads/*\n prefetchref = refs/tags/*\n prefetchref =\n prefetchref = refs/heads/*\n\nThe final value would be only \"refs/heads/*\" due to the reset.\n\nSo overall I'm still leaning into the direction of making this a\nmulti-valued list for the sake of consistency with other refspec\nconfigs. @Junio Let me know in case you disagree though.\n\n> > I agree that it is the right place to configure this as attributes\n> > to remotes. It would make it handy if we could give a catch-all\n> > configuration, though. For example:\n> >\n> > [remote \"origin\"]\n> > prefetch = true\n> > prefetchref = refs/heads/* refs/tags/*\n> > [remote \"*\"]\n> > prefetch = false\n> >\n> > may toggle prefetch off for all remotes, except that the tags and\n> > the local branches of the remote \"origin\" are prefetched. Instead\n> > of a multi-value configuration variable (like remote.*.fetch) where\n> > we need to worry about clearing convention, we can use a regular\n> > \"last one wins\" variable that is whitespace separated patterns, as\n> > such a pattern can never have a whitespace in it.\n> \n> which is what my implementation is based on.\n> \n> > This is essentially open-coding a bunch of logic around how we parse\n> > refspecs. I'd propose to instead use the APIs we already have in this\n> > area, namely those in \"refspec.h\".\n> \n> Is there any that you would suggest? I don't see anything in `refspec.h` that\n> might cover a similar logic. I'd previously used `wildmatch.h`, but\n> that is really\n> only suitable for glob-style wildcards.\n\nThe logic around refspecs is somewhat split up between different headers\nindeed. \"refspec.h\" really only concerns parsing of refspecs, but does\nnot provide the interfaces to also use them to translate or match refs.\n\nI think the closest to what you need is `omit_name_by_refspec()`: you\ngive it a refspec and a refname, and it returns whether any of the\nrefspec items matches that ref. This also takes into oaccount whether\nthe refspec item is negated (\"!\" prefix\") or whether it is a pattern\n(contains \"*\").\n\nIdeally, we'd move interfaces that provide basic refspec functionality\ninto \"refspec.h\" so that they are easier to discover. But I'm fine with\nmaking this a #leftoverbit, I don't want to push the burden on you.\n\nPatrick\n"},{"id":"506704","messageId":"ZysSEJUqSKUq4PKb@pks.im","threadId":"62083","inReplyTo":"xmqqy11xqjpx.fsf@gitster.g","subject":"Re: [PATCH v4] remote: allow specifying refs to prefetch","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-06T06:52:05Z","receivedAt":"2024-11-06T06:52:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Nov 05, 2024 at 04:39:06PM -0800, Junio C Hamano wrote:\n> Shubham Kanodia writes:\n> \n> > I don't have a particular preference here, and this was discussed in\n> > an earlier thread\n> > where Junio opined (https://lore.kernel.org/git/xmqq5xrcn2k1.fsf@gitster.g/—\n> >\n> >> I agree that it is the right place to configure this as attributes\n> >> to remotes. It would make it handy if we could give a catch-all\n> >> configuration, though. For example:\n> >>\n> >> [remote \"origin\"]\n> >> prefetch = true\n> >> prefetchref = refs/heads/* refs/tags/*\n> >> [remote \"*\"]\n> >> prefetch = false\n> >>\n> >> may toggle prefetch off for all remotes, except that the tags and\n> >> the local branches of the remote \"origin\" are prefetched. Instead\n> >> of a multi-value configuration variable (like remote.*.fetch) where\n> >> we need to worry about clearing convention, we can use a regular\n> >> \"last one wins\" variable that is whitespace separated patterns, as\n> >> such a pattern can never have a whitespace in it.\n> > which is what my implementation is based on.\n> \n> I am fine with space separated list or multi-valued variable. The\n> only difference is that with multi-valued list, we'd need to worry\n> about ensuring that we have a way to \"clear\" the values we have seen\n> so far. It has plenty of precedence and is not a rocket science.\n> The above, if I recall correctly, was solely about the need for\n> \"catch-all default\" (aka \"*\" remote) and not about multi-value vs\n> space separated last-one-wins value at all. IOW, the above could\n> have been\n> \n> [remote \"origin\"]\n> \tprefetch = true\n> \tprefetchref = refs/heads/*\n> \tprefetchref = refs/tags/*\n> [remote \"*\"]\n> \tprefetch = false\n> \n> and conveyed exactly what I wanted to say in the message you quoted.\n\nAh, I missed your mail here. I replied to this bit in a parallel email.\n\n> In any case, I somehow thought that we discarded the arrangement\n> with \"*\" wildcard as unworkable. If I remember the discussion\n> before I left correctly, didn't it turn out to be troublesome to\n> have [remote \"*\"] section because existing code would need to\n> enumerate configured remotes, and we do not want to see \"*\" listed?\n\nI wouldn't say unworkable, but it certainly isn't as easy as just adding\nthe new syntax.\n\n> If we found a workable solution to that while I was away, that would\n> be great, but I haven't looked at what this latest round of the\n> series does to solve it (yet). Perhaps teaching \"git remote\" and\n> \"git fetch --all\" to skip \"*\" while enumerating remotes was\n> sufficient? I dunno.\n\nSo yes, we'd have to teach Git to ignore \"*\" remotes in many places. I\nwould hope that it isn't all that involved and that we only need to\nadjust a couple of places to ignore \"*\". But the remote logic is\nsomewhat outside of my area of expertise, so my hope might be misplaced.\n\nIf so, we might think about using a different syntax to achieve the same\nthing.\n\nPatrick\n"},{"id":"506705","messageId":"xmqqo72spyco.fsf@gitster.g","threadId":"62083","inReplyTo":"ZysSEJUqSKUq4PKb@pks.im","subject":"Re: [PATCH v4] remote: allow specifying refs to prefetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-06T08:20:39Z","receivedAt":"2024-11-06T08:20:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> So yes, we'd have to teach Git to ignore \"*\" remotes in many places. I\n> would hope that it isn't all that involved and that we only need to\n> adjust a couple of places to ignore \"*\". But the remote logic is\n> somewhat outside of my area of expertise, so my hope might be misplaced.\n\nI am hoping that it would not be too painful to exclude '*' from\nenumeration, and also, \"git fetch '*'\" and other things that expect\nthe name of a remote do not say silly things like \"ah, there is\n\"remote.*.\" configuration defined, so '*' must be a name\nof a remote\" ;-)\n\nIt might take certain refactoring, but if it results in a cleaner\ncodebase, that is also a welcome outcome.\n\nThanks.\n\n\n"},{"id":"506710","messageId":"43dd8960-4438-402e-ad45-973565a6860e@gmail.com","threadId":"62083","inReplyTo":"ZysQvUyxgdRqjvj2@pks.im","subject":"Re: [PATCH v4] remote: allow specifying refs to prefetch","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-11-06T11:04:48Z","receivedAt":"2024-11-06T11:04:51Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 06/11/2024 06:46, Patrick Steinhardt wrote:\n> On Tue, Nov 05, 2024 at 09:56:52PM +0530, Shubham Kanodia wrote:\n>>\n>> Let me add context here based on a few things that have already been discussed\n>> as a part of this thread or an earlier discussion on\n>> https://lore.kernel.org/git/CAG=Um+1wTbXn_RN+LOCrpZpSNR_QF582PszxNyhz5anVHtBp+w@mail.gmail.com/\n>>\n>>> I'm coming rather late to the party and simply want to review this so\n>>> that the thread gets revived. So my context may be lacking, please\n>>> forgive me if I am reopening things that were already discussed.\n>>\n>> I don't have a particular preference here, and this was discussed in\n>> an earlier thread\n>> where Junio opined (https://lore.kernel.org/git/xmqq5xrcn2k1.fsf@gitster.g/—\n> \n> Fair enough, thanks for the pointer! The reason stated by Junio is that\n> having a space-separated list of refspecs makes it easier to reset the\n> refspec again, as we can simply use a \"last-one-wins\" pattern. And while\n> I understand that, I would claim that we already have a properly\n> established way to reset multi-valued lists by first assigning an empty\n> value:\n> \n> [remote \"origin\"]\n> prefetchref = refs/heads/*\n> prefetchref = refs/tags/*\n> prefetchref =\n> prefetchref = refs/heads/*\n> \n> The final value would be only \"refs/heads/*\" due to the reset.\n> \n> So overall I'm still leaning into the direction of making this a\n> multi-valued list for the sake of consistency with other refspec\n> configs. @Junio Let me know in case you disagree though.\n\nIt is also easier to manipulate the list with \"git config\" when it is \nmultivalued as one can add values and \"git config --add\" and remove \nspecific entries with \"git config --unset \". With a \nsingle entry to append a new value one has to resort to something like\n\n\tgit config \"$(echo $(git config ) )\"\n\nwhich is not very user friendly. Deleting a value is even less friendly.\n\nBest Wishes\n\nPhillip\n\n"}]}