Re: [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'

[Taylor Blau <me@ttaylorr.com>]

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