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

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 22, 2020, 20:41 UTC
Message-ID: <20200422204114.GA4850@syl.local>
In-Reply-To: <CAP8UFD0kqSQAnpfUxqDn_qwigQZhq7zyxY_CZhd1nJzqHT1cqw@mail.gmail.com>

On Tue, Apr 21, 2020 at 02:34:18PM +0200, Christian Couder wrote:

Show 49 quoted lines

> On Fri, Apr 17, 2020 at 8:06 PM Jeff King <peff@peff.net> 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.

Show 14 quoted lines

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