{"thread":{"id":"58937","subject":"[PATCH 2/3] bundle-uri: advertise based on repo config","startedAt":"2022-12-12T17:34:06Z","lastAt":"2022-12-20T21:08:25Z","messageCount":17,"participants":["Derrick Stolee via GitGitGadget","Victoria Dye","Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"468908","messageId":"857d1abec4cf124e011c7f84276ce105cb5b3a96.1670866407.git.gitgitgadget@gmail.com","threadId":"58937","inReplyTo":"pull.1443.git.1670866407.gitgitgadget@gmail.com","subject":"[PATCH 2/3] bundle-uri: advertise based on repo config","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-12T17:33:25Z","receivedAt":"2022-12-12T17:34:06Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe bundle_uri_advertise() method was not using its repository\nparameter, but this is a mistake. Use repo_config_get_maybe_bool()\ninstead of git_config_maybe_bool().\n\nSigned-off-by: Derrick Stolee \n---\n bundle-uri.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/bundle-uri.c b/bundle-uri.c\nindex 8efb4e7acad..5f158cc52e1 100644\n--- a/bundle-uri.c\n+++ b/bundle-uri.c\n@@ -610,7 +610,7 @@ int bundle_uri_advertise(struct repository *r, struct strbuf *value)\n \t\tgoto cached;\n \n \tadvertise_bundle_uri = 0;\n-\tgit_config_get_maybe_bool(\"uploadpack.advertisebundleuris\", &advertise_bundle_uri);\n+\trepo_config_get_maybe_bool(r, \"uploadpack.advertisebundleuris\", &advertise_bundle_uri);\n \n cached:\n \treturn advertise_bundle_uri;\n-- \ngitgitgadget\n\n"},{"id":"468909","messageId":"aafee168fbae2a1887f53febc4abd15522b12bc2.1670866407.git.gitgitgadget@gmail.com","threadId":"58937","inReplyTo":"pull.1443.git.1670866407.gitgitgadget@gmail.com","subject":"[PATCH 3/3] bundle-uri: remove GIT_TEST_BUNDLE_URI env variable","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-12T17:33:26Z","receivedAt":"2022-12-12T17:34:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe GIT_TEST_BUNDLE_URI environment variable is used in the t573* suite\nof tests that consume the bundle URIs advertised by the Git server. This\nvariable is equivalent to setting transfer.bundleURI=true, so we can do\nthat in these tests instead.\n\nThe environment variable has a name that implies it would be useful\noutside of these tests. It is not useful to set across all tests since\nit would do very little without some setup on the server side. Remove\nit.\n\nSigned-off-by: Derrick Stolee \n---\n t/lib-bundle-uri-protocol.sh | 12 ++++++++----\n t/t5601-clone.sh | 8 ++++----\n transport.c | 5 ++---\n 3 files changed, 14 insertions(+), 11 deletions(-)\n\ndiff --git a/t/lib-bundle-uri-protocol.sh b/t/lib-bundle-uri-protocol.sh\nindex 73e2d45bc8b..a4a1af8d029 100644\n--- a/t/lib-bundle-uri-protocol.sh\n+++ b/t/lib-bundle-uri-protocol.sh\n@@ -88,8 +88,8 @@ test_expect_success \"clone with $BUNDLE_URI_PROTOCOL:// using protocol v2: reque\n \ttest_when_finished \"rm -rf log* cloned*\" &&\n \n \tGIT_TRACE_PACKET=\"$PWD/log\" \\\n-\tGIT_TEST_BUNDLE_URI=0 \\\n \tgit \\\n+\t\t-c transfer.bundleURI=false \\\n \t\t-c protocol.version=2 \\\n \t\tclone \"$BUNDLE_URI_REPO_URI\" cloned \\\n \t\t>actual 2>err &&\n@@ -137,6 +137,13 @@ test_expect_success \"clone with $BUNDLE_URI_PROTOCOL:// using protocol v2: reque\n \t! grep \"> command=bundle-uri\" log3\n '\n \n+# The remaining tests will all assume transfer.bundleURI=true\n+#\n+# This test can be removed when transfer.bundleURI is enabled by default.\n+test_expect_success 'enable transfer.bundleURI for remaining tests' '\n+\tgit config --global transfer.bundleURI true\n+'\n+\n test_expect_success \"test bundle-uri with $BUNDLE_URI_PROTOCOL:// using protocol v2\" '\n \ttest_config -C \"$BUNDLE_URI_PARENT\" \\\n \t\tbundle.only.uri \"$BUNDLE_URI_BUNDLE_URI_ESCAPED\" &&\n@@ -150,7 +157,6 @@ test_expect_success \"test bundle-uri with $BUNDLE_URI_PROTOCOL:// using protocol\n \t\turi = $BUNDLE_URI_BUNDLE_URI_ESCAPED\n \tEOF\n \n-\tGIT_TEST_BUNDLE_URI=1 \\\n \ttest-tool bundle-uri \\\n \t\tls-remote \\\n \t\t\"$BUNDLE_URI_REPO_URI\" \\\n@@ -174,7 +180,6 @@ test_expect_success \"test bundle-uri with $BUNDLE_URI_PROTOCOL:// using protocol\n \t\turi = $BUNDLE_URI_BUNDLE_URI_ESCAPED\n \tEOF\n \n-\tGIT_TEST_BUNDLE_URI=1 \\\n \ttest-tool bundle-uri \\\n \t\tls-remote \\\n \t\t\"$BUNDLE_URI_REPO_URI\" \\\n@@ -203,7 +208,6 @@ test_expect_success \"test bundle-uri with $BUNDLE_URI_PROTOCOL:// using protocol\n \t\turi = $BUNDLE_URI_BUNDLE_URI_ESCAPED-3.bdl\n \tEOF\n \n-\tGIT_TEST_BUNDLE_URI=1 \\\n \ttest-tool bundle-uri \\\n \t\tls-remote \\\n \t\t\"$BUNDLE_URI_REPO_URI\" \\\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex a08a691ec4c..1928ea1dd7c 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -786,9 +786,9 @@ test_expect_success 'auto-discover bundle URI from HTTP clone' '\n \tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/repo2.git\" config \\\n \t\tbundle.everything.uri \"$HTTPD_URL/everything.bundle\" &&\n \n-\tGIT_TEST_BUNDLE_URI=1 \\\n \tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n-\t\tgit -c protocol.version=2 clone \\\n+\t\tgit -c protocol.version=2 \\\n+\t\t -c transfer.bundleURI=true clone \\\n \t\t$HTTPD_URL/smart/repo2.git repo2 &&\n \tcat >pattern <<-EOF &&\n \t\"event\":\"child_start\".*\"argv\":\\[\"git-remote-https\",\"$HTTPD_URL/everything.bundle\"\\]\n@@ -815,9 +815,9 @@ test_expect_success 'auto-discover multiple bundles from HTTP clone' '\n \tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/repo3.git\" config \\\n \t\tbundle.new.uri \"$HTTPD_URL/new.bundle\" &&\n \n-\tGIT_TEST_BUNDLE_URI=1 \\\n \tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n-\t\tgit -c protocol.version=2 clone \\\n+\t\tgit -c protocol.version=2 \\\n+\t\t -c transfer.bundleURI=true clone \\\n \t\t$HTTPD_URL/smart/repo3.git repo3 &&\n \n \t# We should fetch _both_ bundles\ndiff --git a/transport.c b/transport.c\nindex 957dca4923c..241f8a6ba2d 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1533,10 +1533,9 @@ int transport_get_remote_bundle_uri(struct transport *transport)\n \n \t/*\n \t * Don't request bundle-uri from the server unless configured to\n-\t * do so by GIT_TEST_BUNDLE_URI=1 or transfer.bundleURI=true.\n+\t * do so by the transfer.bundleURI=true config option.\n \t */\n-\tif (!git_env_bool(\"GIT_TEST_BUNDLE_URI\", 0) &&\n-\t (git_config_get_bool(\"transfer.bundleuri\", &value) || !value))\n+\tif (git_config_get_bool(\"transfer.bundleuri\", &value) || !value)\n \t\treturn 0;\n \n \tif (!transport->bundles->baseURI)\n-- \ngitgitgadget\n"},{"id":"468910","messageId":"d17f08ed4b68d711b452b5cfb54a949845bdea81.1670866407.git.gitgitgadget@gmail.com","threadId":"58937","inReplyTo":"pull.1443.git.1670866407.gitgitgadget@gmail.com","subject":"[PATCH 1/3] bundle-uri: drop unused 'uri' parameter","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-12T17:33:24Z","receivedAt":"2022-12-12T17:34:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe 'uri' parameter of fetch_bundle_list() was added early in\ndevelopment, but is not required since the 'list' parameter includes a\n'baseURI' member. Remove the 'uri' parameter and make this expectation\nexplicit.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/clone.c | 1 -\n bundle-uri.c | 2 +-\n bundle-uri.h | 8 ++++----\n 3 files changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 12fb527d7bb..430b2e981e3 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1284,7 +1284,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tif (repo_init(the_repository, git_dir, work_tree))\n \t\t\t\twarning(_(\"failed to initialize the repo, skipping bundle URI\"));\n \t\t\telse if (fetch_bundle_list(the_repository,\n-\t\t\t\t\t\t remote->url[0],\n \t\t\t\t\t\t transport->bundles))\n \t\t\t\twarning(_(\"failed to fetch advertised bundles\"));\n \t\t} else {\ndiff --git a/bundle-uri.c b/bundle-uri.c\nindex c411b871bdd..8efb4e7acad 100644\n--- a/bundle-uri.c\n+++ b/bundle-uri.c\n@@ -577,7 +577,7 @@ cleanup:\n \treturn result;\n }\n \n-int fetch_bundle_list(struct repository *r, const char *uri, struct bundle_list *list)\n+int fetch_bundle_list(struct repository *r, struct bundle_list *list)\n {\n \tint result;\n \tstruct bundle_list global_list;\ndiff --git a/bundle-uri.h b/bundle-uri.h\nindex b2c9c160a52..29b0c98ee65 100644\n--- a/bundle-uri.h\n+++ b/bundle-uri.h\n@@ -68,8 +68,8 @@ struct bundle_list {\n \t * In the case of the 'bundle-uri' protocol v2 command, the base\n \t * URI is the URI of the Git remote.\n \t *\n-\t * Otherewise, the bundle list was downloaded over HTTP from some\n-\t * known URI.\n+\t * Otherwise, the bundle list was downloaded over HTTP from some\n+\t * known URI. 'baseURI' is set to that value.\n \t *\n \t * The baseURI is used as the base for any relative URIs\n \t * advertised by the bundle list at that location.\n@@ -112,10 +112,10 @@ int fetch_bundle_uri(struct repository *r, const char *uri);\n * bundle-uri protocol v2 verb) at the given uri, fetch and unbundle the\n * bundles according to the bundle strategy of that list.\n *\n- * Returns non-zero if no bundle information is found at the given 'uri'.\n+ * It is expected that the given 'list' is initialized, including its\n+ * 'baseURI' value.\n */\n int fetch_bundle_list(struct repository *r,\n-\t\t const char *uri,\n \t\t struct bundle_list *list);\n \n /**\n-- \ngitgitgadget\n\n"},{"id":"468911","messageId":"pull.1443.git.1670866407.gitgitgadget@gmail.com","threadId":"58937","inReplyTo":null,"subject":"[PATCH 0/3] Bundle URIs 4.5: fixups from part IV","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-12T17:33:23Z","receivedAt":"2022-12-12T17:34:15Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"These patches represent small fixups that came in review from the last\nversion of 'ds/bundle-uri-4'. Since it was merged to 'next', these patches\nare forward-fixes on that branch.\n\nNote: I did not include any changes that could be solved by adding an UNUSED\nmacro, saving that for Peff and his already-prepared patches in that area.\n\nThanks, -Stolee\n\nDerrick Stolee (3):\n bundle-uri: drop unused 'uri' parameter\n bundle-uri: advertise based on repo config\n bundle-uri: remove GIT_TEST_BUNDLE_URI env variable\n\n builtin/clone.c | 1 -\n bundle-uri.c | 4 ++--\n bundle-uri.h | 8 ++++----\n t/lib-bundle-uri-protocol.sh | 12 ++++++++----\n t/t5601-clone.sh | 8 ++++----\n transport.c | 5 ++---\n 6 files changed, 20 insertions(+), 18 deletions(-)\n\n\nbase-commit: cec58f9965c845be74753aac6f49b4f177faa064\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1443%2Fderrickstolee%2Fbundle-redo%2Fadvertise-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1443/derrickstolee/bundle-redo/advertise-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1443\n-- \ngitgitgadget\n"},{"id":"468912","messageId":"543a72cd-a488-d535-f6fe-4b70c5f8d333@github.com","threadId":"58937","inReplyTo":"pull.1443.git.1670866407.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Bundle URIs 4.5: fixups from part IV","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-12-12T18:07:26Z","receivedAt":"2022-12-12T18:08:33Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Derrick Stolee via GitGitGadget wrote:\n> These patches represent small fixups that came in review from the last\n> version of 'ds/bundle-uri-4'. Since it was merged to 'next', these patches\n> are forward-fixes on that branch.\n> \n> Note: I did not include any changes that could be solved by adding an UNUSED\n> macro, saving that for Peff and his already-prepared patches in that area.\n> \n> Thanks, -Stolee\n> \n> Derrick Stolee (3):\n> bundle-uri: drop unused 'uri' parameter\n> bundle-uri: advertise based on repo config\n> bundle-uri: remove GIT_TEST_BUNDLE_URI env variable\n\nThe first two patches (unused arg removal & using repo to get config) are\nstraightforward fixes for issues mentioned earlier ([1] and [2],\nrespectively). The last patch replaces the 'GIT_TEST_BUNDLE_URI' environment\nvariable with globally setting 'transfer.bundleURI' for a subset of the\n'lib-bundle-uri-protocol.sh' tests. The comment you added in that file (\"The\nremaining tests will all assume transfer.bundleURI=true\") clearly explains\nwhat you're doing there as a reference for future updates to the tests.\n\nThese patches all look good to me. Thanks!\n\n[1] https://lore.kernel.org/git/affbc458-d4f5-525f-d431-5ec1d489afc8@github.com/\n[2] https://lore.kernel.org/git/4d4e02c3-89dc-8372-7e8a-7ec76fdd6f4e@github.com/\n"},{"id":"468928","messageId":"Y5eWJ03XQTqkJHjK@coredump.intra.peff.net","threadId":"58937","inReplyTo":"pull.1443.git.1670866407.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Bundle URIs 4.5: fixups from part IV","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-12T20:59:19Z","receivedAt":"2022-12-12T21:02:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 12, 2022 at 05:33:23PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> These patches represent small fixups that came in review from the last\n> version of 'ds/bundle-uri-4'. Since it was merged to 'next', these patches\n> are forward-fixes on that branch.\n> \n> Note: I did not include any changes that could be solved by adding an UNUSED\n> macro, saving that for Peff and his already-prepared patches in that area.\n\nThanks, that sounds good on my part. The first two look obviously\ncorrect to me. The third looks sensible, too, but isn't an area I'd\nlooked at before.\n\n-Peff\n"},{"id":"469280","messageId":"221219.86tu1rzmbs.gmgdl@evledraar.gmail.com","threadId":"58937","inReplyTo":"d17f08ed4b68d711b452b5cfb54a949845bdea81.1670866407.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] bundle-uri: drop unused 'uri' parameter","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-19T10:57:17Z","receivedAt":"2022-12-19T11:04:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 12 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n>\n> The 'uri' parameter of fetch_bundle_list() was added early in\n> development, but is not required since the 'list' parameter includes a\n> 'baseURI' member. Remove the 'uri' parameter and make this expectation\n> explicit.\n\nMakes sense, and this is a straightforward commit, but...\n\n> diff --git a/bundle-uri.h b/bundle-uri.h\n> index b2c9c160a52..29b0c98ee65 100644\n> --- a/bundle-uri.h\n> +++ b/bundle-uri.h\n> @@ -68,8 +68,8 @@ struct bundle_list {\n> \t * In the case of the 'bundle-uri' protocol v2 command, the base\n> \t * URI is the URI of the Git remote.\n> \t *\n> -\t * Otherewise, the bundle list was downloaded over HTTP from some\n> -\t * known URI.\n> +\t * Otherwise, the bundle list was downloaded over HTTP from some\n> +\t * known URI. 'baseURI' is set to that value.\n\n...here we have a stray typo fix, not called out in the commit\nmessage. I think that can pass, but if you're re-rolling I think any\nsuch \"while-at-it\" would be better split into their own commits.\n\nBut more importantly:\n\n> \t *\n> \t * The baseURI is used as the base for any relative URIs\n> \t * advertised by the bundle list at that location.\n> @@ -112,10 +112,10 @@ int fetch_bundle_uri(struct repository *r, const char *uri);\n> * bundle-uri protocol v2 verb) at the given uri, fetch and unbundle the\n> * bundles according to the bundle strategy of that list.\n> *\n> - * Returns non-zero if no bundle information is found at the given 'uri'.\n> + * It is expected that the given 'list' is initialized, including its\n> + * 'baseURI' value\n\nAt first sight this seems like a regression. Why have we stopped\ndocumenting the exit code?\n\nBut looking again is this because in 7cc47a980ac (bundle-uri: download\nbundles from an advertised list, 2022-12-05) you accidentally\ncopy/pasted the documentation for fetch_bundle_uri(), and this was never\napplicable to this function?\n\nEven then, not documenting the code we return now is a regression. If it\nwas wrong before, let's make it correct, not stop discussing it\nentirely.\n\nIn either case this is another while-at-it entirely unrelated to the\n$subject here of dropping an unused parameter.\n\nThe same goes for the added docs, that we \"expect [that] 'list' is\ninitialized\" may be true, but that would have been true before we\nremoved this unused parameter, so let's not stick that in this unrelated\n\"UNUSED\" change.\n\n> */\n> int fetch_bundle_list(struct repository *r,\n> -\t\t const char *uri,\n> \t\t struct bundle_list *list);\n> \n> /**\n\n"},{"id":"469281","messageId":"221219.86pmcfzm7g.gmgdl@evledraar.gmail.com","threadId":"58937","inReplyTo":"857d1abec4cf124e011c7f84276ce105cb5b3a96.1670866407.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] bundle-uri: advertise based on repo config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-19T11:04:48Z","receivedAt":"2022-12-19T11:07:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 12 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n>\n> The bundle_uri_advertise() method was not using its repository\n> parameter, but this is a mistake. Use repo_config_get_maybe_bool()\n> instead of git_config_maybe_bool().\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> bundle-uri.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/bundle-uri.c b/bundle-uri.c\n> index 8efb4e7acad..5f158cc52e1 100644\n> --- a/bundle-uri.c\n> +++ b/bundle-uri.c\n> @@ -610,7 +610,7 @@ int bundle_uri_advertise(struct repository *r, struct strbuf *value)\n> \t\tgoto cached;\n> \n> \tadvertise_bundle_uri = 0;\n> -\tgit_config_get_maybe_bool(\"uploadpack.advertisebundleuris\", &advertise_bundle_uri);\n> +\trepo_config_get_maybe_bool(r, \"uploadpack.advertisebundleuris\", &advertise_bundle_uri);\n> \n> cached:\n> \treturn advertise_bundle_uri;\n\nThis looks good, but as with another parallel topic of yours that I\ncommented on[1] leaves us wondering if this had any effect.\n\nI.e. is this just for good measure because we have a \"r\" parameter, or\ndid we do the wrong thing for submodules before this change? In that\ncase let's add the missing test coverage.\n\nOr, if it's the former let's update the commit message here, saying e.g.:\n\n\tWhile we should use \"r\" for , we\n\talready did the right thing for submodules, as \"the_repository\"\n\twould be set to the submodule because .\n\n1. https://lore.kernel.org/git/221215.865yec3b1j.gmgdl@evledraar.gmail.com/\n"},{"id":"469282","messageId":"221219.86len3zlqj.gmgdl@evledraar.gmail.com","threadId":"58937","inReplyTo":"aafee168fbae2a1887f53febc4abd15522b12bc2.1670866407.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] bundle-uri: remove GIT_TEST_BUNDLE_URI env variable","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-19T11:09:14Z","receivedAt":"2022-12-19T11:17:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 12 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n>\n> The GIT_TEST_BUNDLE_URI environment variable is used in the t573* suite\n> of tests that consume the bundle URIs advertised by the Git server. This\n> variable is equivalent to setting transfer.bundleURI=true, so we can do\n> that in these tests instead.\n\nI think this is probably OK. I can't remember why I added both the env\nvariable and the setting in what became 0ef961dda05 (bundle-uri client:\nadd boolean transfer.bundleURI setting, 2022-12-05).\n\nBut I think this commit message really doesn't explain why it's OK to\nremove it. In general we do have GIT_TEST_* settings that duplicate\nconfig, e.g. GIT_TEST_PROTOCOL_VERSION.\n\nWe do so because we'd like the environment variable to override the\nsetting, or the other way around (I think depending on the GIT_TEST_*\nvariable it's either-or, it's a mess).\n\nBut in this case we'd never like to combine the two?\n\n> The environment variable has a name that implies it would be useful\n> outside of these tests. It is not useful to set across all tests since\n> it would do very little without some setup on the server side. Remove\n> it.\n\nThis part I really don't get, why does its name imply that? We use\nGIT_TEST_* variables for this sort of thing, maybe it's not needed here,\nbut what should it have been named? GIT_TEST_T57XX_* or something?\n\nI named it like that for consistency with existing test variables, ...\n"},{"id":"469334","messageId":"xmqq4jtq292e.fsf@gitster.g","threadId":"58937","inReplyTo":"221219.86tu1rzmbs.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/3] bundle-uri: drop unused 'uri' parameter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-20T00:49:29Z","receivedAt":"2022-12-20T00:49:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> ...here we have a stray typo fix, not called out in the commit\n> message. I think that can pass, but if you're re-rolling I think any\n> such \"while-at-it\" would be better split into their own commits.\n>\n> But more importantly:\n> ...\n> But looking again is this because in 7cc47a980ac (bundle-uri: download\n> bundles from an advertised list, 2022-12-05) you accidentally\n> copy/pasted the documentation for fetch_bundle_uri(), and this was never\n> applicable to this function?\n> ...\n\nI think these three patches were designed to be \"oops, that was\nwrong and here is a band-aid\" follow-up fixes on top of what was\nback then in 'next'. Now the base topic has been kicked out of\n'next' together with these, we can afford to roll them into the base\nseries before merging it back to 'next', but due to things generally\nbeing slow toward the end of the year, that hasn't happened yet.\n\nThanks for reviewing these carefully.\n"},{"id":"469379","messageId":"de1aca07-dd03-3f46-9f56-1cc7616b573a@github.com","threadId":"58937","inReplyTo":"221219.86len3zlqj.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 3/3] bundle-uri: remove GIT_TEST_BUNDLE_URI env variable","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-12-20T13:51:08Z","receivedAt":"2022-12-20T13:51:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/19/22 6:09 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Dec 12 2022, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee \n>>\n>> The GIT_TEST_BUNDLE_URI environment variable is used in the t573* suite\n>> of tests that consume the bundle URIs advertised by the Git server. This\n>> variable is equivalent to setting transfer.bundleURI=true, so we can do\n>> that in these tests instead.\n> \n> I think this is probably OK. I can't remember why I added both the env\n> variable and the setting in what became 0ef961dda05 (bundle-uri client:\n> add boolean transfer.bundleURI setting, 2022-12-05).\n> \n> But I think this commit message really doesn't explain why it's OK to\n> remove it. In general we do have GIT_TEST_* settings that duplicate\n> config, e.g. GIT_TEST_PROTOCOL_VERSION.\n> \n> We do so because we'd like the environment variable to override the\n> setting, or the other way around (I think depending on the GIT_TEST_*\n> variable it's either-or, it's a mess).\n\nIf the variable is named GIT_TEST_* then it should be intended for\nuse within tests. However, it provides _no value_ over the existing\nconfig option, so the tests are updated to use the config value\ninstead.\n\nAs mentioned, the one exception is where we don't want to uddate\nevery test to use the config variable and instead want to set the\nGIT_TEST_* variable across all tests and see how it interacts with\nother tests. However, _as mentioned in the commit message_ this\nvariable would not have any effect in other tests because the\nadvertisement depends on other config options on the server side.\n\nThanks,\n-Stolee\n"},{"id":"469380","messageId":"49eb61a4-2b5d-c695-25e1-621bac6f3421@github.com","threadId":"58937","inReplyTo":"221219.86pmcfzm7g.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/3] bundle-uri: advertise based on repo config","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-12-20T13:54:39Z","receivedAt":"2022-12-20T13:54:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/19/22 6:04 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Dec 12 2022, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee \n>>\n>> The bundle_uri_advertise() method was not using its repository\n>> parameter, but this is a mistake. Use repo_config_get_maybe_bool()\n>> instead of git_config_maybe_bool().\n>>\n>> Signed-off-by: Derrick Stolee \n>> ---\n>> bundle-uri.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/bundle-uri.c b/bundle-uri.c\n>> index 8efb4e7acad..5f158cc52e1 100644\n>> --- a/bundle-uri.c\n>> +++ b/bundle-uri.c\n>> @@ -610,7 +610,7 @@ int bundle_uri_advertise(struct repository *r, struct strbuf *value)\n>> \t\tgoto cached;\n>> \n>> \tadvertise_bundle_uri = 0;\n>> -\tgit_config_get_maybe_bool(\"uploadpack.advertisebundleuris\", &advertise_bundle_uri);\n>> +\trepo_config_get_maybe_bool(r, \"uploadpack.advertisebundleuris\", &advertise_bundle_uri);\n>> \n>> cached:\n>> \treturn advertise_bundle_uri;\n> \n> This looks good, but as with another parallel topic of yours that I\n> commented on[1] leaves us wondering if this had any effect.\n> \n> I.e. is this just for good measure because we have a \"r\" parameter, or\n> did we do the wrong thing for submodules before this change? In that\n> case let's add the missing test coverage.\n> \n> Or, if it's the former let's update the commit message here, saying e.g.:\n> \n> \tWhile we should use \"r\" for , we\n> \talready did the right thing for submodules, as \"the_repository\"\n> \twould be set to the submodule because \tknow about...>.\n\nAs I mentioned in another thread, we should always be using a\nlocal 'struct repository *' if we can so we aren't contributing\nto that particular dimension of technical debt, but submodules\nhave a lot of work ahead before these kinds of things can be\ntested individually.\n\nThis is especially the case when working with server code, where\nwe would never be in the submodule, but we don't want to block\nfuture removals of the non-repo versions of these methods.\n\nThanks,\n-Stolee\n"},{"id":"469381","messageId":"75107325-bd28-1adf-b260-ba8846c63229@github.com","threadId":"58937","inReplyTo":"221219.86tu1rzmbs.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/3] bundle-uri: drop unused 'uri' parameter","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-12-20T13:57:07Z","receivedAt":"2022-12-20T13:57:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/19/22 5:57 AM, Ævar Arnfjörð Bjarmason wrote:\n> On Mon, Dec 12 2022, Derrick Stolee via GitGitGadget wrote:\n\n>> @@ -112,10 +112,10 @@ int fetch_bundle_uri(struct repository *r, const char *uri);\n>> * bundle-uri protocol v2 verb) at the given uri, fetch and unbundle the\n>> * bundles according to the bundle strategy of that list.\n>> *\n>> - * Returns non-zero if no bundle information is found at the given 'uri'.\n>> + * It is expected that the given 'list' is initialized, including its\n>> + * 'baseURI' value\n\n> The same goes for the added docs, that we \"expect [that] 'list' is\n> initialized\" may be true, but that would have been true before we\n> removed this unused parameter, so let's not stick that in this unrelated\n> \"UNUSED\" change.\n\nIt is _not_ unrelated. The 'uri' parameter looks like it should\nbe used to determine relative URLs for the included list. However,\nthis reasoning around the 'baseURI' value points out that we are\nusing that value _instead_ of the 'uri' value, which is why it\nis safe to remove the parameter.\n\nThanks,\n-Stolee\n"},{"id":"469382","messageId":"be44ef00-2d80-18d5-4dab-357faca20853@github.com","threadId":"58937","inReplyTo":"xmqq4jtq292e.fsf@gitster.g","subject":"Re: [PATCH 1/3] bundle-uri: drop unused 'uri' parameter","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-12-20T14:02:13Z","receivedAt":"2022-12-20T14:02:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/19/22 7:49 PM, Junio C Hamano wrote:\n\n> I think these three patches were designed to be \"oops, that was\n> wrong and here is a band-aid\" follow-up fixes on top of what was\n> back then in 'next'. Now the base topic has been kicked out of\n> 'next' together with these, we can afford to roll them into the base\n> series before merging it back to 'next', but due to things generally\n> being slow toward the end of the year, that hasn't happened yet.\n\nI wasn't expecting to re-roll the base topic, but I'll get\nstarted on that now.\n\nHowever, the comments in this review are barely actionable. They\nprovide very little value especially for how verbose they are. I'm\nfrustrated to see such a drive-by review so late in the process.\n\nThanks,\n-Stolee\n"},{"id":"469397","messageId":"221220.86o7rxx0so.gmgdl@evledraar.gmail.com","threadId":"58937","inReplyTo":"de1aca07-dd03-3f46-9f56-1cc7616b573a@github.com","subject":"Re: [PATCH 3/3] bundle-uri: remove GIT_TEST_BUNDLE_URI env variable","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-20T20:41:05Z","receivedAt":"2022-12-20T20:44:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 20 2022, Derrick Stolee wrote:\n\n> On 12/19/22 6:09 AM, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Mon, Dec 12 2022, Derrick Stolee via GitGitGadget wrote:\n>> \n>>> From: Derrick Stolee \n>>>\n>>> The GIT_TEST_BUNDLE_URI environment variable is used in the t573* suite\n>>> of tests that consume the bundle URIs advertised by the Git server. This\n>>> variable is equivalent to setting transfer.bundleURI=true, so we can do\n>>> that in these tests instead.\n>> \n>> I think this is probably OK. I can't remember why I added both the env\n>> variable and the setting in what became 0ef961dda05 (bundle-uri client:\n>> add boolean transfer.bundleURI setting, 2022-12-05).\n>> \n>> But I think this commit message really doesn't explain why it's OK to\n>> remove it. In general we do have GIT_TEST_* settings that duplicate\n>> config, e.g. GIT_TEST_PROTOCOL_VERSION.\n>> \n>> We do so because we'd like the environment variable to override the\n>> setting, or the other way around (I think depending on the GIT_TEST_*\n>> variable it's either-or, it's a mess).\n>\n> If the variable is named GIT_TEST_* then it should be intended for\n> use within tests. However, it provides _no value_ over the existing\n> config option, so the tests are updated to use the config value\n> instead.\n>\n> As mentioned, the one exception is where we don't want to uddate\n> every test to use the config variable and instead want to set the\n> GIT_TEST_* variable across all tests and see how it interacts with\n> other tests. However, _as mentioned in the commit message_ this\n> variable would not have any effect in other tests because the\n> advertisement depends on other config options on the server side.\n\nMakes sense, I think I had a bit of a mental block on it in my initial\nlook at it. Re-reading your commit message it does make sense.\n\nI think it makes sense to squash this in:\n\t\n\tdiff --git a/transport.c b/transport.c\n\tindex 77a61a9d7bb..efc8fa43183 100644\n\t--- a/transport.c\n\t+++ b/transport.c\n\t@@ -1523,7 +1523,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)\n\t \n\t int transport_get_remote_bundle_uri(struct transport *transport)\n\t {\n\t-\tint value = 0;\n\t+\tint value;\n\t \tconst struct transport_vtable *vtable = transport->vtable;\n\t \n\t \t/* Check config only once. */\n\nThe reason we init'd the variable was because we were looking for this\neither in the environment or the config, so we needed to have a fallback\nin case we used the env var.\n\nBut with the logic after your fix-up we know that if we end up in the\nRHS of the \"||\" that \"value\" was initialized, as we returned 0 from the\nconfig API call.\n\n"},{"id":"469398","messageId":"221220.86k02lx0l7.gmgdl@evledraar.gmail.com","threadId":"58937","inReplyTo":"75107325-bd28-1adf-b260-ba8846c63229@github.com","subject":"Re: [PATCH 1/3] bundle-uri: drop unused 'uri' parameter","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-20T20:46:21Z","receivedAt":"2022-12-20T20:49:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 20 2022, Derrick Stolee wrote:\n\n> On 12/19/22 5:57 AM, Ævar Arnfjörð Bjarmason wrote:\n>> On Mon, Dec 12 2022, Derrick Stolee via GitGitGadget wrote:\n>\n>>> @@ -112,10 +112,10 @@ int fetch_bundle_uri(struct repository *r, const char *uri);\n>>> * bundle-uri protocol v2 verb) at the given uri, fetch and unbundle the\n>>> * bundles according to the bundle strategy of that list.\n>>> *\n>>> - * Returns non-zero if no bundle information is found at the given 'uri'.\n>>> + * It is expected that the given 'list' is initialized, including its\n>>> + * 'baseURI' value\n>\n>> The same goes for the added docs, that we \"expect [that] 'list' is\n>> initialized\" may be true, but that would have been true before we\n>> removed this unused parameter, so let's not stick that in this unrelated\n>> \"UNUSED\" change.\n>\n> It is _not_ unrelated. The 'uri' parameter looks like it should\n> be used to determine relative URLs for the included list. However,\n> this reasoning around the 'baseURI' value points out that we are\n> using that value _instead_ of the 'uri' value, which is why it\n> is safe to remove the parameter.\n\nI'm saying that the function expected that baseURI to be initialized\nbefore, but with your rationale I think it's fine to also do it in one\ncommit.\n\nI also pointed out (and you elided) that we should still document the\nreturn code, do you agree that we should keep that?\n"},{"id":"469399","messageId":"221220.86fsd9wzpb.gmgdl@evledraar.gmail.com","threadId":"58937","inReplyTo":"be44ef00-2d80-18d5-4dab-357faca20853@github.com","subject":"Re: [PATCH 1/3] bundle-uri: drop unused 'uri' parameter","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-20T20:50:29Z","receivedAt":"2022-12-20T21:08:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 20 2022, Derrick Stolee wrote:\n\n> On 12/19/22 7:49 PM, Junio C Hamano wrote:\n>\n>> I think these three patches were designed to be \"oops, that was\n>> wrong and here is a band-aid\" follow-up fixes on top of what was\n>> back then in 'next'. Now the base topic has been kicked out of\n>> 'next' together with these, we can afford to roll them into the base\n>> series before merging it back to 'next', but due to things generally\n>> being slow toward the end of the year, that hasn't happened yet.\n>\n> I wasn't expecting to re-roll the base topic, but I'll get\n> started on that now.\n>\n> However, the comments in this review are barely actionable.\n\nSkimming over my comments again the actionable bits are:\n\n * 1/3: Commit says it's removing an unused param, also has while-at-it\n typo fix, maybe split that up?\n * 1/3: Commit says , has while-at-it removal of documenting what\n a function's return value means, maybe keep that?\n * 1/3: Commit says , has seeming while-at-it discussion of what\n another parameter is expected to contain (you replied saying they\n were related)\n * 2/3: Question about whether reading \"r\" v.s. \"the_repository\" has an\n observable behavior change. If yes let's add a test, if no let's note\n \"it's for good measure\".\n * 3/3: A question about whether we really didn't need\n GIT_TEST_BUNDLE_URI. You managed to convince me that no, we don't\n * 3/3: Question about what the 2nd paragraph of the commit message is\n trying to convey (you didn't reply to this bit). The actionable thing\n would be to clarify it, or remove it.\n\n> They provide very little value especially for how verbose they are.\n\nI agree that this was all of relatively little value, these are all\nrather trivial patches after all, and the bundle-uri feature is opt-in\nand WIP at this point.\n\nBut even trivial patches can be helped along by review. I'm just trying\nto help this topic land & show Junio that others have reviewed this\ncarefully.\n\nI agree that my E-Mails are verbose, sorry. This isn't my native\nlanguage, it's a balancing act between trying to be unambiguously\nunderstood, and verbosity. sorry.\n\n> frustrated to see such a drive-by review so late in the process.\n\nThis I'm confused by. You submitted this ~1 week ago on the 12th, Junio\nrewound the parent topic out of \"next\" on the 14th.\n\nIsn't this the appropriate time to comment on both this topic & its\nparent?\n"}]}