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?
Maybe there is a greater incidence of this happening in PostgreSQL
compared to other projects? But in any case the project has every right
to refuse further review if these 2 simple criteria are not met. Also
with a submission from git, you can near 100% guarantee that the author
has actually built and run the code before submission. I have seen
occasions where a patch has been submitted, and a committer will send a
quick email along the lines of "The patch looks good, but I've just
applied a patch that will conflict with this, so please rebase and
resubmit".
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.
Occasionally sometimes people do get a patch ready for commit but it
fails build/test on one particular platform. 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.
Also you mentioned about "light" review but I wouldn't call it that. I
see it more as with each iteration the magnifying glass used to look at
the code gets bigger and bigger.
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.
ATB,
Mark.
