On 11/28/2022 7:59 PM, Victoria Dye wrote:
> Ævar Arnfjörð Bjarmason via GitGitGadget wrote:
>> From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
>> <avarab@xxxxxxxxx>
>>
>> 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').
I think that is a reasonable recommendation.
>> --- 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.
I will think more on this as I get further into your review and figure out
a way to do the error case tests. At minimum, I've split out some things
so they might be easier to rearrange, but the 'git clone' integration is
(currently) still paired with the implementation in transport.c.
>> 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?
This can be delayed until the next change that actually reads that config.
>> 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?
You're right. This is essentially inserting test code into the product
(although the assert()s would be compiled out, I assume). The only differnce
here is that after the handshake, protocol v2 has not executed the 'ls-refs'
command, while the other protocol versions start with a ref advertisement
in the initial response.
Thanks,
-Stolee
