On Mon, Jul 27, 2020 at 09:59:12PM +0200, SZEDER Gábor wrote: > 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). Ah, thanks for your patient explanation of what's going on here. These tests _would_ be flakey if run over non-local protocols, but they aren't, so we can (and should) safely grep for the right error message. So the things to do are the original list, nixing "stop grepping stderr for ...". Easy ;-). > > Maybe I'm misunderstanding you/the problem... > > > > > > Do I have that right? > > > > > > > > Thanks, > > > > Taylor > > > > Thanks, > > Taylor Thanks, Taylor
