Re: Commitfest problems - Mailing list pgsql-hackers

On 16/12/14 15:42, Claudio Freire wrote:

>> Also
>> with a submission from git, you can near 100% guarantee that the author
>> has actually built and run the code before submission.
> 
> I don't see how. Forks don't have travis ci unless you add it, or am I mistaken?

Well as I mentioned in my last email, practically all developers will
rebase and run "make check" on their patched tree before submitting to
the list. Hopefully there aren't hordes of developers deliberating
creating and submitting broken patches ;)

>> You mention about performance, but again I've seen from this list that
>> good developers can generally pick up on a lot of potential regressions
>> by eye, e.g. lookup times go from O(N) to O(N^2) without having to
>> resort to building and testing the code. And by breaking into small
>> chunks people can focus their time on reviewing the areas they are good at.
> 
> I meant performance as in running, not really performance. Sorry for
> the confusion.

Okay no worries.

> I meant, you're looking at the code and guessing how it runs, but you
> don't really know. It's easy to make assumptions that are false while
> reviewing, quickly disproved by actually running tests.
> 
> A light review without ever building can fail to detect those issues.
> A heavier review patching up and building manually is too much manual
> work that could really be automated. The optimum is somewhere between
> those two extremes.

Correct. My analogy here was that people with varying abilities review
the patches at their experience level, and once low-hanging/obvious
design issues have been resolved then more experienced developers will
start to review the patch at a deeper level.

>> Occasionally sometimes people do get a patch ready for commit but it
>> fails build/test on one particular platform.
> 
> Again, pre-review CI can take care of this.

Yes - see my next reply...

>> In that case, the committer
>> simply posts the build/test failure to the list and requests that the
>> patchset be fixed ready for re-test. This is a very rare occurrence though.
> 
> It shouldn't need to reach the repo, don't you think?

When I say repo, I mean the local repo of the tree maintainer. Currently
the tree maintainer pulls each merge request into his local tree,
performs a buildfarm test and only pushes the merge upstream once this
has passed. I guess the PostgreSQL equivalent of this would be having a
"merge-pending" branch on the buildfarm rather than just a post-commit
build of git master (which we see from reports to the list periodically
fails in this way) and only pushing a set of patches when the buildfarm
comes back green.

>> Also you mentioned about "light" review but I wouldn't call it that.
> 
> I've made my fair share of light reviews (not for pg though) and can
> recognize the description. It is a light review what you described.
> 
> Surely not all reviews with inline comments are light reviews, but
> what you described was indeed a light review.
> 
>> A lot of initial comments for the second project are along the lines of
>> "this doesn't look right - won't this overflow on values >2G?" or
>> "you're assuming here that A occurs before B, whereas if you run with
>> -foo this likely won't work". And this is the area which I believe will
>> have the greatest benefit for PostgreSQL - making it easier to catch and
>> rework the obvious flaws in the patch in a timely manner before it gets
>> to the committer.
> 
> If you read carefully my reply, I'm not at all opposed to that. I'm
> just pointing out, that easier reviews need not result in better
> reviews.
> 
> Better, easier reviews are those where the trivial reviews are
> automated, as is done in the project I linked.

Yes I can definitely agree with that. See below again:

> Formatting, whether it still applies, and whether it builds and checks
> pass, all those are automatable.

I should add that QEMU provides a branch of checkpatch.pl in the source
tree which submitters are requested to run on their patchset before
submission to the list. This catches patches that don't meet the project
style guidelines including casing, braces, trailing whitespace,
tab/space issues etc. and that's before the patch is even submitted to
the list.


ATB,

Mark.