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

[Jeff King <peff@peff.net>]

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

> 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. :)

> > 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