Re: [PATCH v2 0/4] upload-pack: custom allowed object filters

> On Mon, Jul 27, 2020 at 03:36:57PM -0400, Taylor Blau wrote:
> > On Mon, Jul 27, 2020 at 09:34:32PM +0200, SZEDER Gábor wrote:
> > > On Fri, Jul 24, 2020 at 12:51:33PM -0400, Taylor Blau wrote:
> > > > On Thu, Jul 23, 2020 at 10:43:25PM +0200, SZEDER Gábor wrote:
> > > > > On Wed, Jul 22, 2020 at 09:48:54PM -0400, Taylor Blau wrote:
> > > > > > Here's a much-delayed v2 of my series to teach upload-pack to limit the
> > > > > > kinds of object filters that it is willing to server in a request.
> > > > > >
> > > > > > Much is the same since last time, with two notable exceptions:
> > > > > >
> > > > > >   - We use the 'uploadpackfilter' top-level configuration key instead of
> > > > > >     pretending that 'uploadpack.filter' is top-level, which greatly
> > > > > >     simplifies the call to 'parse_config_key()'.
> > > > > >
> > > > > >   - Instead of writing an err packet, 'git upload-pack' simply 'die()'s,
> > > > >
> > > > > To clarify, I only recommended to pass the same message to die() as in
> > > > > the ERR packet, not dropping the ERR packet, because ...
> > > > >
> > > > > >     which propagates the error through 'git clone' always,
> > > > >
> > > > > it does in the new tests when creating a local clone, but does it
> > > > > really work with all protocols and remote helpers and what not?
> > > > >
> > > > > >     and resolves
> > > > > >     a flaky set of tests that used to result in a SIGPIPE.
> > > > >
> > > > > This doesn't resolve the SIGPIPE flakiness, because 'git upload-pack'
> > > > > can still abort while 'git clone' is still sending packets.  IOW we
> > > > > still need that 'test_must_fail ok=sigpipe' in all new tests.
> > > >
> > > > Let me double check my understanding... I think that you are suggesting
> > > > the following three things:
> > > >
> > > >   - Write the same message as an err packet over the wire as we do when
> > > >     'die()'ing from inside of upload-pack.c
> > >
> > > Yes, though I'm not quite sure that I understand this sentence
> > > correctly, and unless you can convincingly argue in the commit message
> > > that the die() messages make it to the client no matter the
> > > protocol.
> > >
> > > >   - Don't mark said message(s) for translation, matching what we do in
> > > >     the rest of upload-pack.c.
> > >
> > > Yes.
> > >
> > > >   - Re-introduce the 'test_must_fail ok=sigpipe'
> > >
> > > Yes.
> > >
> > > >      and stop grepping stderr for the right message.
> > >
> > > No, please check them, I love those error messages :)
> >
> > Isn't the problem that these messages only sometimes make it to the
> > client depending on what protocol is in use? If so, the right thing to
> > do would be to not grep for them, since it will make that test flakey.
>
> To my understanding the die() messages always make it to the client's
> stderr when cloning from a local repository.  And that's exactly what
> all new tests do, i.e they do run 'git clone ... file://', that's why
> I recommended to pass those fancy error messages to die(), because
> that's easy enough to do, and doesn't require us to make 'git clone'
> SIGPIPE-proof first (which, I agree, is beyond the scope of this
> series).

> > Maybe I'm misunderstanding you/the problem...
> >
> > > > Do I have that right?
> > > >
> > > > Thanks,
> > > > Taylor
> >
> > Thanks,
> > Taylor