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

From: SZEDER Gábor <[email-masked]>
Date: Mon, 27 Jul 2020 21:59:12 +0200

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