Re: [PATCH 4/4] upload-pack: only accept packfile-uris if we advertised it

[Junio C Hamano <gitster@pobox.com>]

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);