Re: [PATCH v2 5/9] bundle-uri client: add boolean transfer.bundleURI setting

[Victoria Dye <vdye@github.com>]

Æ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;