Re: [RFC PATCH 0/2] upload-pack.c: limit allowed filter choices

From: Jeff King <peff@peff.net>
Date: Apr 17, 2020, 18:06 UTC
Message-ID: <20200417180645.GJ1739940@coredump.intra.peff.net>
In-Reply-To: <20200417174030.GB2103@syl.local>

On Fri, Apr 17, 2020 at 11:40:30AM -0600, Taylor Blau wrote:

Show 12 quoted lines

> > What do you think about something like:
> >
> > [promisorFilter "noBlobs"]
> >         type = blob:none
> >         uploadpack = true # maybe "allow" could also mean "true" here
> >         ...
> > ?
> 
> I'm not sure about introducing a layer of indirection here with
> "noBlobs". It's nice that it could perhaps be enabled/disabled for
> different builtins (e.g., by adding 'revList = false', say), but I'm not
> convinced that this is improving all of those cases, either.

Yeah, I don't like forcing the user to invent a subsection name. My first thought was to suggest:

  [promisorFilter "blob:none"]
  uploadpack = true

but your tree example shows why that gets awkward: there are more keys than just "allow this".

> One thing that I can think of (other than replacing the '.' with another
> delimiting character other than '=') is renaming the key from
> 'uploadPack' to 'uploadPackFilter'. I believe that this was suggested by

Yeah, that proposal isn't bad. To me the two viable options seem like:

 - uploadpack.filter.<filter>.*: this has the ugly fake multilevel
   subsection, but stays under uploadpack.*

 - uploadpackfilter.<filter>.*: more natural subsection, but not grouped
   syntactically with other uploadpack stuff

I am actually leaning towards the second. It should make the parsing code less confusing, and it's not like there aren't already other config sections that impact uploadpack.

Show 15 quoted lines

> > > For reference, the patch I was thinking of was this:
> > >
> > >   https://lore.kernel.org/git/20190830121005.GI8571@szeder.dev/
> >
> > Are you using the patches in this series with or without something
> > like the above patch? I am ok to resend this patch series including
> > the above patch (crediting Szeder) if you use something like it.
> 
> We're not using them, but without them we suffer from a problem that if
> we can get a SIGPIPE when writing the "sorry, I don't support that
> filter" message back to the client, then they won't receive it.
> 
> Szeder's patches help address that issue by catching the SIGPIPE and
> popping off enough from the client buffer so that we can write the
> message out before dying.

I definitely think we should pursue that patch, but it really can be done orthogonally. It's an existing bug that affects other instances where upload-pack returns an error. The tests can work around it with "test_must_fail ok=sigpipe" in the meantime.

-Peff