Re: [RFC/PATCH] upload-pack: disable object filtering when disabled by config

From: Jeff King <peff@peff.net>
Date: Mar 29, 2018, 22:17 UTC
Message-ID: <20180329221725.GM2939@sigill.intra.peff.net>
In-Reply-To: <20180329220257.GA209272@aiede.svl.corp.google.com>

On Thu, Mar 29, 2018 at 03:03:54PM -0700, Jonathan Nieder wrote:

Show 17 quoted lines

> Jeff King wrote:
> > On Wed, Mar 28, 2018 at 01:33:03PM -0700, Jonathan Nieder wrote:
> 
> >> When upload-pack gained partial clone support (v2.17.0-rc0~132^2~12,
> >> 2017-12-08), it was guarded by the uploadpack.allowFilter config item
> >> to allow server operators to control when they start supporting it.
> >>
> >> That config item didn't go far enough, though: it controls whether the
> >> 'filter' capability is advertised, but if a (custom) client ignores
> >> the capability advertisement and passes a filter specification anyway,
> >> the server would handle that despite allowFilter being false.
> [...]
> > Great catch. If I understand correctly, this is an issue in the 2.17.0
> > release candidates. Is this worth delaying the release?
> 
> Yes, IMHO this is a release blocker.  (To put it another way, if this is
> going to delay the release, then we need to revert that patch.)

I think I'd agree on it being a release blocker. Given that your fix is really a one-liner (3 of the lines are just changing the variable name, which I agree with), I'd be fine with applying it on top rather than reverting the original, even if it means delaying the release slightly. It seems like about the same amount of risk to me.

(Sometimes it's actually _less_ risky to apply the one-liner fix, because reverting a large feature can have unintended interactions with other features that were built in the meantime. Looking at the original patch, though, I doubt that is the case here, hence my "about the same amount of risk").

-Peff