Re: Commitfest problems

On Tue, Dec 16, 2014 at 11:47 AM, Mark Cave-Ayland
<mark(dot)cave-ayland(at)ilande(dot)co(dot)uk> wrote:
> On 16/12/14 13:37, Claudio Freire wrote:
>
>>> For the second project, I can skim through my inbox daily picking up
>>> specific areas I work on/are interested in, hit reply to add a couple of
>>> lines of inline comments to the patch and send feedback to the
>>> author/list in just a few minutes.
>>
>> Notice though that on the second project, the review is made "in the
>> air". You didn't build, nor ran tests, you're guessing how the code
>> performs rather than seeing it perform, so the review is "light"
>> compared to the first.
>
> I think this doing developers in general a little injustice here. The
> general rule for a submitting patch to *any* project is: i) does it
> apply to the current source tree and ii) does it build and pass the
> regression tests?

That it's a policy doesn't guarantee that people submitting patches
will adhere. They could be failing to adhere even unknowingly (ie:
bitrot - there's quite some time going on from first submission to
first review).

> Maybe there is a greater incidence of this happening in PostgreSQL
> compared to other projects?

Perhaps, but it's because the reviewers take care to test things
before they hit the build farm.

That basic testing really is something for a CI tool, like the build
farm itself, not reviewers. But you cannot wait until after comitting
to let the build farm tell you something's broken: you need CI results
*during* review.

> But in any case the project has every right
> to refuse further review if these 2 simple criteria are not met.

Nobody said otherwise

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

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

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.

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

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

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

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