Jeff King <peff@xxxxxxxx> wrote:
> On Sat, Jul 09, 2016 at 11:45:18PM +0000, Eric Wong wrote:
> > Junio C Hamano <gitster@xxxxxxxxx> 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
> > 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@xxxxxxxxxxxxx/raw
> >
> > Jeff liked it:
> > https://public-inbox.org/git/20160627214947.GA17149@xxxxxxxxxxxxxxxxxxxxx/
>
> 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.
> ---
> 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?
> +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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html