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

[Taylor Blau <me@ttaylorr.com>]

On Wed, Jul 08, 2020 at 04:45:27AM -0400, Jeff King wrote:
> On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:
>
> > +static void parse_object_filter_config(const char *var, const char *value,
> > +				       struct upload_pack_data *data)
> > +{
> > +	struct strbuf spec = STRBUF_INIT;
> > +	const char *sub, *key;
> > +	size_t sub_len;
> > +
> > +	if (parse_config_key(var, "uploadpack", &sub, &sub_len, &key))
> > +		return;
> > +	if (!sub || !skip_prefix(sub, "filter.", &sub))
> > +		return;
>
> Just while I'm thinking about the config name and case-sensitivity (from
> the cover letter): if we did want to use this scheme, then
> skip_iprefix() would make this behave more like a regular part of the
> section name.
>
> But I'd prefer to just do away with it by using a scheme that doesn't
> have the extra layer of dots.

Yeah. I definitely flip-flopped on this when preparing this for the
list. I still feel like 'uploadpackfilter' is gross, so I was hoping to
send it with 'uploadpack.filter' (which reads nicely, but forces us to
write some gross code), but both options are frustrating to me for
different reasons.

Playing around with it more, though, I think that uploadpackfilter is
our best bet. I'd hate to introduce a string manipulation bug by munging
the reuslt of 'parse_config_key()', which is so clearly designed for
something like 'uploadpackfilter[.<filter>].allow'.

> > +	if (sub != key)
> > +		strbuf_add(&spec, sub, key - sub - 1);
> > +	strbuf_tolower(&spec);
>
> On the flip side, I'd actually consider _not_ matching the filter name
> case-insensitively. We don't do so elsewhere (e.g., "git rev-list
> --filter=BLOB:NONE" will complain).

I dropped the case-insensitive matching in the latest revision. I don't
think that we need to be overly accommodating here.

> -Peff

Thanks,
Taylor