| Message ID | c3269a24b5780023cbb4d173cb9cfb10c5a4b0d8.1668628303.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bundle URIs IV: advertise over protocol v2 | expand |
Ævar Arnfjörð Bjarmason via GitGitGadget wrote: > 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? > --- > 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/ > 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);
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@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'). 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
diff --git a/builtin/clone.c b/builtin/clone.c index edf98295af2..22b1e506452 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1271,7 +1271,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) * Populate transport->got_remote_bundle_uri and * transport->bundle_uri. We might get nothing. */ - transport_get_remote_bundle_uri(transport); + transport_get_remote_bundle_uri(transport, 1); if (mapped_refs) { int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport)); 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 @@ -3,6 +3,10 @@ #include "bundle-uri.h" #include "strbuf.h" #include "string-list.h" +#include "transport.h" +#include "ref-filter.h" +#include "remote.h" +#include "refs.h" enum input_mode { KEY_VALUE_PAIRS, @@ -68,6 +72,46 @@ usage: usage_with_options(usage, options); } +static int cmd_ls_remote(int argc, const char **argv) +{ + const char *uploadpack = NULL; + struct string_list server_options = STRING_LIST_INIT_DUP; + const char *dest; + struct remote *remote; + struct transport *transport; + int status = 0; + + dest = argc > 1 ? argv[1] : NULL; + + remote = remote_get(dest); + if (!remote) { + if (dest) + die(_("bad repository '%s'"), dest); + die(_("no remote configured to get bundle URIs from")); + } + if (!remote->url_nr) + die(_("remote '%s' has no configured URL"), dest); + + transport = transport_get(remote, NULL); + if (uploadpack) + transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack); + if (server_options.nr) + transport->server_options = &server_options; + + if (transport_get_remote_bundle_uri(transport, 0) < 0) { + error(_("could not get the bundle-uri list")); + status = 1; + goto cleanup; + } + + print_bundle_list(stdout, transport->bundles); + +cleanup: + if (transport_disconnect(transport)) + return 1; + return status; +} + int cmd__bundle_uri(int argc, const char **argv) { const char *usage[] = { @@ -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); 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 ' # Poor man's URI escaping. Good enough for the test suite whose trash @@ -61,9 +63,8 @@ test_expect_success "connect with $T5730_PROTOCOL:// using protocol v2: no bundl git -C "$T5730_PARENT" config uploadpack.advertiseBundleURIs false && GIT_TRACE_PACKET="$PWD/log" \ - git \ - -c protocol.version=2 \ - ls-remote --symref "$T5730_URI" \ + test-tool bundle-uri \ + ls-remote "$T5730_URI" \ >actual 2>err && # Server responded using protocol v2 @@ -76,12 +77,11 @@ test_expect_success "connect with $T5730_PROTOCOL:// using protocol v2: have bun test_when_finished "rm -f log" && test_config -C "$T5730_PARENT" \ - uploadpack.bundleURI "$T5730_BUNDLE_URI_ESCAPED" && + bundle.only.uri "$T5730_BUNDLE_URI_ESCAPED" && GIT_TRACE_PACKET="$PWD/log" \ - git \ - -c protocol.version=2 \ - ls-remote --symref "$T5730_URI" \ + test-tool bundle-uri \ + ls-remote "$T5730_URI" \ >actual 2>err && # Server responded using protocol v2 @@ -94,8 +94,8 @@ test_expect_success "connect with $T5730_PROTOCOL:// using protocol v2: have bun test_expect_success !T5730_HTTP "bad client with $T5730_PROTOCOL:// using protocol v2" ' test_when_finished "rm -f log" && - test_config -C "$T5730_PARENT" uploadpack.bundleURI \ - "$T5730_BUNDLE_URI_ESCAPED" && + test_config -C "$T5730_PARENT" \ + bundle.only.uri "$T5730_BUNDLE_URI_ESCAPED" && cat >err.expect <<-\EOF && Cloning into '"'"'child'"'"'... @@ -146,3 +146,46 @@ test_expect_success !T5730_HTTP "bad client with $T5730_PROTOCOL:// using protoc grep "clone> test-bad-client$" log >sent-bad-request && test_file_not_empty sent-bad-request ' + +test_expect_success "ls-remote with $T5730_PROTOCOL:// using protocol v2" ' + test_when_finished "rm -f log" && + + test_config -C "$T5730_PARENT" \ + bundle.only.uri "$T5730_BUNDLE_URI_ESCAPED" && + + # All data about bundle URIs + cat >expect <<-EOF && + [bundle] + version = 1 + mode = all + EOF + GIT_TRACE_PACKET="$PWD/log" \ + test-tool bundle-uri \ + ls-remote \ + "$T5730_URI" \ + >actual && + test_cmp_config_output expect actual +' + +test_expect_success "ls-remote with $T5730_PROTOCOL:// using protocol v2 and extra data" ' + test_when_finished "rm -f log" && + + test_config -C "$T5730_PARENT" \ + bundle.only.uri "$T5730_BUNDLE_URI_ESCAPED" && + + # Extra data should be ignored + test_config -C "$T5730_PARENT" bundle.only.extra bogus && + + # All data about bundle URIs + cat >expect <<-EOF && + [bundle] + version = 1 + mode = all + EOF + GIT_TRACE_PACKET="$PWD/log" \ + test-tool bundle-uri \ + ls-remote \ + "$T5730_URI" \ + >actual && + test_cmp_config_output expect actual +' diff --git a/transport.c b/transport.c index a020adc1f56..86460f5be28 100644 --- a/transport.c +++ b/transport.c @@ -198,6 +198,7 @@ struct git_transport_data { struct git_transport_options options; struct child_process *conn; int fd[2]; + unsigned got_advertisement : 1; unsigned got_remote_heads : 1; enum protocol_version version; struct oid_array extra_have; @@ -346,6 +347,7 @@ static struct ref *handshake(struct transport *transport, int for_push, BUG("unknown protocol version"); } data->got_remote_heads = 1; + data->got_advertisement = 1; transport->hash_algo = reader.hash_algo; if (reader.line_peeked) @@ -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; + } + } + + /* + * "Support" protocol v0 and v2 without bundle-uri support by + * silently degrading to a NOOP. + */ + if (!server_supports_v2("bundle-uri", 0)) + return 0; + packet_reader_init(&reader, data->fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE | PACKET_READ_GENTLE_ON_EOF); @@ -1507,7 +1536,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs) return rc; } -int transport_get_remote_bundle_uri(struct transport *transport) +int transport_get_remote_bundle_uri(struct transport *transport, int quiet) { const struct transport_vtable *vtable = transport->vtable; @@ -1515,20 +1544,16 @@ int transport_get_remote_bundle_uri(struct transport *transport) if (transport->got_remote_bundle_uri++) return 0; - /* - * "Support" protocol v0 and v2 without bundle-uri support by - * silently degrading to a NOOP. - */ - if (!server_supports_v2("bundle-uri", 0)) - return 0; - /* * This is intentionally below the transport.injectBundleURI, * we want to be able to inject into protocol v0, or into the * dialog of a server who doesn't support this. */ - if (!vtable->get_bundle_uri) + if (!vtable->get_bundle_uri) { + if (quiet) + return -1; return error(_("bundle-uri operation not supported by protocol")); + } if (vtable->get_bundle_uri(transport) < 0) return error(_("could not retrieve server-advertised bundle-uri list")); diff --git a/transport.h b/transport.h index 85150f504fb..dd0115b83bf 100644 --- a/transport.h +++ b/transport.h @@ -297,8 +297,12 @@ const struct ref *transport_get_remote_refs(struct transport *transport, /** * Retrieve bundle URI(s) from a remote. Populates "struct * transport"'s "bundle_uri" and "got_remote_bundle_uri". + * + * With `quiet=1` it will not complain if the serve doesn't support + * the protocol, but only if we discover the server uses it, and + * encounter issues then. */ -int transport_get_remote_bundle_uri(struct transport *transport); +int transport_get_remote_bundle_uri(struct transport *transport, int quiet); /* * Fetch the hash algorithm used by a remote.