Re: Commitfest problems - Mailing list pgsql-hackers

On 14/12/14 15:57, Craig Ringer wrote:

> On 12/14/2014 10:35 PM, Mark Cave-Ayland wrote:
> 
>> If I could name just one thing that I think would improve things it
>> would be submission of patches to the list in git format-patch format.
>> Why? Because it enables two things: 1) by definition patches are
>> "small-chunked" into individually reviewable pieces and 2) subject lines
>> allow list members to filter things that aren't relevant to them.
> 
> Personally I do just that. Even though the official docs say context
> diff is preferred, I think most people now use context diff in practice,
> from 'git diff'.
> 
> I'm surprised most people don't use git format-patch .
> 
> Note that we can't just "git am" a patch from git format-patch at the
> moment, because policy is that the committer should be the Author field.
> The project would have to define a transition point where we started
> using the git Author field for the author and the separate Committer
> field, like git-am does by default. I'm not sure that'll ever actually
> happen, or that it's even desirable.

Sure, I appreciate that. The part I particularly wanted to emphasise
here was the use of a system identifier as part of the subject line,
e.g. say if there was a hypothetical patchset which sped up PostgreSQL
by 20% then you could see subjects like this:

[PATCH 1/x] lmgr: reduce lock strength for LWLock
[PATCH 2/x] wal: change lock ordering for WAL writes
[PATCH 3/x] optimiser: exit early if lock unobtainable

While these are obviously unrealistic, what I hope here is that people
with interest in these areas would take note of individual patches even
if they aren't interested in the entire patchset.

So maybe since Andreas did some recent locking work, he spots "lmgr" in
the subject and then makes a note to review that part of the patch.
Similary Heikki could spot "wal" and make a note to look at that, whilst
Tom would zero in on the optimiser changes.

The aim here is that no one person needs to sit and initially review a
complete patch before returning feedback to the developer.

>> - Patchsets must be iterative and fully bisectable, with clear comments
>> supplied in the commit message
> 
> Fully support this one.
> 
> Also in the smallest reasonable size divisions.

I should add here that the QEMU folk do tend to go to great lengths to
preserve bisectability; often intermediate compatibility APIs are
introduced early in the patchset and then removed at the very end when
the final feature is implemented.

>> - If a maintainer is happy with the whole patchset, they reply to the
>> cover letter with a "Reviewed-by" and a line stating which subtree the
>> patchset has been applied to. Maintainers add a "Signed-off-by" to all
>> patches accepted via their subtree.
> 
> Sounds a lot like the kernel workflow (though in practice it seems like
> the kernel folks tend bypass all this mailing list guff and just use
> direct pulls/pushes for lots of things).

Yes indeed - mainly from the people on the KVM module side. Again I'm
not saying that this workflow is correct for PostgreSQL, I was trying to
use this an example to explain *why* the workflow is done in this manner
and how it helps developers such as myself.

>> - Any member of the mailing list may reply/review an individual patch by
>> hitting "reply" in their mail client. Patch comments are included
>> inline. If a reviewer is happy with an individual patch, they reply to
>> the patch and add a "Reviewed-by" line; anyone can provide a review,
>> even if they are not a maintainer
> 
> This is "fun" with many mail clients that like to linewrap text bodies
> and don't permit inline replies to attached text.

Wow really? I can't say I've seen this in practice for a long time but I
defer to your experience here.

>> 6) Smaller patches are applied more often
>>
>> By breaking large patches down into smaller chunks, even if the main
>> feature itself isn't ready to be applied to the main repository then a
>> lot of the preceding patches laying down the groundwork can.
> 
> I think PostgreSQL does OK on smaller patches - at least sometimes.
> There can be a great deal of bikeshedding and endless arguments,
> back-and-forth about utility, in-tree users, etc, but no formal process
> is ever going to change that. The process of getting an uncontroversial
> patch into Pg is generally painless, if often rather slow unless a
> committer sees it and commits it immediately upon it being posted.

I agree that trivial patches do tend to get picked up reasonably
quickly. It strikes me that it may prevent patches slipping through the
list if someone were officially nominated as a trivial patch monitor,
i.e. someone for developers to "ping" if their patch has been ignored
but it sounds like this is not such an issue in practice.

>> The benefits here are that people with large out-of-tree patches 
> 
> I'm not sure we have all that many people in that category - though I'm
> a part of a team that most certainly does fit the bill.
> 
> Even the "small" patches for BDR have been challenging and often
> contentious though.
> 
>> Since as large features get
>> implemented as a series of smaller patches
> 
> A big barrier here is the "we must have in-tree users" thing, which
> makes it hard to do iterative work on a finer grained scale.
> 
> Some C-level unit tests that let us exercise small units of
> functionality might ease those complaints. Right now the closest we have
> to that is contrib/test_[blah] which is only useful for public API and
> even then quite limited.

Agreed. In the end my thoughts on how to increase I feel we can increase
patch review can be summarised as this:

- Reduce patches into small, manageable and bisectable patchsets

- Indicate in the subject line the subsystem to attract the attention of
people who have knowledge/interest in that area

- Encourage review of individual simpler patches from non-core
developers; committers can then focus on their specialist areas for the
remaining patches

- Actively aim to merge reviewed patches where possible, thus reducing
the size of outstanding patchsets requiring review (particularly for
committers) upon the next patch iteration


ATB,

Mark.