Re: [PATCH v2 3/9] bundle-uri client: add helper for testing server

From: Victoria Dye <vdye@github.com>
Date: Nov 29, 2022, 00:59 UTC
Message-ID: <84c07e84-0805-6163-d77b-cb9f42db402e@github.com>
In-Reply-To: <c3269a24b5780023cbb4d173cb9cfb10c5a4b0d8.1668628303.git.gitgitgadget@gmail.com>

Ævar Arnfjörð Bjarmason via GitGitGadget wrote:

Show 14 quoted lines

> From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
>  <avarab@gmail.com>
> 
> Add a 'test-tool bundle-uri ls-remote' command. This is a thin wrapper
> for issuing protocol v2 "bundle-uri" commands to a server, and to the
> parsing routines in bundle-uri.c.
> 
> Since in the "git clone" case we'll have already done the handshake(),
> but not here, introduce a "got_advertisement" state along with
> "got_remote_heads". It seems to me that the "got_remote_heads" is
> badly named in the first place, and the whole logic of eagerly getting
> ls-refs on handshake() or not could be refactored somewhat, but let's
> not do that now, and instead just add another self-documenting state
> variable.

Maybe I'm missing something, but why not just rename 'got_remote_heads' to something like 'finished_handshake' rather than adding 'got_advertisement' (since, AFAICT, it's always identical in value to 'got_remote_heads').

> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>

This commit also introduces the 'quiet' flag to 'transport_get_remote_bundle_uri()', but there's no mention in the commit message. The message also doesn't explain the changes to existing tests (adding 'bundle.*' settings, swapping out 'git ls-remote' for the new 'test-tool bundle-uri ls-remote' in existing tests, etc.). I think these are all relevant to fully understanding the patch, so could you mention them in your next reroll?

Show 18 quoted lines

> ---
>  builtin/clone.c                       |  2 +-
>  t/helper/test-bundle-uri.c            | 46 +++++++++++++++++++
>  t/lib-t5730-protocol-v2-bundle-uri.sh | 63 ++++++++++++++++++++++-----
>  transport.c                           | 43 ++++++++++++++----
>  transport.h                           |  6 ++-
>  5 files changed, 139 insertions(+), 21 deletions(-)
> 
> diff --git a/t/helper/test-bundle-uri.c b/t/helper/test-bundle-uri.c
> index 25afd393428..ffb975b7b4f 100644
> --- a/t/helper/test-bundle-uri.c
> +++ b/t/helper/test-bundle-uri.c
> @@ -88,6 +132,8 @@ int cmd__bundle_uri(int argc, const char **argv)
>  		return cmd__bundle_uri_parse(argc - 1, argv + 1, KEY_VALUE_PAIRS);
>  	if (!strcmp(argv[1], "parse-config"))
>  		return cmd__bundle_uri_parse(argc - 1, argv + 1, CONFIG_FILE);
> +	if (!strcmp(argv[1], "ls-remote"))
> +		return cmd_ls_remote(argc - 1, argv + 1);

With this helper being added, I'm not sure if/why 'clone' was needed to test the bundle URIs in patch 2 (I assumed integrating with a command was the only way to test it, which is why I didn't mention this in my review [1]). In the spirit of having commits avoid "doing more than one thing" could these patches be reorganized into something like:

1. Add the no-op client & some basic tests around fetching the bundle URI
   list using this test helper.
2. Add the 'transport_get_remote_bundle_uri()' call to 'clone()' with
   clone-specific tests.

It probably wouldn't make the patches much shorter, but it would help avoid the churn of test changes & changing assumptions around 'quiet' & 'got_advertisement' in this patch.

[1] https://lore.kernel.org/git/ca410bed-e8d1-415f-5235-b64fe18bed27@github.com/

Show 15 quoted lines

>  	error("there is no test-tool bundle-uri tool '%s'", argv[1]);
>  
>  usage:
> diff --git a/t/lib-t5730-protocol-v2-bundle-uri.sh b/t/lib-t5730-protocol-v2-bundle-uri.sh
> index 27294e9c976..c327544641b 100644
> --- a/t/lib-t5730-protocol-v2-bundle-uri.sh
> +++ b/t/lib-t5730-protocol-v2-bundle-uri.sh
> @@ -34,7 +34,9 @@ esac
>  test_expect_success "setup protocol v2 $T5730_PROTOCOL:// tests" '
>  	git init "$T5730_PARENT" &&
>  	test_commit -C "$T5730_PARENT" one &&
> -	git -C "$T5730_PARENT" config uploadpack.advertiseBundleURIs true
> +	git -C "$T5730_PARENT" config uploadpack.advertiseBundleURIs true &&
> +	git -C "$T5730_PARENT" config bundle.version 1 &&
> +	git -C "$T5730_PARENT" config bundle.mode all

Why are these config settings added here? I don't see them used anywhere?

Show 25 quoted lines

> diff --git a/transport.c b/transport.c
> index a020adc1f56..86460f5be28 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -371,6 +373,33 @@ static int get_bundle_uri(struct transport *transport)
>  		init_bundle_list(transport->bundles);
>  	}
>  
> +	if (!data->got_advertisement) {
> +		struct ref *refs;
> +		struct git_transport_data *data = transport->data;
> +		enum protocol_version version;
> +
> +		refs = handshake(transport, 0, NULL, 0);
> +		version = data->version;
> +
> +		switch (version) {
> +		case protocol_v2:
> +			assert(!refs);
> +			break;
> +		case protocol_v0:
> +		case protocol_v1:
> +		case protocol_unknown_version:
> +			assert(refs);
> +			break;

Why were these 'refs' assertions added? What are they intended to validate?

Show 9 quoted lines

> +		}
> +	}
> +
> +	/*
> +	 * "Support" protocol v0 and v2 without bundle-uri support by
> +	 * silently degrading to a NOOP.
> +	 */
> +	if (!server_supports_v2("bundle-uri", 0))
> +		return 0;

I was originally confused as to why this was moved out of 'transport_get_remote_bundle_uri()', but it looks like the answer is "we were previously relying on the handshake being done by the time we called 'transport_get_remote_bundle_uri()', but we can't anymore."

> +
>  	packet_reader_init(&reader, data->fd[0], NULL, 0,
>  			   PACKET_READ_CHOMP_NEWLINE |
>  			   PACKET_READ_GENTLE_ON_EOF);