Re: [RFC PATCH 0/2] upload-pack.c: limit allowed filter choices
- From
Christian Couder <christian.couder@gmail.com>
- Date
- Apr 17, 2020, 09:41 UTC
- Message-ID
- <CAP8UFD3v_J3zGqHKa94d71QB82hTsX0MZasERB-jOnY3Ya-uJw@mail.gmail.com>
- In-Reply-To
- <20200318101825.GB1227946@coredump.intra.peff.net>
Hi Taylor and Peff,
On Wed, Mar 18, 2020 at 11:18 AM Jeff King <peff@peff.net> wrote:
Show 7 quoted lines
> > On Tue, Mar 17, 2020 at 02:39:05PM -0600, Taylor Blau wrote: > > > Of course, I would be happy to send along our patches. They are included > > in the series below, and correspond roughly to what we are running at > > GitHub. (For us, there have been a few more clean-ups and additional > > patches, but I squashed them into 2/2 below).
Thanks for the patches, and sorry for the delay in responding!
Show 29 quoted lines
> > The approach is roughly that we have: > > > > - 'uploadpack.filter.allow' -> specifying the default for unspecified > > filter choices, itself defaulting to true in order to maintain > > backwards compatibility, and > > > > - 'uploadpack.filter.<filter>.allow' -> specifying whether or not each > > filter kind is allowed or not. (Originally this was given as 'git > > config uploadpack.filter=blob:none.allow true', but this '=' is > > ambiguous to configuration given over '-c', which itself uses an '=' > > to separate keys from values.) > > One thing that's a little ugly here is the embedded dot in the > subsection (i.e., "filter.<filter>"). It makes it look like a four-level > key, but really there is no such thing in Git. But everything else we > tried was even uglier. > > I think we want to declare a real subsection for each filter and not > just "uploadpack.filter.<filter>". That gives us room to expand to other > config options besides "allow" later on if we need to. > > We don't want to claim "uploadpack.allow" and "uploadpack.<filter>.allow"; > that's too generic. > > Likewise "filter.allow" is too generic. > > We could do "uploadpackfilter.allow" and "uploadpackfilter.<filter>.allow", > but that's both ugly _and_ separates these options from the rest of > uploadpack.*.
What do you think about something like:
[promisorFilter "noBlobs"]
type = blob:none
uploadpack = true # maybe "allow" could also mean "true" here
...
?Show 11 quoted lines
> > I noted in the second patch that there is the unfortunate possibility of > > encountering a SIGPIPE when trying to write the ERR sideband back to a > > client who requested a non-supported filter. Peff and I have had some > > discussion off-list about resurrecting SZEDZER's work which makes room > > in the buffer by reading one packet back from the client when the server > > encounters a SIGPIPE. It is for this reason that I am marking the series > > as 'RFC'. > > 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.
Thanks, Christian.