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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
