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

[Jeff King <peff@peff.net>]

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

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

> +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.

> +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