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@xxxxxxxx>
> ---
> 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
>
Attachment:
signature.asc
Description: PGP signature
