| Message ID | 933974689312bbb130236c81550ee3467f295a43.1668628303.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| 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. Thanks for adding this, an "opt-in" approach seems reasonable for introducing this feature. > > To enable this setting by default in the correct tests, add a > GIT_TEST_BUNDLE_URI environment variable. This makes sense. I'm less concerned with this environment variable than those in patch 2 [1], since it's in line with the test variables that enable/disable whole features ('GIT_TEST_SPLIT_INDEX', 'GIT_TEST_COMMIT_GRAPH', etc.). The only thing feedback I can think of would be that this patch could be moved to earlier in the series (that is, immediately after creating 'transport_get_remote_bundle_uri()'). That said, I don't feel strongly either way. [1] https://lore.kernel.org/git/ca410bed-e8d1-415f-5235-b64fe18bed27@github.com/ > > Co-authored-by: Derrick Stolee <derrickstolee@github.com> > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > Signed-off-by: Derrick Stolee <derrickstolee@github.com> The implementation and documentation below align with the commit message. Looks good! > --- > Documentation/config/transfer.txt | 6 ++++++ > t/lib-t5730-protocol-v2-bundle-uri.sh | 3 +++ > transport.c | 10 +++++++--- > 3 files changed, 16 insertions(+), 3 deletions(-) > > 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-t5730-protocol-v2-bundle-uri.sh b/t/lib-t5730-protocol-v2-bundle-uri.sh > index 000fcc5e20b..872bc39ad1b 100644 > --- a/t/lib-t5730-protocol-v2-bundle-uri.sh > +++ b/t/lib-t5730-protocol-v2-bundle-uri.sh > @@ -1,5 +1,8 @@ > # Included from t573*-protocol-v2-bundle-uri-*.sh > > +GIT_TEST_BUNDLE_URI=1 > +export GIT_TEST_BUNDLE_URI > + > T5730_PARENT= > T5730_URI= > T5730_BUNDLE_URI= > diff --git a/transport.c b/transport.c > index 86460f5be28..b33180226ae 100644 > --- a/transport.c > +++ b/transport.c > @@ -1538,6 +1538,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs) > > int transport_get_remote_bundle_uri(struct transport *transport, int quiet) > { > + int value = 0; > const struct transport_vtable *vtable = transport->vtable; > > /* Check config only once. */ > @@ -1545,10 +1546,13 @@ int transport_get_remote_bundle_uri(struct transport *transport, int quiet) > return 0; > > /* > - * This is intentionally below the transport.injectBundleURI, > - * we want to be able to inject into protocol v0, or into the > - * dialog of a server who doesn't support this. > + * Don't use bundle-uri at all, if configured not to. Only proceed > + * if 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) { > if (quiet) > return -1;
On 11/28/2022 8:03 PM, Victoria Dye wrote: > Æ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. > > Thanks for adding this, an "opt-in" approach seems reasonable for > introducing this feature. > >> >> To enable this setting by default in the correct tests, add a >> GIT_TEST_BUNDLE_URI environment variable. > > This makes sense. I'm less concerned with this environment variable than > those in patch 2 [1], since it's in line with the test variables that > enable/disable whole features ('GIT_TEST_SPLIT_INDEX', > 'GIT_TEST_COMMIT_GRAPH', etc.). > > The only thing feedback I can think of would be that this patch could be > moved to earlier in the series (that is, immediately after creating > 'transport_get_remote_bundle_uri()'). That said, I don't feel strongly > either way. It was simple enough to reorder them, so I've done that. 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-t5730-protocol-v2-bundle-uri.sh b/t/lib-t5730-protocol-v2-bundle-uri.sh index 000fcc5e20b..872bc39ad1b 100644 --- a/t/lib-t5730-protocol-v2-bundle-uri.sh +++ b/t/lib-t5730-protocol-v2-bundle-uri.sh @@ -1,5 +1,8 @@ # Included from t573*-protocol-v2-bundle-uri-*.sh +GIT_TEST_BUNDLE_URI=1 +export GIT_TEST_BUNDLE_URI + T5730_PARENT= T5730_URI= T5730_BUNDLE_URI= diff --git a/transport.c b/transport.c index 86460f5be28..b33180226ae 100644 --- a/transport.c +++ b/transport.c @@ -1538,6 +1538,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs) int transport_get_remote_bundle_uri(struct transport *transport, int quiet) { + int value = 0; const struct transport_vtable *vtable = transport->vtable; /* Check config only once. */ @@ -1545,10 +1546,13 @@ int transport_get_remote_bundle_uri(struct transport *transport, int quiet) return 0; /* - * This is intentionally below the transport.injectBundleURI, - * we want to be able to inject into protocol v0, or into the - * dialog of a server who doesn't support this. + * Don't use bundle-uri at all, if configured not to. Only proceed + * if 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) { if (quiet) return -1;