Re: [PATCH v3 2/4] upload-pack.c: allow banning certain object filter(s)

From: Jeff King <peff@peff.net>
Date: Jul 31, 2020, 20:54 UTC
Message-ID: <20200731205434.GB1440890@coredump.intra.peff.net>
In-Reply-To: <a0a04277578333daa0b38a8e06e1738b920f5ac3.1596227003.git.me@ttaylorr.com>

On Fri, Jul 31, 2020 at 04:26:31PM -0400, Taylor Blau wrote:

Show 20 quoted lines

> Git clients may ask the server for a partial set of objects, where the
> set of objects being requested is refined by one or more object filters.
> Server administrators can configure 'git upload-pack' to allow or ban
> these filters by setting the 'uploadpack.allowFilter' variable to
> 'true' or 'false', respectively.
> 
> However, administrators using bitmaps may wish to allow certain kinds of
> object filters, but ban others. Specifically, they may wish to allow
> object filters that can be optimized by the use of bitmaps, while
> rejecting other object filters which aren't and represent a perceived
> performance degradation (as well as an increased load factor on the
> server).
> 
> Allow configuring 'git upload-pack' to support object filters on a
> case-by-case basis by introducing two new configuration variables:
> 
>   - 'uploadpackfilter.allow'
>   - 'uploadpackfilter.<kind>.allow'
> 
> where '<kind>' may be one of 'blobNone', 'blobLimit', 'tree', and so on.

Minor nit, but <kind> is "blob:none", "blob:limit", etc. The code and documentation gets this right; it's just the commit message.

I'm pretty sure this is a casualty of updating the syntax as the series was developed. One trick I use is to avoid repeating explanations that are in the documentation from the patch already. I.e., explain "why" here, but it's OK to let "what" come from the patch itself. That's not only one less thing to remember to update, but it's less for reviewers to read through, too.

</meta-patch-advice>

Show 6 quoted lines

> +test_expect_success 'upload-pack fails banned object filters' '
> +	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
> +	test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
> +		"file://$(pwd)/srv.bare" pc3 2>err &&
> +	test_i18ngrep "filter '\''blob:none'\'' not supported" err
> +'

These messages aren't translated now, so we can just use grep, I think.

Ditto in the other tests below.

Show 14 quoted lines

> +static void die_if_using_banned_filter(struct upload_pack_data *data)
> +{
> +	struct list_objects_filter_options *banned = banned_filter(data,
> +								   &data->filter_options);
> +	struct strbuf buf = STRBUF_INIT;
> +	if (!banned)
> +		return;
> +
> +	strbuf_addf(&buf, "git upload-pack: filter '%s' not supported",
> +		    list_object_filter_config_name(banned->choice));
> +
> +	packet_writer_error(&data->writer, "%s\n", buf.buf);
> +	die("%s", buf.buf);
> +}

Hmm, the strbuf was unexpected. I'd have just written it out twice. After all, the messages don't have to be the same. And perhaps we don't want them to be the same? A user receiving the ERR packet would see:

  remote error: git upload-pack: filter 'foo' not supported

do we need the "git upload-pack" part there? Other errors say just "upload-pack". IMHO even that is unnecessarily verbose, and I wouldn't mind a separate patch to reduce it. But definitely going even longer doesn't seem like the right direction. :)

I also wondered about the trailing newline. Other callers of packet_writer_error() don't seem to use it. I think in practice it doesn't matter much because readers will generally be using CHOMP_NEWLINE, but it probably makes sense to be consistent.

> [...]

Aside from this nits, this patch looks good to me.

-Peff