{"thread":{"id":"61026","subject":"[PATCH 0/4] some v2 capability advertisement cleanups","startedAt":"2024-02-28T22:46:26Z","lastAt":"2024-03-04T10:02:03Z","messageCount":15,"participants":["Jeff King","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"489636","messageId":"20240228224625.GA1158651@coredump.intra.peff.net","threadId":"61026","inReplyTo":null,"subject":"[PATCH 0/4] some v2 capability advertisement cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-28T22:46:25Z","receivedAt":"2024-02-28T22:46:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"While working on another series, I noticed that upload-pack will accept\nthe \"packfile-uris\" directive even if we didn't advertise it. That's not\na huge deal in practice, but the spec says we're not supposed to. And\nwhile cleaning that up, I noticed a bit of duplication in the existing\nadvertisement/allow code.\n\nSo patches 1-3 clean up the situation a bit, and then patch 4 tightens\nup the packfile-uris handling.\n\nThere are some small textual conflicts with the series I just posted in:\n\n  https://lore.kernel.org/git/20240228223700.GA1157826@coredump.intra.peff.net/\n\nI'm happy to prepare this on top of that if it's easier.\n\n  [1/4]: upload-pack: use repository struct to get config\n  [2/4]: upload-pack: centralize setup of sideband-all config\n  [3/4]: upload-pack: use existing config mechanism for advertisement\n  [4/4]: upload-pack: only accept packfile-uris if we advertised it\n\n t/t5702-protocol-v2.sh | 18 +++++++++++++\n upload-pack.c          | 58 +++++++++++++++++++-----------------------\n 2 files changed, 44 insertions(+), 32 deletions(-)\n\n-Peff\n"},{"id":"489637","messageId":"20240228224647.GA1158898@coredump.intra.peff.net","threadId":"61026","inReplyTo":"20240228224625.GA1158651@coredump.intra.peff.net","subject":"[PATCH 1/4] upload-pack: use repository struct to get config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-28T22:46:47Z","receivedAt":"2024-02-28T22:46:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our upload_pack_v2() function gets a repository struct, but we ignore it\ntotally.  In practice this doesn't cause any problems, as it will never\ndiffer from the_repository. But in the spirit of taking a small step\ntowards getting rid of the_repository, let's at least starting using it\nto grab config. There are probably other spots that could benefit, but\nit's a start.\n\nNote that we don't need to pass the repo for protected_config(); the\nwhole point there is that we are not looking at repo config, so there is\nno repo-specific version of the function.\n\nFor the v0 version of the protocol, we're not passed a repository\nstruct, so we'll continue to use the_repository there.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n upload-pack.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 2537affa90..e156c1e472 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1385,9 +1385,10 @@ static int upload_pack_protected_config(const char *var, const char *value,\n \treturn 0;\n }\n \n-static void get_upload_pack_config(struct upload_pack_data *data)\n+static void get_upload_pack_config(struct repository *r,\n+\t\t\t\t   struct upload_pack_data *data)\n {\n-\tgit_config(upload_pack_config, data);\n+\trepo_config(r, upload_pack_config, data);\n \tgit_protected_config(upload_pack_protected_config, data);\n }\n \n@@ -1398,7 +1399,7 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,\n \tstruct upload_pack_data data;\n \n \tupload_pack_data_init(&data);\n-\tget_upload_pack_config(&data);\n+\tget_upload_pack_config(the_repository, &data);\n \n \tdata.stateless_rpc = stateless_rpc;\n \tdata.timeout = timeout;\n@@ -1771,7 +1772,7 @@ enum fetch_state {\n \tFETCH_DONE,\n };\n \n-int upload_pack_v2(struct repository *r UNUSED, struct packet_reader *request)\n+int upload_pack_v2(struct repository *r, struct packet_reader *request)\n {\n \tenum fetch_state state = FETCH_PROCESS_ARGS;\n \tstruct upload_pack_data data;\n@@ -1780,7 +1781,7 @@ int upload_pack_v2(struct repository *r UNUSED, struct packet_reader *request)\n \n \tupload_pack_data_init(&data);\n \tdata.use_sideband = LARGE_PACKET_MAX;\n-\tget_upload_pack_config(&data);\n+\tget_upload_pack_config(r, &data);\n \n \twhile (state != FETCH_DONE) {\n \t\tswitch (state) {\n-- \n2.44.0.rc2.424.gbdbf4d014b\n\n"},{"id":"489638","messageId":"20240228224718.GB1158898@coredump.intra.peff.net","threadId":"61026","inReplyTo":"20240228224625.GA1158651@coredump.intra.peff.net","subject":"[PATCH 2/4] upload-pack: centralize setup of sideband-all config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-28T22:47:18Z","receivedAt":"2024-02-28T22:47:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We read uploadpack.allowsidebandall to set a matching flag in our\nupload_pack_data struct. But for our tests, we also respect\nGIT_TEST_SIDEBAND_ALL from the environment, and anybody looking at the\nflag in the struct needs to remember to check both. There's only one\nsuch piece of code now, but we're about to add another.\n\nSo let's have the config step actually fold the environment value into\nthe struct, letting the rest of the code use the flag in the obvious\nway.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n upload-pack.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex e156c1e472..6bda20754d 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1390,6 +1390,8 @@ static void get_upload_pack_config(struct repository *r,\n {\n \trepo_config(r, upload_pack_config, data);\n \tgit_protected_config(upload_pack_protected_config, data);\n+\n+\tdata->allow_sideband_all |= git_env_bool(\"GIT_TEST_SIDEBAND_ALL\", 0);\n }\n \n void upload_pack(const int advertise_refs, const int stateless_rpc,\n@@ -1639,8 +1641,7 @@ static void process_args(struct packet_reader *request,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif ((git_env_bool(\"GIT_TEST_SIDEBAND_ALL\", 0) ||\n-\t\t     data->allow_sideband_all) &&\n+\t\tif (data->allow_sideband_all &&\n \t\t    !strcmp(arg, \"sideband-all\")) {\n \t\t\tdata->writer.use_sideband = 1;\n \t\t\tcontinue;\n-- \n2.44.0.rc2.424.gbdbf4d014b\n\n"},{"id":"489640","messageId":"20240228224818.GA1158952@coredump.intra.peff.net","threadId":"61026","inReplyTo":"20240228224625.GA1158651@coredump.intra.peff.net","subject":"[PATCH 3/4] upload-pack: use existing config mechanism for advertisement","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-28T22:48:18Z","receivedAt":"2024-02-28T22:48:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When serving a v2 capabilities request, we call upload_pack_advertise()\nto tell us the set of features we can advertise to the client. That\ninvolves looking at various config options, all of which need to be kept\nin sync with the rules we use in upload_pack_config to set flags like\nallow_filter, allow_sideband_all, and so on. If these two pieces of code\nget out of sync then we may refuse to respect a capability we\nadvertised, or vice versa accept one that we should not.\n\nInstead, let's call the same config helper that we'll use for processing\nthe actual client request, and then just pick the values out of the\nresulting struct. This is only a little bit shorter than the current\ncode, but we don't repeat any policy logic (e.g., we don't have to worry\nabout the magic sideband-all environment variable here anymore).\n\nAnd this reveals a gap in the existing code: there is no struct flag for\nthe packfile-uris capability (we accept it even if it is not advertised,\nwhich we should not). We'll leave the advertisement code for now and\ndeal with it in the next patch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n upload-pack.c | 26 ++++++++++----------------\n 1 file changed, 10 insertions(+), 16 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 6bda20754d..491ef51daa 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1841,41 +1841,35 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request)\n int upload_pack_advertise(struct repository *r,\n \t\t\t  struct strbuf *value)\n {\n+\tstruct upload_pack_data data;\n+\n+\tupload_pack_data_init(&data);\n+\tget_upload_pack_config(r, &data);\n+\n \tif (value) {\n-\t\tint allow_filter_value;\n-\t\tint allow_ref_in_want;\n-\t\tint allow_sideband_all_value;\n \t\tchar *str = NULL;\n \n \t\tstrbuf_addstr(value, \"shallow wait-for-done\");\n \n-\t\tif (!repo_config_get_bool(r,\n-\t\t\t\t\t \"uploadpack.allowfilter\",\n-\t\t\t\t\t &allow_filter_value) &&\n-\t\t    allow_filter_value)\n+\t\tif (data.allow_filter)\n \t\t\tstrbuf_addstr(value, \" filter\");\n \n-\t\tif (!repo_config_get_bool(r,\n-\t\t\t\t\t \"uploadpack.allowrefinwant\",\n-\t\t\t\t\t &allow_ref_in_want) &&\n-\t\t    allow_ref_in_want)\n+\t\tif (data.allow_ref_in_want)\n \t\t\tstrbuf_addstr(value, \" ref-in-want\");\n \n-\t\tif (git_env_bool(\"GIT_TEST_SIDEBAND_ALL\", 0) ||\n-\t\t    (!repo_config_get_bool(r,\n-\t\t\t\t\t   \"uploadpack.allowsidebandall\",\n-\t\t\t\t\t   &allow_sideband_all_value) &&\n-\t\t     allow_sideband_all_value))\n+\t\tif (data.allow_sideband_all)\n \t\t\tstrbuf_addstr(value, \" sideband-all\");\n \n \t\tif (!repo_config_get_string(r,\n \t\t\t\t\t    \"uploadpack.blobpackfileuri\",\n \t\t\t\t\t    &str) &&\n \t\t    str) {\n \t\t\tstrbuf_addstr(value, \" packfile-uris\");\n \t\t\tfree(str);\n \t\t}\n \t}\n \n+\tupload_pack_data_clear(&data);\n+\n \treturn 1;\n }\n-- \n2.44.0.rc2.424.gbdbf4d014b\n\n"},{"id":"489641","messageId":"20240228225050.GA1159078@coredump.intra.peff.net","threadId":"61026","inReplyTo":"20240228224625.GA1158651@coredump.intra.peff.net","subject":"[PATCH 4/4] upload-pack: only accept packfile-uris if we advertised it","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-28T22:50:50Z","receivedAt":"2024-02-28T22:50:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Clients are only supposed to request particular capabilities or features\nif the server advertised them. For the \"packfile-uris\" feature, we only\nadvertise it if uploadpack.blobpacfileuri is set, but we always accept a\nrequest from the client regardless.\n\nIn practice this doesn't really hurt anything, as we'd pass the client's\nprotocol list on to pack-objects, which ends up ignoring it. But we\nshould try to follow the protocol spec, and tightening this up may catch\nbuggy or misbehaving clients more easily.\n\nThanks to recent refactoring, we can hoist the config check from\nupload_pack_advertise() into upload_pack_config(). Note the subtle\nhandling of a value-less bool (which does not count for triggering an\nadvertisement).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI suspect in the long term that we may have other ways to trigger this\nfeature than the static blobpackfileuri config (e.g., a hook that knows\nabout site-specific packfiles \"somehow\"). So we may need to update the\ntest later for that, but presumably in the vanilla config we'll continue\nto skip advertising it.\n\n t/t5702-protocol-v2.sh | 18 ++++++++++++++++++\n upload-pack.c          | 16 +++++++---------\n 2 files changed, 25 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 6ef4971845..902e42c1c0 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -778,6 +778,24 @@ test_expect_success 'archive with custom path does not request v2' '\n \t! grep ^GIT_PROTOCOL env.trace\n '\n \n+test_expect_success 'reject client packfile-uris if not advertised' '\n+\t{\n+\t\tpacketize command=fetch &&\n+\t\tprintf 0001 &&\n+\t\tpacketize packfile-uris https &&\n+\t\tpacketize done &&\n+\t\tprintf 0000\n+\t} >input &&\n+\ttest_must_fail env GIT_PROTOCOL=version=2 \\\n+\t\tgit upload-pack client <input &&\n+\ttest_must_fail env GIT_PROTOCOL=version=2 \\\n+\t\tgit -c uploadpack.blobpackfileuri \\\n+\t\tupload-pack client <input &&\n+\tGIT_PROTOCOL=version=2 \\\n+\t\tgit -c uploadpack.blobpackfileuri=anything \\\n+\t\tupload-pack client <input\n+'\n+\n # Test protocol v2 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 491ef51daa..66f4de9d87 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -113,6 +113,7 @@ struct upload_pack_data {\n \tunsigned done : 1;\t\t\t\t\t/* v2 only */\n \tunsigned allow_ref_in_want : 1;\t\t\t\t/* v2 only */\n \tunsigned allow_sideband_all : 1;\t\t\t/* v2 only */\n+\tunsigned allow_packfile_uris : 1;\t\t\t/* v2 only */\n \tunsigned advertise_sid : 1;\n \tunsigned sent_capabilities : 1;\n };\n@@ -1362,6 +1363,9 @@ static int upload_pack_config(const char *var, const char *value,\n \t\tdata->allow_ref_in_want = git_config_bool(var, value);\n \t} else if (!strcmp(\"uploadpack.allowsidebandall\", var)) {\n \t\tdata->allow_sideband_all = git_config_bool(var, value);\n+\t} else if (!strcmp(\"uploadpack.blobpackfileuri\", var)) {\n+\t\tif (value)\n+\t\t\tdata->allow_packfile_uris = 1;\n \t} else if (!strcmp(\"core.precomposeunicode\", var)) {\n \t\tprecomposed_unicode = git_config_bool(var, value);\n \t} else if (!strcmp(\"transfer.advertisesid\", var)) {\n@@ -1647,7 +1651,8 @@ static void process_args(struct packet_reader *request,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (skip_prefix(arg, \"packfile-uris \", &p)) {\n+\t\tif (data->allow_packfile_uris &&\n+\t\t    skip_prefix(arg, \"packfile-uris \", &p)) {\n \t\t\tstring_list_split(&data->uri_protocols, p, ',', -1);\n \t\t\tcontinue;\n \t\t}\n@@ -1847,8 +1852,6 @@ int upload_pack_advertise(struct repository *r,\n \tget_upload_pack_config(r, &data);\n \n \tif (value) {\n-\t\tchar *str = NULL;\n-\n \t\tstrbuf_addstr(value, \"shallow wait-for-done\");\n \n \t\tif (data.allow_filter)\n@@ -1860,13 +1863,8 @@ int upload_pack_advertise(struct repository *r,\n \t\tif (data.allow_sideband_all)\n \t\t\tstrbuf_addstr(value, \" sideband-all\");\n \n-\t\tif (!repo_config_get_string(r,\n-\t\t\t\t\t    \"uploadpack.blobpackfileuri\",\n-\t\t\t\t\t    &str) &&\n-\t\t    str) {\n+\t\tif (data.allow_packfile_uris)\n \t\t\tstrbuf_addstr(value, \" packfile-uris\");\n-\t\t\tfree(str);\n-\t\t}\n \t}\n \n \tupload_pack_data_clear(&data);\n-- \n2.44.0.rc2.424.gbdbf4d014b\n"},{"id":"489642","messageId":"xmqq5xy8jgtn.fsf@gitster.g","threadId":"61026","inReplyTo":"20240228225050.GA1159078@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] upload-pack: only accept packfile-uris if we advertised it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-28T23:43:16Z","receivedAt":"2024-02-28T23:43:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I suspect in the long term that we may have other ways to trigger this\n> feature than the static blobpackfileuri config (e.g., a hook that knows\n> about site-specific packfiles \"somehow\"). So we may need to update the\n> test later for that, but presumably in the vanilla config we'll continue\n> to skip advertising it.\n\nSounds quite sensible.  Thanks for a series that is very pleasant to\nread.\n\n>\n>  t/t5702-protocol-v2.sh | 18 ++++++++++++++++++\n>  upload-pack.c          | 16 +++++++---------\n>  2 files changed, 25 insertions(+), 9 deletions(-)\n>\n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 6ef4971845..902e42c1c0 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -778,6 +778,24 @@ test_expect_success 'archive with custom path does not request v2' '\n>  \t! grep ^GIT_PROTOCOL env.trace\n>  '\n>  \n> +test_expect_success 'reject client packfile-uris if not advertised' '\n> +\t{\n> +\t\tpacketize command=fetch &&\n> +\t\tprintf 0001 &&\n> +\t\tpacketize packfile-uris https &&\n> +\t\tpacketize done &&\n> +\t\tprintf 0000\n> +\t} >input &&\n> +\ttest_must_fail env GIT_PROTOCOL=version=2 \\\n> +\t\tgit upload-pack client <input &&\n> +\ttest_must_fail env GIT_PROTOCOL=version=2 \\\n> +\t\tgit -c uploadpack.blobpackfileuri \\\n> +\t\tupload-pack client <input &&\n> +\tGIT_PROTOCOL=version=2 \\\n> +\t\tgit -c uploadpack.blobpackfileuri=anything \\\n> +\t\tupload-pack client <input\n> +'\n> +\n>  # Test protocol v2 with 'http://' transport\n>  #\n>  . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 491ef51daa..66f4de9d87 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -113,6 +113,7 @@ struct upload_pack_data {\n>  \tunsigned done : 1;\t\t\t\t\t/* v2 only */\n>  \tunsigned allow_ref_in_want : 1;\t\t\t\t/* v2 only */\n>  \tunsigned allow_sideband_all : 1;\t\t\t/* v2 only */\n> +\tunsigned allow_packfile_uris : 1;\t\t\t/* v2 only */\n>  \tunsigned advertise_sid : 1;\n>  \tunsigned sent_capabilities : 1;\n>  };\n> @@ -1362,6 +1363,9 @@ static int upload_pack_config(const char *var, const char *value,\n>  \t\tdata->allow_ref_in_want = git_config_bool(var, value);\n>  \t} else if (!strcmp(\"uploadpack.allowsidebandall\", var)) {\n>  \t\tdata->allow_sideband_all = git_config_bool(var, value);\n> +\t} else if (!strcmp(\"uploadpack.blobpackfileuri\", var)) {\n> +\t\tif (value)\n> +\t\t\tdata->allow_packfile_uris = 1;\n>  \t} else if (!strcmp(\"core.precomposeunicode\", var)) {\n>  \t\tprecomposed_unicode = git_config_bool(var, value);\n>  \t} else if (!strcmp(\"transfer.advertisesid\", var)) {\n> @@ -1647,7 +1651,8 @@ static void process_args(struct packet_reader *request,\n>  \t\t\tcontinue;\n>  \t\t}\n>  \n> -\t\tif (skip_prefix(arg, \"packfile-uris \", &p)) {\n> +\t\tif (data->allow_packfile_uris &&\n> +\t\t    skip_prefix(arg, \"packfile-uris \", &p)) {\n>  \t\t\tstring_list_split(&data->uri_protocols, p, ',', -1);\n>  \t\t\tcontinue;\n>  \t\t}\n> @@ -1847,8 +1852,6 @@ int upload_pack_advertise(struct repository *r,\n>  \tget_upload_pack_config(r, &data);\n>  \n>  \tif (value) {\n> -\t\tchar *str = NULL;\n> -\n>  \t\tstrbuf_addstr(value, \"shallow wait-for-done\");\n>  \n>  \t\tif (data.allow_filter)\n> @@ -1860,13 +1863,8 @@ int upload_pack_advertise(struct repository *r,\n>  \t\tif (data.allow_sideband_all)\n>  \t\t\tstrbuf_addstr(value, \" sideband-all\");\n>  \n> -\t\tif (!repo_config_get_string(r,\n> -\t\t\t\t\t    \"uploadpack.blobpackfileuri\",\n> -\t\t\t\t\t    &str) &&\n> -\t\t    str) {\n> +\t\tif (data.allow_packfile_uris)\n>  \t\t\tstrbuf_addstr(value, \" packfile-uris\");\n> -\t\t\tfree(str);\n> -\t\t}\n>  \t}\n>  \n>  \tupload_pack_data_clear(&data);\n"},{"id":"489643","messageId":"xmqq1q8wjgft.fsf@gitster.g","threadId":"61026","inReplyTo":"20240228224625.GA1158651@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] some v2 capability advertisement cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-28T23:51:34Z","receivedAt":"2024-02-28T23:51:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> There are some small textual conflicts with the series I just posted in:\n>\n>   https://lore.kernel.org/git/20240228223700.GA1157826@coredump.intra.peff.net/\n>\n> I'm happy to prepare this on top of that if it's easier.\n\nThanks for a heads-up.\n\nThe other one merged first and then this one does give the\nfollowing, which does not look like a huge deal.\n\ndiff --cc upload-pack.c\nindex 281bdf85c9,66f4de9d87..0000000000\n--- i/upload-pack.c\n+++ w/upload-pack.c\n@@@ -113,7 -113,7 +113,8 @@@ struct upload_pack_data \n  \tunsigned done : 1;\t\t\t\t\t/* v2 only */\n  \tunsigned allow_ref_in_want : 1;\t\t\t\t/* v2 only */\n  \tunsigned allow_sideband_all : 1;\t\t\t/* v2 only */\n +\tunsigned seen_haves : 1;\t\t\t\t/* v2 only */\n+ \tunsigned allow_packfile_uris : 1;\t\t\t/* v2 only */\n  \tunsigned advertise_sid : 1;\n  \tunsigned sent_capabilities : 1;\n  };\n@@@ -1648,10 -1651,8 +1654,11 @@@ static void process_args(struct packet_\n  \t\t\tcontinue;\n  \t\t}\n  \n- \t\tif (skip_prefix(arg, \"packfile-uris \", &p)) {\n+ \t\tif (data->allow_packfile_uris &&\n+ \t\t    skip_prefix(arg, \"packfile-uris \", &p)) {\n +\t\t\tif (data->uri_protocols.nr)\n +\t\t\t\tsend_err_and_die(data,\n +\t\t\t\t\t\t \"multiple packfile-uris lines forbidden\");\n  \t\t\tstring_list_split(&data->uri_protocols, p, ',', -1);\n  \t\t\tcontinue;\n  \t\t}\n"},{"id":"489645","messageId":"20240229004446.GA1654078@coredump.intra.peff.net","threadId":"61026","inReplyTo":"xmqq1q8wjgft.fsf@gitster.g","subject":"Re: [PATCH 0/4] some v2 capability advertisement cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-29T00:44:46Z","receivedAt":"2024-02-29T00:44:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 28, 2024 at 03:51:34PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > There are some small textual conflicts with the series I just posted in:\n> >\n> >   https://lore.kernel.org/git/20240228223700.GA1157826@coredump.intra.peff.net/\n> >\n> > I'm happy to prepare this on top of that if it's easier.\n> \n> Thanks for a heads-up.\n> \n> The other one merged first and then this one does give the\n> following, which does not look like a huge deal.\n\nYep, that matches the resolution I did locally.\n\n-Peff\n"},{"id":"489651","messageId":"20240229054239.GA1669526@coredump.intra.peff.net","threadId":"61026","inReplyTo":"20240228225050.GA1159078@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] upload-pack: only accept packfile-uris if we advertised it","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-29T05:42:39Z","receivedAt":"2024-02-29T05:42:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 28, 2024 at 05:50:50PM -0500, Jeff King wrote:\n\n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 6ef4971845..902e42c1c0 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -778,6 +778,24 @@ test_expect_success 'archive with custom path does not request v2' '\n>  \t! grep ^GIT_PROTOCOL env.trace\n>  '\n>  \n> +test_expect_success 'reject client packfile-uris if not advertised' '\n> +\t{\n> +\t\tpacketize command=fetch &&\n> +\t\tprintf 0001 &&\n> +\t\tpacketize packfile-uris https &&\n> +\t\tpacketize done &&\n> +\t\tprintf 0000\n> +\t} >input &&\n> +\ttest_must_fail env GIT_PROTOCOL=version=2 \\\n> +\t\tgit upload-pack client <input &&\n> +\ttest_must_fail env GIT_PROTOCOL=version=2 \\\n> +\t\tgit -c uploadpack.blobpackfileuri \\\n> +\t\tupload-pack client <input &&\n> +\tGIT_PROTOCOL=version=2 \\\n> +\t\tgit -c uploadpack.blobpackfileuri=anything \\\n> +\t\tupload-pack client <input\n> +'\n\nSorry, this needs one tweak to pass under the sha256 CI job:\n\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 902e42c1c0..1ef540f73d 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -781,6 +781,7 @@ test_expect_success 'archive with custom path does not request v2' '\n test_expect_success 'reject client packfile-uris if not advertised' '\n \t{\n \t\tpacketize command=fetch &&\n+\t\tpacketize object-format=$(test_oid algo) &&\n \t\tprintf 0001 &&\n \t\tpacketize packfile-uris https &&\n \t\tpacketize done &&\n\nOtherwise the server complains that the other side did not respect its\nadvertised object-format (I sure am glad to have included the final\n\"hey, this input works, right?\" test there, as that is what caught it).\n\n-Peff\n"},{"id":"489667","messageId":"xmqqmsrji5zr.fsf@gitster.g","threadId":"61026","inReplyTo":"20240229054239.GA1669526@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] upload-pack: only accept packfile-uris if we advertised it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-29T16:34:48Z","receivedAt":"2024-02-29T16:34:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Sorry, this needs one tweak to pass under the sha256 CI job:\n>\n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 902e42c1c0..1ef540f73d 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -781,6 +781,7 @@ test_expect_success 'archive with custom path does not request v2' '\n>  test_expect_success 'reject client packfile-uris if not advertised' '\n>  \t{\n>  \t\tpacketize command=fetch &&\n> +\t\tpacketize object-format=$(test_oid algo) &&\n>  \t\tprintf 0001 &&\n>  \t\tpacketize packfile-uris https &&\n>  \t\tpacketize done &&\n>\n> Otherwise the server complains that the other side did not respect its\n> advertised object-format (I sure am glad to have included the final\n> \"hey, this input works, right?\" test there, as that is what caught it).\n\nAh, good finding.  Will use it to amend.\n\nI wonder if it is still worth testing if the command is happy with\nan input that lacks object-format capability under SHA-1 mode.  This\ntest piece is primarily about packfile-uris, so in that sense, we\nare not all that interested in that unspecified client object-format\ndefaults to the initial value of serve.c:client_hash_algo (which is\nSHA-1), and not testing that here is perfectly fine, I guess.\n\nThanks.\n"},{"id":"489729","messageId":"20240301071041.GA2682168@coredump.intra.peff.net","threadId":"61026","inReplyTo":"xmqqmsrji5zr.fsf@gitster.g","subject":"Re: [PATCH 4/4] upload-pack: only accept packfile-uris if we advertised it","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-01T07:10:41Z","receivedAt":"2024-03-01T07:10:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 29, 2024 at 08:34:48AM -0800, Junio C Hamano wrote:\n\n> > Otherwise the server complains that the other side did not respect its\n> > advertised object-format (I sure am glad to have included the final\n> > \"hey, this input works, right?\" test there, as that is what caught it).\n> \n> Ah, good finding.  Will use it to amend.\n> \n> I wonder if it is still worth testing if the command is happy with\n> an input that lacks object-format capability under SHA-1 mode.  This\n> test piece is primarily about packfile-uris, so in that sense, we\n> are not all that interested in that unspecified client object-format\n> defaults to the initial value of serve.c:client_hash_algo (which is\n> SHA-1), and not testing that here is perfectly fine, I guess.\n\nYeah, if we want to test it, I'd prefer to do so separately as its own\ntest rather than keeping it as a subtle side effect of this one. I\nlooked around to see if it might exist already, but I didn't find one\n(OTOH, it is hard to grep for since we are looking for the _absence_ of\nan object-format line in hand-crafted input).\n\nBut taking a step back, why do we care about this case? It is because\nolder clients that speak v2 but do not yet know about object-format will\nmake a request without it, and they should still work in sha1 mode. So\nthe more general thing to test here is whether those older versions can\nsuccessfully fetch from a newer server.\n\nThere are tests in t/interop for cloning and fetching from a different\nversion. I kind of doubt anybody runs them regularly, though (and\npicking the interesting versions to find this case isn't entirely\ntrivial). So I dunno.\n\n-Peff\n"},{"id":"489840","messageId":"ZeV7-NkUwT53IvIa@tanuki","threadId":"61026","inReplyTo":"20240228224625.GA1158651@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] some v2 capability advertisement cleanups","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T07:44:56Z","receivedAt":"2024-03-04T07:45:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Feb 28, 2024 at 05:46:25PM -0500, Jeff King wrote:\n> While working on another series, I noticed that upload-pack will accept\n> the \"packfile-uris\" directive even if we didn't advertise it. That's not\n> a huge deal in practice, but the spec says we're not supposed to. And\n> while cleaning that up, I noticed a bit of duplication in the existing\n> advertisement/allow code.\n> \n> So patches 1-3 clean up the situation a bit, and then patch 4 tightens\n> up the packfile-uris handling.\n\nThis patch series was really easy to follow and feels like the right\nthing to do. I've got a couple of nits, but none of them are important\nenough to warrant a reroll.\n\nThanks!\n\nPatrick\n\n> There are some small textual conflicts with the series I just posted in:\n> \n>   https://lore.kernel.org/git/20240228223700.GA1157826@coredump.intra.peff.net/\n> \n> I'm happy to prepare this on top of that if it's easier.\n> \n>   [1/4]: upload-pack: use repository struct to get config\n>   [2/4]: upload-pack: centralize setup of sideband-all config\n>   [3/4]: upload-pack: use existing config mechanism for advertisement\n>   [4/4]: upload-pack: only accept packfile-uris if we advertised it\n> \n>  t/t5702-protocol-v2.sh | 18 +++++++++++++\n>  upload-pack.c          | 58 +++++++++++++++++++-----------------------\n>  2 files changed, 44 insertions(+), 32 deletions(-)\n> \n> -Peff\n> \n"},{"id":"489841","messageId":"ZeV7_obKou-jtyA-@tanuki","threadId":"61026","inReplyTo":"20240228224647.GA1158898@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] upload-pack: use repository struct to get config","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T07:45:02Z","receivedAt":"2024-03-04T07:45:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Feb 28, 2024 at 05:46:47PM -0500, Jeff King wrote:\n> Our upload_pack_v2() function gets a repository struct, but we ignore it\n> totally.  In practice this doesn't cause any problems, as it will never\n> differ from the_repository. But in the spirit of taking a small step\n> towards getting rid of the_repository, let's at least starting using it\n\nNit: s/starting/start\n\nPatrick\n\n> to grab config. There are probably other spots that could benefit, but\n> it's a start.\n> \n> Note that we don't need to pass the repo for protected_config(); the\n> whole point there is that we are not looking at repo config, so there is\n> no repo-specific version of the function.\n> \n> For the v0 version of the protocol, we're not passed a repository\n> struct, so we'll continue to use the_repository there.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  upload-pack.c | 11 ++++++-----\n>  1 file changed, 6 insertions(+), 5 deletions(-)\n> \n> diff --git a/upload-pack.c b/upload-pack.c\n> index 2537affa90..e156c1e472 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -1385,9 +1385,10 @@ static int upload_pack_protected_config(const char *var, const char *value,\n>  \treturn 0;\n>  }\n>  \n> -static void get_upload_pack_config(struct upload_pack_data *data)\n> +static void get_upload_pack_config(struct repository *r,\n> +\t\t\t\t   struct upload_pack_data *data)\n>  {\n> -\tgit_config(upload_pack_config, data);\n> +\trepo_config(r, upload_pack_config, data);\n>  \tgit_protected_config(upload_pack_protected_config, data);\n>  }\n>  \n> @@ -1398,7 +1399,7 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,\n>  \tstruct upload_pack_data data;\n>  \n>  \tupload_pack_data_init(&data);\n> -\tget_upload_pack_config(&data);\n> +\tget_upload_pack_config(the_repository, &data);\n>  \n>  \tdata.stateless_rpc = stateless_rpc;\n>  \tdata.timeout = timeout;\n> @@ -1771,7 +1772,7 @@ enum fetch_state {\n>  \tFETCH_DONE,\n>  };\n>  \n> -int upload_pack_v2(struct repository *r UNUSED, struct packet_reader *request)\n> +int upload_pack_v2(struct repository *r, struct packet_reader *request)\n>  {\n>  \tenum fetch_state state = FETCH_PROCESS_ARGS;\n>  \tstruct upload_pack_data data;\n> @@ -1780,7 +1781,7 @@ int upload_pack_v2(struct repository *r UNUSED, struct packet_reader *request)\n>  \n>  \tupload_pack_data_init(&data);\n>  \tdata.use_sideband = LARGE_PACKET_MAX;\n> -\tget_upload_pack_config(&data);\n> +\tget_upload_pack_config(r, &data);\n>  \n>  \twhile (state != FETCH_DONE) {\n>  \t\tswitch (state) {\n> -- \n> 2.44.0.rc2.424.gbdbf4d014b\n> \n> \n"},{"id":"489842","messageId":"ZeV8BNCr0QN84ZQE@tanuki","threadId":"61026","inReplyTo":"20240228225050.GA1159078@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] upload-pack: only accept packfile-uris if we advertised it","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T07:45:08Z","receivedAt":"2024-03-04T07:45:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Feb 28, 2024 at 05:50:50PM -0500, Jeff King wrote:\n> Clients are only supposed to request particular capabilities or features\n> if the server advertised them. For the \"packfile-uris\" feature, we only\n> advertise it if uploadpack.blobpacfileuri is set, but we always accept a\n\nNit: s/uploadpack.blobpackfileuri. I noticed that this isn't actually\ndocumented in git-config(1), but that's not a problem of this commit.\n\n> request from the client regardless.\n> \n> In practice this doesn't really hurt anything, as we'd pass the client's\n> protocol list on to pack-objects, which ends up ignoring it. But we\n> should try to follow the protocol spec, and tightening this up may catch\n> buggy or misbehaving clients more easily.\n> \n> Thanks to recent refactoring, we can hoist the config check from\n\nNit: s/refactoring/&s.\n\nPatrick\n\n> upload_pack_advertise() into upload_pack_config(). Note the subtle\n> handling of a value-less bool (which does not count for triggering an\n> advertisement).\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I suspect in the long term that we may have other ways to trigger this\n> feature than the static blobpackfileuri config (e.g., a hook that knows\n> about site-specific packfiles \"somehow\"). So we may need to update the\n> test later for that, but presumably in the vanilla config we'll continue\n> to skip advertising it.\n> \n>  t/t5702-protocol-v2.sh | 18 ++++++++++++++++++\n>  upload-pack.c          | 16 +++++++---------\n>  2 files changed, 25 insertions(+), 9 deletions(-)\n> \n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 6ef4971845..902e42c1c0 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -778,6 +778,24 @@ test_expect_success 'archive with custom path does not request v2' '\n>  \t! grep ^GIT_PROTOCOL env.trace\n>  '\n>  \n> +test_expect_success 'reject client packfile-uris if not advertised' '\n> +\t{\n> +\t\tpacketize command=fetch &&\n> +\t\tprintf 0001 &&\n> +\t\tpacketize packfile-uris https &&\n> +\t\tpacketize done &&\n> +\t\tprintf 0000\n> +\t} >input &&\n> +\ttest_must_fail env GIT_PROTOCOL=version=2 \\\n> +\t\tgit upload-pack client <input &&\n> +\ttest_must_fail env GIT_PROTOCOL=version=2 \\\n> +\t\tgit -c uploadpack.blobpackfileuri \\\n> +\t\tupload-pack client <input &&\n> +\tGIT_PROTOCOL=version=2 \\\n> +\t\tgit -c uploadpack.blobpackfileuri=anything \\\n> +\t\tupload-pack client <input\n> +'\n> +\n>  # Test protocol v2 with 'http://' transport\n>  #\n>  . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 491ef51daa..66f4de9d87 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -113,6 +113,7 @@ struct upload_pack_data {\n>  \tunsigned done : 1;\t\t\t\t\t/* v2 only */\n>  \tunsigned allow_ref_in_want : 1;\t\t\t\t/* v2 only */\n>  \tunsigned allow_sideband_all : 1;\t\t\t/* v2 only */\n> +\tunsigned allow_packfile_uris : 1;\t\t\t/* v2 only */\n>  \tunsigned advertise_sid : 1;\n>  \tunsigned sent_capabilities : 1;\n>  };\n> @@ -1362,6 +1363,9 @@ static int upload_pack_config(const char *var, const char *value,\n>  \t\tdata->allow_ref_in_want = git_config_bool(var, value);\n>  \t} else if (!strcmp(\"uploadpack.allowsidebandall\", var)) {\n>  \t\tdata->allow_sideband_all = git_config_bool(var, value);\n> +\t} else if (!strcmp(\"uploadpack.blobpackfileuri\", var)) {\n> +\t\tif (value)\n> +\t\t\tdata->allow_packfile_uris = 1;\n>  \t} else if (!strcmp(\"core.precomposeunicode\", var)) {\n>  \t\tprecomposed_unicode = git_config_bool(var, value);\n>  \t} else if (!strcmp(\"transfer.advertisesid\", var)) {\n> @@ -1647,7 +1651,8 @@ static void process_args(struct packet_reader *request,\n>  \t\t\tcontinue;\n>  \t\t}\n>  \n> -\t\tif (skip_prefix(arg, \"packfile-uris \", &p)) {\n> +\t\tif (data->allow_packfile_uris &&\n> +\t\t    skip_prefix(arg, \"packfile-uris \", &p)) {\n>  \t\t\tstring_list_split(&data->uri_protocols, p, ',', -1);\n>  \t\t\tcontinue;\n>  \t\t}\n> @@ -1847,8 +1852,6 @@ int upload_pack_advertise(struct repository *r,\n>  \tget_upload_pack_config(r, &data);\n>  \n>  \tif (value) {\n> -\t\tchar *str = NULL;\n> -\n>  \t\tstrbuf_addstr(value, \"shallow wait-for-done\");\n>  \n>  \t\tif (data.allow_filter)\n> @@ -1860,13 +1863,8 @@ int upload_pack_advertise(struct repository *r,\n>  \t\tif (data.allow_sideband_all)\n>  \t\t\tstrbuf_addstr(value, \" sideband-all\");\n>  \n> -\t\tif (!repo_config_get_string(r,\n> -\t\t\t\t\t    \"uploadpack.blobpackfileuri\",\n> -\t\t\t\t\t    &str) &&\n> -\t\t    str) {\n> +\t\tif (data.allow_packfile_uris)\n>  \t\t\tstrbuf_addstr(value, \" packfile-uris\");\n> -\t\t\tfree(str);\n> -\t\t}\n>  \t}\n>  \n>  \tupload_pack_data_clear(&data);\n> -- \n> 2.44.0.rc2.424.gbdbf4d014b\n> \n"},{"id":"489865","messageId":"20240304100202.GC3723539@coredump.intra.peff.net","threadId":"61026","inReplyTo":"ZeV7-NkUwT53IvIa@tanuki","subject":"Re: [PATCH 0/4] some v2 capability advertisement cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-04T10:02:02Z","receivedAt":"2024-03-04T10:02:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 04, 2024 at 08:44:56AM +0100, Patrick Steinhardt wrote:\n\n> On Wed, Feb 28, 2024 at 05:46:25PM -0500, Jeff King wrote:\n> > While working on another series, I noticed that upload-pack will accept\n> > the \"packfile-uris\" directive even if we didn't advertise it. That's not\n> > a huge deal in practice, but the spec says we're not supposed to. And\n> > while cleaning that up, I noticed a bit of duplication in the existing\n> > advertisement/allow code.\n> > \n> > So patches 1-3 clean up the situation a bit, and then patch 4 tightens\n> > up the packfile-uris handling.\n> \n> This patch series was really easy to follow and feels like the right\n> thing to do. I've got a couple of nits, but none of them are important\n> enough to warrant a reroll.\n\nThanks for reviewing. The typos you found are definitely all wrong, but\nI think the topic is in 'next' already (and they were all just in the\ncommit messages).\n\n-Peff\n"}]}