Re: [PATCH v2 5/9] bundle-uri client: add boolean transfer.bundleURI setting
- From
Victoria Dye <vdye@github.com>
- Date
- Nov 29, 2022, 01:03 UTC
- Message-ID
- <4ec06afa-d804-16b1-f140-dfd9ebd912e5@github.com>
- In-Reply-To
- <933974689312bbb130236c81550ee3467f295a43.1668628303.git.gitgitgadget@gmail.com>
Ævar Arnfjörð Bjarmason via GitGitGadget wrote:
Show 10 quoted lines
> 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!
Show 61 quoted lines
> ---
> 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;