| Message ID | e46118e60f7a59ef25edf5f1378b4ef0c007cce8.1670262639.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 0ef961dda05dd5a9cdf7eac0dc72816b9950eaac |
| Headers | show |
| Series | Bundle URIs IV: advertise over protocol v2 | expand |
Ævar Arnfjörð Bjarmason via GitGitGadget wrote: > From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= > <avarab@gmail.com> > > The yet-to-be introduced client support for bundle-uri will always > fall back on a full clone, but we'd still like to be able to ignore a > server's bundle-uri advertisement entirely. > > The new transfer.bundleURI config option defaults to 'false', but a user > can set it to 'true' to enable checking for bundle URIs from the origin > Git server using protocol v2. > > To enable this setting by default in the correct tests, add a > GIT_TEST_BUNDLE_URI environment variable. It wasn't immediately clear to me from reading this patch, but it looks like 'GIT_TEST_BUNDLE_URI' is mainly used to allow 'test-tool bundle-uri ls-remote' to issue the bundle URI command (since it can't use a '-c transfer.bundleURI=true' command line option) in patch 7 [1]. If that's the only use for 'GIT_TEST_BUNDLE_URI', could you avoid the environment variable altogether by setting 'transfer.bundleURI=true' with 'test_config' before the 'test-tool' call (and 'test_unconfig' after, if needed)? Alternatively, if you do want to be able to test the bundle URI protocol wholesale across all tests (e.g., in the 'linux-TEST-vars' CI job), then I think the environment variable makes sense. [1] https://lore.kernel.org/git/acc5a8f57f903342c47802115f8e3de9e9d588dc.1670262639.git.gitgitgadget@gmail.com/ > diff --git a/t/lib-bundle-uri-protocol.sh b/t/lib-bundle-uri-protocol.sh > index d44c6e10f9e..77bfd4f0119 100644 > --- a/t/lib-bundle-uri-protocol.sh > +++ b/t/lib-bundle-uri-protocol.sh > @@ -85,9 +85,10 @@ test_expect_success "connect with $BUNDLE_URI_PROTOCOL:// using protocol v2: hav > ' > > test_expect_success "clone with $BUNDLE_URI_PROTOCOL:// using protocol v2: request bundle-uris" ' > - test_when_finished "rm -rf log cloned" && > + test_when_finished "rm -rf log cloned cloned2" && > > GIT_TRACE_PACKET="$PWD/log" \ > + GIT_TEST_BUNDLE_URI=0 \ > git \ > -c protocol.version=2 \ > clone "$BUNDLE_URI_REPO_URI" cloned \ > @@ -99,6 +100,22 @@ test_expect_success "clone with $BUNDLE_URI_PROTOCOL:// using protocol v2: reque > # Server advertised bundle-uri capability > grep "< bundle-uri" log && > > + # Client did not issue bundle-uri command > + ! grep "> command=bundle-uri" log && > + > + GIT_TRACE_PACKET="$PWD/log" \ > + git \ > + -c transfer.bundleURI=true \ > + -c protocol.version=2 \ > + clone "$BUNDLE_URI_REPO_URI" cloned2 \ > + >actual 2>err && If 'GIT_TEST_BUNDLE_URI' is set to '1' in a more global scope (by a CI job or user running the tests), then the '-c transfer.bundleURI' config isn't actually what's enabling the behavior. To make this more directly comparable to the case earlier in this test, could you add 'GIT_TEST_BUNDLE_URI=0' here as well?
On 12/5/2022 6:32 PM, Victoria Dye wrote: > Ævar Arnfjörð Bjarmason via GitGitGadget wrote: >> + # Client did not issue bundle-uri command >> + ! grep "> command=bundle-uri" log && >> + >> + GIT_TRACE_PACKET="$PWD/log" \ >> + git \ >> + -c transfer.bundleURI=true \ >> + -c protocol.version=2 \ >> + clone "$BUNDLE_URI_REPO_URI" cloned2 \ >> + >actual 2>err && > > If 'GIT_TEST_BUNDLE_URI' is set to '1' in a more global scope (by a CI job > or user running the tests), then the '-c transfer.bundleURI' config isn't > actually what's enabling the behavior. To make this more directly comparable > to the case earlier in this test, could you add 'GIT_TEST_BUNDLE_URI=0' here > as well? You're right that GIT_TEST_BUNDLE_URI is not needed if we can set transfer.bundleURI globally earlier in the test. It doesn't make much sense to run the entire test suite with it on, since the server side does not advertise bundles unless explicitly configured to do so. Thanks, -Stolee
diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt index 264812cca4d..c3ac767d1e4 100644 --- a/Documentation/config/transfer.txt +++ b/Documentation/config/transfer.txt @@ -115,3 +115,9 @@ transfer.unpackLimit:: transfer.advertiseSID:: Boolean. When true, client and server processes will advertise their unique session IDs to their remote counterpart. Defaults to false. + +transfer.bundleURI:: + When `true`, local `git clone` commands will request bundle + information from the remote server (if advertised) and download + bundles before continuing the clone through the Git protocol. + Defaults to `false`. diff --git a/t/lib-bundle-uri-protocol.sh b/t/lib-bundle-uri-protocol.sh index d44c6e10f9e..77bfd4f0119 100644 --- a/t/lib-bundle-uri-protocol.sh +++ b/t/lib-bundle-uri-protocol.sh @@ -85,9 +85,10 @@ test_expect_success "connect with $BUNDLE_URI_PROTOCOL:// using protocol v2: hav ' test_expect_success "clone with $BUNDLE_URI_PROTOCOL:// using protocol v2: request bundle-uris" ' - test_when_finished "rm -rf log cloned" && + test_when_finished "rm -rf log cloned cloned2" && GIT_TRACE_PACKET="$PWD/log" \ + GIT_TEST_BUNDLE_URI=0 \ git \ -c protocol.version=2 \ clone "$BUNDLE_URI_REPO_URI" cloned \ @@ -99,6 +100,22 @@ test_expect_success "clone with $BUNDLE_URI_PROTOCOL:// using protocol v2: reque # Server advertised bundle-uri capability grep "< bundle-uri" log && + # Client did not issue bundle-uri command + ! grep "> command=bundle-uri" log && + + GIT_TRACE_PACKET="$PWD/log" \ + git \ + -c transfer.bundleURI=true \ + -c protocol.version=2 \ + clone "$BUNDLE_URI_REPO_URI" cloned2 \ + >actual 2>err && + + # Server responded using protocol v2 + grep "< version 2" log && + + # Server advertised bundle-uri capability + grep "< bundle-uri" log && + # Client issued bundle-uri command grep "> command=bundle-uri" log ' diff --git a/transport.c b/transport.c index b6f279e92cb..9f9e38d66dd 100644 --- a/transport.c +++ b/transport.c @@ -1516,6 +1516,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs) int transport_get_remote_bundle_uri(struct transport *transport) { + int value = 0; const struct transport_vtable *vtable = transport->vtable; /* Check config only once. */ @@ -1523,6 +1524,14 @@ int transport_get_remote_bundle_uri(struct transport *transport) return 0; transport->got_remote_bundle_uri = 1; + /* + * Don't request bundle-uri from the server unless configured to + * do so by GIT_TEST_BUNDLE_URI=1 or transfer.bundleURI=true. + */ + if (!git_env_bool("GIT_TEST_BUNDLE_URI", 0) && + (git_config_get_bool("transfer.bundleuri", &value) || !value)) + return 0; + if (!vtable->get_bundle_uri) return error(_("bundle-uri operation not supported by protocol"));