Re: Commitfest problems

On 16/12/14 10:49, Marko Tiikkaja wrote:

> 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.

I've already covered this earlier in the thread so I won't go into too
much detail, but the overall "flow" of the patchset is described by the
cover letter (please feel free to look at the example link I posted).

In terms of individual patches within a patchset then if the combination
of the cover letter and individual commit message don't give you enough
context then the developer needs to fix this; either the patchset has
been split at a nonsensical place or either one or other of the cover
letter and/or commit message need to be revised.

> 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".

Sure, there is always scope for doing that which is what splitting
patches and constant review encourages. In terms of the current
commitfest system, the process for review is clearly documented and as
I've pointed out in my response to David, extremely heavyweight in
comparison.


ATB,

Mark.