Open Stack

On Fri, 2013-11-08 at 09:32 +1300, Robert Collins wrote:
> On 7 November 2013 13:15, Day, Phil <philip.day at hp.com> wrote:
> >
> >
> 
> >>
> >> Core reviewers look for the /comments/ from people, not just the votes. A
> >> +1 from someone that isn't core is meaningless unless they are known to be
> >> a thoughtful code reviewer. A -1 with no comment is also bad, because it
> >> doesn't help the reviewee get whatever the issue is fixed.
> >>
> >> It's very much not OK to spend an hour reviewing something and then +1
> >> with no comment: if I, and I think any +2er across the project see a patch that
> >> needs an hour of review, with a commentless +1, we'll likely discount the +1
> >> as being meaningful.
> >>
> >
> > I don't really get what you're saying here Rob.   It seems to me an almost inevitable
> > part of the review process that useful comments are going to be mostly negative.
> > If someone has invested that amount of effort because the patch is complex, or
> > It took then a while to work their way back into that part of the systems, etc, but
> > having given the code careful consideration they decide it's good - do you want
> > comments in there saying "I really like your code", "Well done on fixing such a
> > complex problem" or some such ?
> 
> Negative comments are fine: I was saying that statistically, having an
> hour-long patch (which BTW puts it waaay past the diminishing returns
> on patch size tradeoff) to review and then having nothing at all to
> say about it is super suspect. Sure, having nothing to say on a 10
> line patch - hit a +1, move on. But something that takes an hour to
> review? I'd expect at minimum the thing to prompt some suggestions
> *even while it gets a +1*.
> 
> > I just don't see how you can use a lack or presence of positive feedback in a +1 as
> > any sort of indication on the quality of that +1.   Unless you start asking reviewers
> > to précis the change in their own words to show that they understood it I don't
> > really see how additional positive comments help in most cases.   Perhaps if you
> > have some specific examples of this it would help to clarify
> 
> It might even be negative feedback. Like - "this patch is correct but
> the fact it had to be done as one patch shows our code structure here
> is wonky. If you could follow up with something to let us avoid future
> mammoth patches, that would be awesome."
> 
> Again, I'm talking about hour long reviews, which is the example Radomir gave.
> 
> Another way of presenting what I'm saying is this: Code reviews are a
> dialogue. How often in a discussion with a live human would you have
> no feedback at all, if they were reading a speech to you. You might go
> 'that was a great speech' (+2) and *still* have something to add. As
> an observer given a very long speech, and an audience, I'd suspect
> folk went to sleep if they didn't have commentary :)

Totally agree with you here. I almost completely discount +1s with no
comments on a large patch.

I make a habit of leaving comments in reviews - positive, negative,
neutral, whatever. If I have something to say which might be useful to
the author, other reviewers, my future self, whatever ... then I'll say
it.

e.g. if I spend 10 minutes looking at one part of a patch, ultimately
convincing myself that there really is no better approach and the author
has made the right tradeoffs ... then I'll say it. I'll briefly describe
the tradeoffs, the other options that I guess the author considered and
discounted.

I sometimes feel guilty about this because I know patch authors just
want their +2 and often don't want to read through my verbiage ... but,
as you say, this is a dialogue and the dialogue can yield some
interesting thoughts and ideas.

Mark.