git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: Jeff King <peff@peff.net>
Cc: Taylor Blau <me@ttaylorr.com>,
	git@vger.kernel.org, chriscool@tuxfamily.org, gitster@pobox.com,
	szeder.dev@gmail.com
Subject: Re: [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'
Date: Fri, 31 Jul 2020 17:29:05 -0400	[thread overview]
Message-ID: <20200731212905.GE3409@syl.lan> (raw)
In-Reply-To: <20200731210114.GC1440890@coredump.intra.peff.net>

On Fri, Jul 31, 2020 at 05:01:14PM -0400, Jeff King wrote:
> On Fri, Jul 31, 2020 at 04:26:39PM -0400, Taylor Blau wrote:
>
> > +test_expect_success 'upload-pack limits tree depth filters' '
> > +	test_config -C srv.bare uploadpackfilter.allow false &&
> > +	test_config -C srv.bare uploadpackfilter.tree.allow true &&
> > +	test_config -C srv.bare uploadpackfilter.tree.maxDepth 0 &&
> > +	test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \
> > +		"file://$(pwd)/srv.bare" pc3 2>err &&
> > +	test_i18ngrep "filter '\''tree'\'' not supported (maximum depth: 0, but got: 1)" err
> > +'
>
> Same i18ngrep comment as in the earlier patch (i.e., we can use grep
> here).
>
> > @@ -1029,6 +1040,11 @@ static void die_if_using_banned_filter(struct upload_pack_data *data)
> >
> >  	strbuf_addf(&buf, "git upload-pack: filter '%s' not supported",
> >  		    list_object_filter_config_name(banned->choice));
> > +	if (banned->choice == LOFC_TREE_DEPTH &&
> > +	    data->tree_filter_max_depth != ULONG_MAX)
> > +		strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"),
> > +			    data->tree_filter_max_depth,
> > +			    banned->tree_exclude_depth);
>
> Hmm. So I see now why you wanted to go with the strbuf in the earlier
> patch. This does still feel awkward, though. You check "is it allowed"
> in an earlier function, we get "nope, it's not allowed", and now we have
> to reimplement the check here. That seems like a maintenance burden.

I'm not sure that I follow. Is the earlier function that you're
referring to 'banned_filter'? If so, the only use of that function is
within 'die_if_using_banned_filter'. 'banned_filter' exists only insofar
as to answer the question "return me the first banned filter, if one
exists, or NULL otherwise".

Then, dying here is as simple as (1) lookup the banned filter, and (2)
check if it's NULL or not.

If you're referring to 'allows_filter_choice', I guess I see what you're
getting it, but to be honest I'm not sure if I'm buying it. If we were
to combine 'allows_filter_choice', 'banned_filter', and
'die_if_using_banned_filter' into one function that traversed the filter
tree and 'die()'d as soon as it got to a banned one, that function would
have to know how to:

  1. Recurse through the tree when it hits a LOFC_COMBINE node.

  2. At each node, translate the filter->choice into the appropriate key
  name, look it up, and then figure out how to interpret its allowed
  status (including falling back to the default if unspecified).

  3. And, it would have to figure out how to format the message at each
  step.

(3) I think is made easier, since we know what message to format based
on whether or not we're in the 'opts->choice == LOFC_TREE_DEPTH' arm or
not. But, there are two more things that we now have to cram into that
same function.

Maybe I'm being too strict an adherent to having simpler functions, but
I'm failing to see how to combine these in a way that's cleaner than
what's written here.

> I think a more natural flow would be either:
>
>   - the "is it allowed" functions calls immediately into the function
>     that sends the error and dies (this might need a conditional if
>     there's a caller who doesn't want to die; I didn't check)
>
> or
>
>   - on failure it populates an error buffer itself, which the caller can
>     then pass along as it sees fit

I guess; I'm not a huge fan of the side-effecting nature, but maybe it's
cleaner.

I dunno. I appreciate your review, but I feel like we're in a bikeshed.

> -Peff

Thanks,
Taylor

  parent reply	other threads:[~2020-07-31 21:29 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
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 [this message]
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=20200731212905.GE3409@syl.lan \
    --to=me@ttaylorr.com \
    --cc=chriscool@tuxfamily.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=szeder.dev@gmail.com \
    --subject='Re: [PATCH v3 4/4] upload-pack.c: introduce '\''uploadpackfilter.tree.maxDepth'\''' \
    /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).