Re: What's cooking in git.git (Jul 2016, #03; Fri, 8)

From: Eric Wong <e@80x24.org>
Date: Jul 10, 2016, 03:47 UTC
Message-ID: <20160710034736.GA19151@dcvr.yhbt.net>
In-Reply-To: <20160710025232.GA4666@sigill.intra.peff.net>

Jeff King <peff@peff.net> wrote:

Show 6 quoted lines

> On Sat, Jul 09, 2016 at 11:45:18PM +0000, Eric Wong wrote:
> > Junio C Hamano <gitster@pobox.com> wrote:
> > > * sb/submodule-parallel-fetch (2016-06-27) 2 commits
> > >   (merged to 'next' on 2016-07-06 at de5fd35)
> > >  + xwrite: poll on non-blocking FDs
> > >  + xread: retry after poll on EAGAIN/EWOULDBLOCK

Show 15 quoted lines

> > Any thoughts on my cleanup 3/2 patch for this series?
> > ("hoist out io_wait function for xread and xwrite")
> > https://public-inbox.org/git/20160627201311.GA7039@dcvr.yhbt.net/raw
> > 
> > Jeff liked it:
> > https://public-inbox.org/git/20160627214947.GA17149@sigill.intra.peff.net/
> 
> I did (and do) like it. I think it may have simply gotten lost in the
> mass of "should we handle EAGAIN from getdelim()" patches I sent (to
> which I concluded "probably not").
> 
> There was one minor improvement I suggested[1] (and which you seemed to
> like), which is to push the errno check into the function. That wasn't
> expressed in patch form, though, so I've included below a version of
> your patch with my suggestion squashed in.

Yes, I'm fine with either, but I'm slightly thrown off by a function relying on errno being set by the caller, even if it is errno. So maybe localizing it is better (see below)

> I didn't include the test I mentioned in [2]. I think the potential for
> portability and raciness problems make it probably not worth the
> trouble.

Agreed.

Show 6 quoted lines

> ---
> Since both conditionals just call "continue", you could actually fold
> them into a single if() in each caller, but I think it's easier to
> follow as two separate ones.
> 
> You could actually fold the t

Copy-paste error?

Show 7 quoted lines

> +static int handle_nonblock(int fd, short poll_events)
> +{
> +	struct pollfd pfd;
> +
> +	if (errno != EAGAIN && errno != EWOULDBLOCK)
> +		return 0;
> +

I prefer localizing errno and having the caller pass it explicitly:

static int handle_nonblock(int fd, short poll_events, int err)
{
	struct pollfd pfd;

	if (err != EAGAIN && err != EWOULDBLOCK)
		return 0;

And the callers would pass errno:

	if (handle_nonblock(fd, POLLIN, errno))
		continue;