Re: Commitfest problems

On 12/16/14 11:26 AM, Mark Cave-Ayland wrote:
> On 15/12/14 19:27, Robert Haas wrote:
>> So, there are certainly some large patches that do that, and they
>> typically require a very senior reviewer.  But there are lots of small
>> patches too, touching little enough that you can learn enough to give
>> them a decent review even if you've never looked at that before.  I
>> find myself in that situation as a reviewer and committer pretty
>> regularly; being a committer doesn't magically make you an expert on
>> the entire code base.  You can (and I do) focus your effort on the
>> things you know best, but you have to step outside your comfort zone
>> sometimes, or you never learn anything new.
>
> Right. Which is why I'm advocating the approach of splitting patches in
> relevant chunks so that experts in those areas can review them in
> parallel.

I don't see how splitting patches up would help with that.  I often look 
at only the parts of patches that touch the things I've worked with 
before.  And in doing that, I've found that having the context there is 
absolutely crucial almost every single time, since I commonly ask myself 
"Why do we need to do this to implement feature X?", and only looking at 
the rest of the complete patch (or patch set, however you want to think 
about it) reveals that.

Of course, me looking at parts of patches, finding nothing questionable 
and not sending an email about my findings (or lack thereof) hardly 
counts as "review", so somebody else still has to review the actual 
patch as a whole.  Nor do I get any credit for doing any of that, which 
might be a show-stopper for someone else.  But I think that's just 
because I'm not doing it correctly.  I don't see why someone couldn't 
comment on a patch saying "I've reviewed the grammar changes, and they 
look good to me".


.marko