git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, chriscool@tuxfamily.org, gitster@pobox.com,
	szeder.dev@gmail.com
Subject: Re: [PATCH v3 2/4] upload-pack.c: allow banning certain object filter(s)
Date: Fri, 31 Jul 2020 16:54:34 -0400	[thread overview]
Message-ID: <20200731205434.GB1440890@coredump.intra.peff.net> (raw)
In-Reply-To: <a0a04277578333daa0b38a8e06e1738b920f5ac3.1596227003.git.me@ttaylorr.com>

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

  reply	other threads:[~2020-07-31 20:54 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-23  1:48 [PATCH v2 0/4] upload-pack: custom allowed object filters Taylor Blau
2020-07-23  1:48 ` [PATCH v2 1/4] list_objects_filter_options: introduce 'list_object_filter_config_name' Taylor Blau
2020-07-23  1:49 ` [PATCH v2 2/4] upload-pack.c: allow banning certain object filter(s) Taylor Blau
2020-07-23  1:49 ` [PATCH v2 3/4] upload-pack.c: pass 'struct list_objects_filter_options *' Taylor Blau
2020-07-23  1:49 ` [PATCH v2 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth' Taylor Blau
2020-07-23 20:43 ` [PATCH v2 0/4] upload-pack: custom allowed object filters SZEDER Gábor
2020-07-24 16:51   ` Taylor Blau
2020-07-24 19:51     ` Jeff King
2020-07-27 14:25       ` Taylor Blau
2020-07-27 19:34     ` SZEDER Gábor
2020-07-27 19:36       ` Taylor Blau
2020-07-27 19:42         ` Jeff King
2020-07-27 19:59         ` SZEDER Gábor
2020-07-27 20:03           ` Taylor Blau
2020-07-31 20:26 ` [PATCH v3 " Taylor Blau
2020-07-31 20:26   ` [PATCH v3 1/4] list_objects_filter_options: introduce 'list_object_filter_config_name' Taylor Blau
2020-07-31 20:26   ` [PATCH v3 2/4] upload-pack.c: allow banning certain object filter(s) Taylor Blau
2020-07-31 20:54     ` Jeff King [this message]
2020-07-31 21:20       ` Taylor Blau
2020-07-31 20:26   ` [PATCH v3 3/4] upload-pack.c: pass 'struct list_objects_filter_options *' Taylor Blau
2020-07-31 20:26   ` [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth' Taylor Blau
2020-07-31 21:01     ` Jeff King
2020-07-31 21:22       ` Jeff King
2020-07-31 21:30         ` Taylor Blau
2020-07-31 21:29       ` Taylor Blau
2020-07-31 21:36         ` Jeff King
2020-07-31 21:43           ` Jeff King
2020-08-03 18:00 ` [PATCH v4 0/3] upload-pack: custom allowed object filters Taylor Blau
2020-08-03 18:00   ` [PATCH v4 2/3] upload-pack.c: allow banning certain object filter(s) Taylor Blau
2020-08-03 18:00   ` [PATCH v4 1/3] list_objects_filter_options: introduce 'list_object_filter_config_name' Taylor Blau
2020-08-03 18:00   ` [PATCH v4 3/3] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth' Taylor Blau
2020-08-04  0:37   ` [PATCH v4 0/3] upload-pack: custom allowed object filters Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200731205434.GB1440890@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=chriscool@tuxfamily.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.com \
    --cc=szeder.dev@gmail.com \
    --subject='Re: [PATCH v3 2/4] upload-pack.c: allow banning certain object filter(s)' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).