git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: Taylor Blau <me@ttaylorr.com>,
	git@vger.kernel.org, peff@peff.net, chriscool@tuxfamily.org,
	gitster@pobox.com
Subject: Re: [PATCH v2 0/4] upload-pack: custom allowed object filters
Date: Mon, 27 Jul 2020 16:03:55 -0400	[thread overview]
Message-ID: <20200727200355.GD62919@syl.lan> (raw)
In-Reply-To: <20200727195912.GP11341@szeder.dev>

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

  reply	other threads:[~2020-07-27 20:04 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-23  1:48 [PATCH v2 0/4] upload-pack: custom allowed object filters Taylor Blau
2020-07-23  1:48 ` [PATCH v2 1/4] list_objects_filter_options: introduce 'list_object_filter_config_name' Taylor Blau
2020-07-23  1:49 ` [PATCH v2 2/4] upload-pack.c: allow banning certain object filter(s) Taylor Blau
2020-07-23  1:49 ` [PATCH v2 3/4] upload-pack.c: pass 'struct list_objects_filter_options *' Taylor Blau
2020-07-23  1:49 ` [PATCH v2 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth' Taylor Blau
2020-07-23 20:43 ` [PATCH v2 0/4] upload-pack: custom allowed object filters SZEDER Gábor
2020-07-24 16:51   ` Taylor Blau
2020-07-24 19:51     ` Jeff King
2020-07-27 14:25       ` Taylor Blau
2020-07-27 19:34     ` SZEDER Gábor
2020-07-27 19:36       ` Taylor Blau
2020-07-27 19:42         ` Jeff King
2020-07-27 19:59         ` SZEDER Gábor
2020-07-27 20:03           ` Taylor Blau [this message]
2020-07-31 20:26 ` [PATCH v3 " Taylor Blau
2020-07-31 20:26   ` [PATCH v3 1/4] list_objects_filter_options: introduce 'list_object_filter_config_name' Taylor Blau
2020-07-31 20:26   ` [PATCH v3 2/4] upload-pack.c: allow banning certain object filter(s) Taylor Blau
2020-07-31 20:54     ` Jeff King
2020-07-31 21:20       ` Taylor Blau
2020-07-31 20:26   ` [PATCH v3 3/4] upload-pack.c: pass 'struct list_objects_filter_options *' Taylor Blau
2020-07-31 20:26   ` [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth' Taylor Blau
2020-07-31 21:01     ` Jeff King
2020-07-31 21:22       ` Jeff King
2020-07-31 21:30         ` Taylor Blau
2020-07-31 21:29       ` Taylor Blau
2020-07-31 21:36         ` Jeff King
2020-07-31 21:43           ` Jeff King
2020-08-03 18:00 ` [PATCH v4 0/3] upload-pack: custom allowed object filters Taylor Blau
2020-08-03 18:00   ` [PATCH v4 2/3] upload-pack.c: allow banning certain object filter(s) Taylor Blau
2020-08-03 18:00   ` [PATCH v4 1/3] list_objects_filter_options: introduce 'list_object_filter_config_name' Taylor Blau
2020-08-03 18:00   ` [PATCH v4 3/3] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth' Taylor Blau
2020-08-04  0:37   ` [PATCH v4 0/3] upload-pack: custom allowed object filters Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200727200355.GD62919@syl.lan \
    --to=me@ttaylorr.com \
    --cc=chriscool@tuxfamily.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).