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

From: Taylor Blau <[email-masked]>
Date: Wed, 22 Apr 2020 14:41:14 -0600

On Tue, Apr 21, 2020 at 02:34:18PM +0200, Christian Couder wrote:
> On Fri, Apr 17, 2020 at 8:06 PM Jeff King <[email-masked]> wrote:
> >
> > On Fri, Apr 17, 2020 at 11:40:30AM -0600, Taylor Blau wrote:
> >
> > > > 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".
>
> I like your first thought better than something that starts with
> "uploadPack". And I think if we let people find a subsection name (as
> what I suggest) they might indeed end up with something like:
>
> [promisorFilter "blob:none"]
>      type = blob:none
>      uploadpack = true
>
> as they might lack inspiration. As filters are becoming more and more
> complex though, people might find it much simpler to use the
> subsection name in commands if we let them do that. For example we
> already allow:
>
> git rev-list --filter=combine:<filter1>+<filter2>+...<filterN> ...
>
> which could be simplified to:
>
> git rev-list --filter=combinedFilter ...
>
> (where "combinedFilter" is defined in the config with
> "type=combine:<filter1>+<filter2>+...<filterN>".)
>
> [...]

I really think that we're getting ahead of ourselves here. For now, I
don't think that we have powerful enough filters that it makes sense to
put them together with combine and give them meaningful names. At least,
no one has asked about such a thing on the list, which I take to mean
that people don't have a use for it.

I'm also skeptical about relying on named filters when working with a
server. If the server defines the filter names (as we at GitHub would
do under this proposal), then what use are they to the client? For the
server, I'm not at all convinced that this is beneficial: the extra
layer of indirection through the configuration makes this brittle and
hard-to-follow.

Not to mention that the server could just as easily spell out the whole
filter.

I'm not trying to give you a too-simple proposal, but I think yours
introduces additional complexity that is trying to enable use-cases that
we don't have in practice.

> > > 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.
>
> Ok, maybe I will take a look a this one then.

Thanks.

> Thanks,
> Christian.

Thanks,
Taylor