On Mon, Jun 27, 2016 at 12:17 PM, Jeff King <peff@xxxxxxxx> wrote:
> 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)
>
> 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
--
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