| Message ID | 20240228225050.GA1159078@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | some v2 capability advertisement cleanups | expand |
Jeff King <peff@peff.net> writes: > I suspect in the long term that we may have other ways to trigger this > feature than the static blobpackfileuri config (e.g., a hook that knows > about site-specific packfiles "somehow"). So we may need to update the > test later for that, but presumably in the vanilla config we'll continue > to skip advertising it. Sounds quite sensible. Thanks for a series that is very pleasant to read. > > t/t5702-protocol-v2.sh | 18 ++++++++++++++++++ > upload-pack.c | 16 +++++++--------- > 2 files changed, 25 insertions(+), 9 deletions(-) > > diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh > index 6ef4971845..902e42c1c0 100755 > --- a/t/t5702-protocol-v2.sh > +++ b/t/t5702-protocol-v2.sh > @@ -778,6 +778,24 @@ test_expect_success 'archive with custom path does not request v2' ' > ! grep ^GIT_PROTOCOL env.trace > ' > > +test_expect_success 'reject client packfile-uris if not advertised' ' > + { > + packetize command=fetch && > + printf 0001 && > + packetize packfile-uris https && > + packetize done && > + printf 0000 > + } >input && > + test_must_fail env GIT_PROTOCOL=version=2 \ > + git upload-pack client <input && > + test_must_fail env GIT_PROTOCOL=version=2 \ > + git -c uploadpack.blobpackfileuri \ > + upload-pack client <input && > + GIT_PROTOCOL=version=2 \ > + git -c uploadpack.blobpackfileuri=anything \ > + upload-pack client <input > +' > + > # Test protocol v2 with 'http://' transport > # > . "$TEST_DIRECTORY"/lib-httpd.sh > diff --git a/upload-pack.c b/upload-pack.c > index 491ef51daa..66f4de9d87 100644 > --- a/upload-pack.c > +++ b/upload-pack.c > @@ -113,6 +113,7 @@ struct upload_pack_data { > unsigned done : 1; /* v2 only */ > unsigned allow_ref_in_want : 1; /* v2 only */ > unsigned allow_sideband_all : 1; /* v2 only */ > + unsigned allow_packfile_uris : 1; /* v2 only */ > unsigned advertise_sid : 1; > unsigned sent_capabilities : 1; > }; > @@ -1362,6 +1363,9 @@ static int upload_pack_config(const char *var, const char *value, > data->allow_ref_in_want = git_config_bool(var, value); > } else if (!strcmp("uploadpack.allowsidebandall", var)) { > data->allow_sideband_all = git_config_bool(var, value); > + } else if (!strcmp("uploadpack.blobpackfileuri", var)) { > + if (value) > + data->allow_packfile_uris = 1; > } else if (!strcmp("core.precomposeunicode", var)) { > precomposed_unicode = git_config_bool(var, value); > } else if (!strcmp("transfer.advertisesid", var)) { > @@ -1647,7 +1651,8 @@ static void process_args(struct packet_reader *request, > continue; > } > > - if (skip_prefix(arg, "packfile-uris ", &p)) { > + if (data->allow_packfile_uris && > + skip_prefix(arg, "packfile-uris ", &p)) { > string_list_split(&data->uri_protocols, p, ',', -1); > continue; > } > @@ -1847,8 +1852,6 @@ int upload_pack_advertise(struct repository *r, > get_upload_pack_config(r, &data); > > if (value) { > - char *str = NULL; > - > strbuf_addstr(value, "shallow wait-for-done"); > > if (data.allow_filter) > @@ -1860,13 +1863,8 @@ int upload_pack_advertise(struct repository *r, > if (data.allow_sideband_all) > strbuf_addstr(value, " sideband-all"); > > - if (!repo_config_get_string(r, > - "uploadpack.blobpackfileuri", > - &str) && > - str) { > + if (data.allow_packfile_uris) > strbuf_addstr(value, " packfile-uris"); > - free(str); > - } > } > > upload_pack_data_clear(&data);
On Wed, Feb 28, 2024 at 05:50:50PM -0500, Jeff King wrote: > diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh > index 6ef4971845..902e42c1c0 100755 > --- a/t/t5702-protocol-v2.sh > +++ b/t/t5702-protocol-v2.sh > @@ -778,6 +778,24 @@ test_expect_success 'archive with custom path does not request v2' ' > ! grep ^GIT_PROTOCOL env.trace > ' > > +test_expect_success 'reject client packfile-uris if not advertised' ' > + { > + packetize command=fetch && > + printf 0001 && > + packetize packfile-uris https && > + packetize done && > + printf 0000 > + } >input && > + test_must_fail env GIT_PROTOCOL=version=2 \ > + git upload-pack client <input && > + test_must_fail env GIT_PROTOCOL=version=2 \ > + git -c uploadpack.blobpackfileuri \ > + upload-pack client <input && > + GIT_PROTOCOL=version=2 \ > + git -c uploadpack.blobpackfileuri=anything \ > + upload-pack client <input > +' Sorry, this needs one tweak to pass under the sha256 CI job: diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 902e42c1c0..1ef540f73d 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -781,6 +781,7 @@ test_expect_success 'archive with custom path does not request v2' ' test_expect_success 'reject client packfile-uris if not advertised' ' { packetize command=fetch && + packetize object-format=$(test_oid algo) && printf 0001 && packetize packfile-uris https && packetize done && Otherwise the server complains that the other side did not respect its advertised object-format (I sure am glad to have included the final "hey, this input works, right?" test there, as that is what caught it). -Peff
Jeff King <peff@peff.net> writes: > Sorry, this needs one tweak to pass under the sha256 CI job: > > diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh > index 902e42c1c0..1ef540f73d 100755 > --- a/t/t5702-protocol-v2.sh > +++ b/t/t5702-protocol-v2.sh > @@ -781,6 +781,7 @@ test_expect_success 'archive with custom path does not request v2' ' > test_expect_success 'reject client packfile-uris if not advertised' ' > { > packetize command=fetch && > + packetize object-format=$(test_oid algo) && > printf 0001 && > packetize packfile-uris https && > packetize done && > > Otherwise the server complains that the other side did not respect its > advertised object-format (I sure am glad to have included the final > "hey, this input works, right?" test there, as that is what caught it). Ah, good finding. Will use it to amend. I wonder if it is still worth testing if the command is happy with an input that lacks object-format capability under SHA-1 mode. This test piece is primarily about packfile-uris, so in that sense, we are not all that interested in that unspecified client object-format defaults to the initial value of serve.c:client_hash_algo (which is SHA-1), and not testing that here is perfectly fine, I guess. Thanks.
On Thu, Feb 29, 2024 at 08:34:48AM -0800, Junio C Hamano wrote: > > Otherwise the server complains that the other side did not respect its > > advertised object-format (I sure am glad to have included the final > > "hey, this input works, right?" test there, as that is what caught it). > > Ah, good finding. Will use it to amend. > > I wonder if it is still worth testing if the command is happy with > an input that lacks object-format capability under SHA-1 mode. This > test piece is primarily about packfile-uris, so in that sense, we > are not all that interested in that unspecified client object-format > defaults to the initial value of serve.c:client_hash_algo (which is > SHA-1), and not testing that here is perfectly fine, I guess. Yeah, if we want to test it, I'd prefer to do so separately as its own test rather than keeping it as a subtle side effect of this one. I looked around to see if it might exist already, but I didn't find one (OTOH, it is hard to grep for since we are looking for the _absence_ of an object-format line in hand-crafted input). But taking a step back, why do we care about this case? It is because older clients that speak v2 but do not yet know about object-format will make a request without it, and they should still work in sha1 mode. So the more general thing to test here is whether those older versions can successfully fetch from a newer server. There are tests in t/interop for cloning and fetching from a different version. I kind of doubt anybody runs them regularly, though (and picking the interesting versions to find this case isn't entirely trivial). So I dunno. -Peff
On Wed, Feb 28, 2024 at 05:50:50PM -0500, Jeff King wrote: > Clients are only supposed to request particular capabilities or features > if the server advertised them. For the "packfile-uris" feature, we only > advertise it if uploadpack.blobpacfileuri is set, but we always accept a Nit: s/uploadpack.blobpackfileuri. I noticed that this isn't actually documented in git-config(1), but that's not a problem of this commit. > request from the client regardless. > > In practice this doesn't really hurt anything, as we'd pass the client's > protocol list on to pack-objects, which ends up ignoring it. But we > should try to follow the protocol spec, and tightening this up may catch > buggy or misbehaving clients more easily. > > Thanks to recent refactoring, we can hoist the config check from Nit: s/refactoring/&s. Patrick > upload_pack_advertise() into upload_pack_config(). Note the subtle > handling of a value-less bool (which does not count for triggering an > advertisement). > > Signed-off-by: Jeff King <peff@peff.net> > --- > I suspect in the long term that we may have other ways to trigger this > feature than the static blobpackfileuri config (e.g., a hook that knows > about site-specific packfiles "somehow"). So we may need to update the > test later for that, but presumably in the vanilla config we'll continue > to skip advertising it. > > t/t5702-protocol-v2.sh | 18 ++++++++++++++++++ > upload-pack.c | 16 +++++++--------- > 2 files changed, 25 insertions(+), 9 deletions(-) > > diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh > index 6ef4971845..902e42c1c0 100755 > --- a/t/t5702-protocol-v2.sh > +++ b/t/t5702-protocol-v2.sh > @@ -778,6 +778,24 @@ test_expect_success 'archive with custom path does not request v2' ' > ! grep ^GIT_PROTOCOL env.trace > ' > > +test_expect_success 'reject client packfile-uris if not advertised' ' > + { > + packetize command=fetch && > + printf 0001 && > + packetize packfile-uris https && > + packetize done && > + printf 0000 > + } >input && > + test_must_fail env GIT_PROTOCOL=version=2 \ > + git upload-pack client <input && > + test_must_fail env GIT_PROTOCOL=version=2 \ > + git -c uploadpack.blobpackfileuri \ > + upload-pack client <input && > + GIT_PROTOCOL=version=2 \ > + git -c uploadpack.blobpackfileuri=anything \ > + upload-pack client <input > +' > + > # Test protocol v2 with 'http://' transport > # > . "$TEST_DIRECTORY"/lib-httpd.sh > diff --git a/upload-pack.c b/upload-pack.c > index 491ef51daa..66f4de9d87 100644 > --- a/upload-pack.c > +++ b/upload-pack.c > @@ -113,6 +113,7 @@ struct upload_pack_data { > unsigned done : 1; /* v2 only */ > unsigned allow_ref_in_want : 1; /* v2 only */ > unsigned allow_sideband_all : 1; /* v2 only */ > + unsigned allow_packfile_uris : 1; /* v2 only */ > unsigned advertise_sid : 1; > unsigned sent_capabilities : 1; > }; > @@ -1362,6 +1363,9 @@ static int upload_pack_config(const char *var, const char *value, > data->allow_ref_in_want = git_config_bool(var, value); > } else if (!strcmp("uploadpack.allowsidebandall", var)) { > data->allow_sideband_all = git_config_bool(var, value); > + } else if (!strcmp("uploadpack.blobpackfileuri", var)) { > + if (value) > + data->allow_packfile_uris = 1; > } else if (!strcmp("core.precomposeunicode", var)) { > precomposed_unicode = git_config_bool(var, value); > } else if (!strcmp("transfer.advertisesid", var)) { > @@ -1647,7 +1651,8 @@ static void process_args(struct packet_reader *request, > continue; > } > > - if (skip_prefix(arg, "packfile-uris ", &p)) { > + if (data->allow_packfile_uris && > + skip_prefix(arg, "packfile-uris ", &p)) { > string_list_split(&data->uri_protocols, p, ',', -1); > continue; > } > @@ -1847,8 +1852,6 @@ int upload_pack_advertise(struct repository *r, > get_upload_pack_config(r, &data); > > if (value) { > - char *str = NULL; > - > strbuf_addstr(value, "shallow wait-for-done"); > > if (data.allow_filter) > @@ -1860,13 +1863,8 @@ int upload_pack_advertise(struct repository *r, > if (data.allow_sideband_all) > strbuf_addstr(value, " sideband-all"); > > - if (!repo_config_get_string(r, > - "uploadpack.blobpackfileuri", > - &str) && > - str) { > + if (data.allow_packfile_uris) > strbuf_addstr(value, " packfile-uris"); > - free(str); > - } > } > > upload_pack_data_clear(&data); > -- > 2.44.0.rc2.424.gbdbf4d014b >
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 6ef4971845..902e42c1c0 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -778,6 +778,24 @@ test_expect_success 'archive with custom path does not request v2' ' ! grep ^GIT_PROTOCOL env.trace ' +test_expect_success 'reject client packfile-uris if not advertised' ' + { + packetize command=fetch && + printf 0001 && + packetize packfile-uris https && + packetize done && + printf 0000 + } >input && + test_must_fail env GIT_PROTOCOL=version=2 \ + git upload-pack client <input && + test_must_fail env GIT_PROTOCOL=version=2 \ + git -c uploadpack.blobpackfileuri \ + upload-pack client <input && + GIT_PROTOCOL=version=2 \ + git -c uploadpack.blobpackfileuri=anything \ + upload-pack client <input +' + # Test protocol v2 with 'http://' transport # . "$TEST_DIRECTORY"/lib-httpd.sh diff --git a/upload-pack.c b/upload-pack.c index 491ef51daa..66f4de9d87 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -113,6 +113,7 @@ struct upload_pack_data { unsigned done : 1; /* v2 only */ unsigned allow_ref_in_want : 1; /* v2 only */ unsigned allow_sideband_all : 1; /* v2 only */ + unsigned allow_packfile_uris : 1; /* v2 only */ unsigned advertise_sid : 1; unsigned sent_capabilities : 1; }; @@ -1362,6 +1363,9 @@ static int upload_pack_config(const char *var, const char *value, data->allow_ref_in_want = git_config_bool(var, value); } else if (!strcmp("uploadpack.allowsidebandall", var)) { data->allow_sideband_all = git_config_bool(var, value); + } else if (!strcmp("uploadpack.blobpackfileuri", var)) { + if (value) + data->allow_packfile_uris = 1; } else if (!strcmp("core.precomposeunicode", var)) { precomposed_unicode = git_config_bool(var, value); } else if (!strcmp("transfer.advertisesid", var)) { @@ -1647,7 +1651,8 @@ static void process_args(struct packet_reader *request, continue; } - if (skip_prefix(arg, "packfile-uris ", &p)) { + if (data->allow_packfile_uris && + skip_prefix(arg, "packfile-uris ", &p)) { string_list_split(&data->uri_protocols, p, ',', -1); continue; } @@ -1847,8 +1852,6 @@ int upload_pack_advertise(struct repository *r, get_upload_pack_config(r, &data); if (value) { - char *str = NULL; - strbuf_addstr(value, "shallow wait-for-done"); if (data.allow_filter) @@ -1860,13 +1863,8 @@ int upload_pack_advertise(struct repository *r, if (data.allow_sideband_all) strbuf_addstr(value, " sideband-all"); - if (!repo_config_get_string(r, - "uploadpack.blobpackfileuri", - &str) && - str) { + if (data.allow_packfile_uris) strbuf_addstr(value, " packfile-uris"); - free(str); - } } upload_pack_data_clear(&data);
Clients are only supposed to request particular capabilities or features if the server advertised them. For the "packfile-uris" feature, we only advertise it if uploadpack.blobpacfileuri is set, but we always accept a request from the client regardless. In practice this doesn't really hurt anything, as we'd pass the client's protocol list on to pack-objects, which ends up ignoring it. But we should try to follow the protocol spec, and tightening this up may catch buggy or misbehaving clients more easily. Thanks to recent refactoring, we can hoist the config check from upload_pack_advertise() into upload_pack_config(). Note the subtle handling of a value-less bool (which does not count for triggering an advertisement). Signed-off-by: Jeff King <peff@peff.net> --- I suspect in the long term that we may have other ways to trigger this feature than the static blobpackfileuri config (e.g., a hook that knows about site-specific packfiles "somehow"). So we may need to update the test later for that, but presumably in the vanilla config we'll continue to skip advertising it. t/t5702-protocol-v2.sh | 18 ++++++++++++++++++ upload-pack.c | 16 +++++++--------- 2 files changed, 25 insertions(+), 9 deletions(-)