Re: [PATCH v2 2/9] bundle-uri client: add minimal NOOP client

[Derrick Stolee <derrickstolee@github.com>]

On 11/28/2022 7:57 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>

>> There's a question of what level of encapsulation we should use here,
>> I've opted to use connect.h in clone.c, but we could also e.g. make
>> transport_get_remote_refs() invoke this, i.e. make it implicitly get
>> the bundle-uri list for later steps.
>
> I'm not sure I follow what this sentence is saying. Looking at the
> implementation below [1], you've added a call to
> 'transport_get_remote_bundle_uri()' in 'clone.c', but that's defined in
> 'transport.h' (which is already included in 'clone.c'). Why is 'connect.h'
> needed at all?

It's not. Good catch!

>> This approach means that we don't "support" this in "git fetch" for
>> now. I'm starting with the case of initial clones, although as noted
>> in preceding commits to the protocol documentation nothing about this
>> approach precludes getting bundles on incremental fetches.
>
> This explanation seems more complicated than necessary. I think it's
> sufficient to say "The no-op client is initially used only in 'clone' to
> test the basic functionality. The bundle URI client will be integrated into
> fetch, pull, etc. in later patches".

Definitely can be reworded. Specifically, fetches need more functionality
(coming in part V) before there is value in that integration.

>> For the t5732-protocol-v2-bundle-uri-http.sh it's not easy to set
>> environment variables for git-upload-pack (it's started by Apache), so
>> let's skip the test under T5730_HTTP, and add unused T5730_{FILE,GIT}
>> prerequisites for consistency and future use.
>
> "skip the test" doesn't explain *which* test is skipped (and it doesn't look
> like you skip all of them). I think you're referring to "bad client with
> $T5730_PROTOCOL:// using protocol v2" specifically?

Will make the specific test to skip more clear.

>> diff --git a/bundle-uri.c b/bundle-uri.c
>> index 32022595964..2201b604b11 100644
>> --- a/bundle-uri.c
>> +++ b/bundle-uri.c
>> @@ -571,6 +571,10 @@ int bundle_uri_advertise(struct repository *r, struct strbuf *value)
>>  {
>>  	static int advertise_bundle_uri = -1;
>>
>> +	if (value &&
>> +	    git_env_bool("GIT_TEST_BUNDLE_URI_UNKNOWN_CAPABILITY_VALUE", 0))
>> +		strbuf_addstr(value, "test-unknown-capability-value");
>
> It looks like 'GIT_TEST_BUNDLE_URI_UNKNOWN_CAPABILITY_VALUE' is being used
> to "mock" server responses to test certain behavior on the client side. I'm
> somewhat uncomfortable with how this mixes test-specific code paths with
> application code, and AFAICT nothing similar is done for other
> advertise/command functions in protocol v2. Is there a way to set up tests
> to intercept the client requests and customize the response?

I'm going to investigate how we can test similar functionality within the
test-tool code instead, if possible.

>> +int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,
>> +			  struct bundle_list *bundles, int stateless_rpc)
>> +{
>> +	int line_nr = 1;
>> +
>> +	/* Assert bundle-uri support */
>> +	server_supports_v2("bundle-uri", 1);
>> +
>> +	/* (Re-)send capabilities */
>> +	send_capabilities(fd_out, reader);
>> +
>> +	/* Send command */
>> +	packet_write_fmt(fd_out, "command=bundle-uri\n");
>> +	packet_delim(fd_out);
>> +
>> +	/* Send options */
>> +	if (git_env_bool("GIT_TEST_PROTOCOL_BAD_BUNDLE_URI", 0))
>> +		packet_write_fmt(fd_out, "test-bad-client\n");
>
> Same comment as on 'GIT_TEST_BUNDLE_URI_UNKNOWN_CAPABILITY_VALUE'. There's
> no precedent that I can find for a test variable like this in 'connect.c', and
> "in the middle of client code" doesn't seem like an ideal place for it.
>
> If there really isn't another way of doing this, could the addition of these
> 'GIT_TEST' variables and their associated tests be split out into a
> dedicated commit? That would at least separate the test code paths from the
> application code in the commit history.

I'll definitely split them out, making this change much more about the
test boilerplate. In addition, most of the test boilerplate actually works
without the 'git clone' update, so this can be split into three commits:

1. Create the test infrastructure to check that the server advertises
   the 'bundle-uri' command appropriately.

2. Implement the basic client that issues and parses the 'bundle-uri'
   command. Add the request to 'git clone' and add a test that verifies
   that the client makes the request.

3. Add the extra error condition tests.

>> +++ b/t/lib-t5730-protocol-v2-bundle-uri.sh
>
> nit: this set of tests is used in more than just 't5730', so the name is
> somewhat misleading. It also seems a bit overly-specific to include
> "protocol-v2" in the filename (the tests themselves mention "protocol v2"
> when it's relevant). What about something like 'lib-proto-bundle-uri.sh'
> (using "proto" to mimic 'lib-proto-disable.sh')?

I agree. I think 'lib-bundle-uri-protocol.sh' is a clearer name.

>> +# Poor man's URI escaping. Good enough for the test suite whose trash
>> +# directory has a space in it. See 93c3fcbe4d4 (git-svn: attempt to
>> +# mimic SVN 1.7 URL canonicalization, 2012-07-28) for prior art.
>> +test_uri_escape() {
>> +	sed 's/ /%20/g'
>> +}
>
> This is a good opportunity to unify on a single implementation rather than
> to have multiple similar ones floating around. Can this be moved into
> 'test-lib.sh' (or 'test-lib-functions.sh'?), with 't9119' and 't9120'
> updated to use the new 'test_uri_escape()'?

Will move, although I was not able to find the use in t9120.

>> +test_expect_success "unknown capability value with $T5730_PROTOCOL:// using protocol v2" '
>
> Why is this test only run for the 'file://' transport protocol? And, why
> isn't it in 'lib-t5730-protocol-v2-bundle-uri.sh'? If nothing else, moving
> this test to that file (even if it needs to be conditional on a specific
> protocol) puts the '$T5730_PARENT', '$T5730_BUNDLE_URI_ESCAPED' and
> '$T5730_URI' variables in scope for better readability.

I think this is one of the tests that doesn't work in HTTP, but could be
skipped using a prereq if it is placed in the common test script.

I will rethink this test coverage to see if there is a different way to
check similar behavior without as much insertion into the client/server
code.

>> +int transport_get_remote_bundle_uri(struct transport *transport)
>> +{
>> +	const struct transport_vtable *vtable = transport->vtable;
>> +
>> +	/* Check config only once. */
>> +	if (transport->got_remote_bundle_uri++)
>> +		return 0;
>
> We're only ever going to read the bundle list once per command (or, at least
> once per 'transport' instance), so if 'transport_get_remote_bundle_uri()'
> has already been called, we can safely assume the correct results (if any)
> are in the 'transport' structure.

Yes, although it suffers from a mistake of this form I've seen before:
got_remote_bundle_uri is a single bit, so this only works every other time.
I will fix this.

Thanks for the detailed review!
-Stolee