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

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 20, 2020, 20:05 UTC
Message-ID: <20200720200528.GA91942@syl.lan>
In-Reply-To: <20200708084527.GB2324177@coredump.intra.peff.net>

On Wed, Jul 08, 2020 at 04:45:27AM -0400, Jeff King wrote:

Show 21 quoted lines

> 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'.

Show 7 quoted lines

> > +	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