Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 20, 2020, 20:07 UTC
Message-ID: <20200720200739.GB91942@syl.lan>
In-Reply-To: <20200715105521.GB3262157@coredump.intra.peff.net>

On Wed, Jul 15, 2020 at 06:55:21AM -0400, Jeff King wrote:

Show 20 quoted lines

> On Wed, Jul 15, 2020 at 12:00:43PM +0200, SZEDER Gábor wrote:
>
> > Once upon a time I had a PoC patch to deal with 'git upload-pack'
> > aborting while 'git fetch' is still send_request()-ing, by catching
> > the write error to the closed connection and trying read any pending
> > ERR packets; Christian cleaned it up and submitted it with a proper
> > commit message in
> >
> >   https://public-inbox.org/git/20200422163357.27056-1-chriscool@tuxfamily.org/
> >
> > but it haven't been picked up yet.  Disappointingly, that patch
> > doesn't solve these issues...  I haven't looked what's going on
> > (perhaps 'git clone' does something differently than 'git fetch'?  no
> > idea)
>
> I suspect it is that fetch ignores SIGPIPE, but clone does not. So even
> when we see a 141 exit code from fetch, it is probably generated
> synthetically from exit(141) after we saw EPIPE. And your patch works
> there because we have a chance to pump the read-side of the pipe,
> whereas in git-clone we die immediately via the signal.

Heh. I was hoping to be rid of those errors with Christian's patches, but it sounds like the problem is coming from outside of 'upload-pack' and instead in 'clone'.

That reasoning seems sound to me, but I'd rather not touch clone in this patch series if I don't have to. What I'd rather do is something like:

  - Introduce this patch series with the 'test_must_fail ok=sigpipe',
    and no error checking.

  - Modify clone to swallow these errors and eat a packet or two.

  - Then, drop the 'ok=sigpipe' from t5616 after 'git clone' is a little
    bit smarter here.

Maybe more steps than is strictly necessary, but I think it keeps the scope of the review on this series reasonable, which is a tradeoff that I'm willing to make.

Show 5 quoted lines

> Probably git-clone should ignore SIGPIPE during the network transfer
> portion of the process for the same reasons given in 143588949c (fetch:
> ignore SIGPIPE during network operation, 2019-03-03).
>
> -Peff

Thanks, Taylor