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

From: Stefan Beller <sbeller@google.com>
Date: Jun 27, 2016, 19:43 UTC
Message-ID: <CAGZ79kZWfGJG=qHtdpuLvQY5XK5P3L0UW7RJ=9ui=PgX+AjErQ@mail.gmail.com>
In-Reply-To: <20160627191720.GE9594@sigill.intra.peff.net>

On Mon, Jun 27, 2016 at 12:17 PM, Jeff King <peff@peff.net> wrote:

Show 17 quoted lines

> On Mon, Jun 27, 2016 at 09:49:06AM -0700, Stefan Beller wrote:
>
>> Quite a while ago, when I started doing code reviews professionally, I
>> wondered if the code review procedure can be semi-automated, as
>> automation helps keeping the error rate low. By that I mean having a
>> check list which I can check off each point for each patch. That seems
>> to be very good in theory, but when trying it I was finding myself
>> doing a lot of unneeded work as some points of such a check list just
>> do not apply for a specific patch. So I did not follow through with
>> that.
>
> I have wondered, too, if we could have better tooling to help us with
> reviews. But one of the things I really _like_ about doing reviews for
> git (versus other projects) is that doing review via email is
> unconstrained. The primary recipient is human, and I can format and say
> whatever I like in the way that best communicates to the human, without
> worrying about fitting my comments into a pre-made form.

Maybe I did not mean automation, but rather a more structured approach, in the review itself. (I just realize this is a slightly different problem than what we saw here).

Here we had a case of:

   1) Jeff: "Wait, there is a bug, please fix!"
   2) Stefan: "ok will do."
   3) Stefan doesn't do it,
   4) Jeff doesn't notice or is tired of doing a review.
   5) Junio picks up the buggy version.

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

    While "Experience/Logical thinking" is a very good approximation
    I find myself not thinking of all these details all the time, so sometimes
    I would fail at step 1 already.

    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.

    3) Optimally here I would have a list of all reviewers suggestions and can
    check these one by one if I really addressed all of them and not forgot one.

    4) If the list from 3) was available, it becomes much easier in a
follow up review
    to see how the author did address each issue.

    5) likewise as 4, just with less attention to the detail. (Junio
trusts Jeff that he checked
    that Stefan addressed all issues in Patch v1)

Show 11 quoted lines

>
> That being said, I suspect one could go a long way by picking out basic
> patterns from emailed responses. For example, you could imagine a system
> that makes a todo list of review comments (one comment per response to a
> quoted section) and associates them with given bits of the code (by
> seeing what's in the quoted section). That todo list can become a
> checklist when sending out the next revision, or could even be used
> interactively to see what happened to each code spot (did you fix it?
> How? In which commit?). That would help reviewers, but also would help
> submitters send out the cover letter for the next version (by reminding
> them what to mention).

I agree that would help with 3 and 4 in the case above.

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

Stefan

>
> -Peff