Æ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'). > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> > Signed-off-by: Derrick Stolee <derrickstolee@xxxxxxxxxx> 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? > --- > 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@xxxxxxxxxx/ > 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? > 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? > + } > + } > + > + /* > + * "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);
