Re: [PATCH 1/2] xread: retry after poll on EAGAIN/EWOULDBLOCK

From: Jeff King <peff@peff.net>
Date: Jun 27, 2016, 19:51 UTC
Message-ID: <20160627195138.GA15147@sigill.intra.peff.net>
In-Reply-To: <CAGZ79kZWfGJG=qHtdpuLvQY5XK5P3L0UW7RJ=9ui=PgX+AjErQ@mail.gmail.com>

On Mon, Jun 27, 2016 at 12:43:32PM -0700, Stefan Beller wrote:

Show 8 quoted lines

> There are a few issues:
> 
>     1) How did you spot the bug? ("Experience/Logical thinking" is the hand
>       wavy answer, but if you had a list like
>       [ ] check for mem leak
>       [ ] check for futureproof design
>       [ ] check for failure modes (What if a syscall fails?")
>       [ ] ... List is not complete, but has some made up points

I use the hand-wavy approach to reviews, and I'm pretty sure that's how I saw your bug (just thinking about what the code would do, how I would write it, and then noticing a discrepancy).

I suspect a checklist could make things more thorough, but I think it can also discourage deep thinking. Ideally we have some reviewers who are checklist-oriented looking for those sorts of details and some who are not.

>     2) I used to send out only "done"s, i.e. when I already fixed the
>     issue, instead of acknowledging the problem and postponing the fix
>     for later.  I'll revert to that.

Yes, I tend to use the review thread as a todo list, and do a single session where I read over all of the review mails again (even if I've read and responded to them already), fixing or adjusting the code. Sort of a human version of the automated tool I described. :)

Show 5 quoted lines

> > Of course, none of that would have helped my comment, which was in a
> > "PS" several emails deep in a discussion thread. ;)
> 
> Maybe a "P.S." would get its own point in the todo list not associated
> with code?  Then it would still be on the radar.

Yes, there are definitely design points that are not about a specific part of the code, and you would want to somehow represent them in your todo list.

There is a danger, though, of having a bunch of false positives added to your list: tangent discussion that distracts you from the actual review. And I think the bug here being in a PS is less relevant than being buried amidst other discussion. That made it hard for a tool _or_ a human to find.

-Peff