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