Re: [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Jul 31, 2020, 21:29 UTC
- Message-ID
- <20200731212905.GE3409@syl.lan>
- In-Reply-To
- <20200731210114.GC1440890@coredump.intra.peff.net>
On Fri, Jul 31, 2020 at 05:01:14PM -0400, Jeff King wrote:
Show 28 quoted lines
> 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.
Show 10 quoted lines
> 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