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

>> 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?

>> 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".

>> 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?

>> 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?

>> +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.

>> +++ 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')?

>> +# 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()'?

>> +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.

>> +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.