On 06/20/2014 10:42 AM, Adrian Crum wrote:
No one is lowering the bar. The problem is, you still don't understand
how an open source community works.
This is wrong. There is no hard-set one-workflow-to-bind-them
methodology here. Some people seem to think there is.
The Review-Commit workflow people sometimes want all stuff reviewed.
While that can help find many issues ahead of time, it will never be
able to find and implement all corner cases. That can generally only
happen when the isolated code is pushed into the wider community. At
some point, you just need to push your reviewed code to someone else,
and continue to fix/improve afterwards.
The Commit-Review workflow can sometimes cause severe breakage for other
users. If not-ready code is pushed into trunk, and it causes feature
breakage, or compilation problems, or corruption, the community at large
might not be able to help, as the newly pushed code is unknown, and
it'll take a while to come up to speed.
Speaking about one earlier point: Committing code *early* does not lower
the bar for the committed code. Code is code. It is, or it isn't. When
something is committed has no bearing on the quality of the commit.
That quality is a separate item from the time of the commit.
Let me give you an example:
I helped design the custom XML file format OFBiz uses for UI labels
(https://issues.apache.org/jira/browse/OFBIZ-1442). There were people
in the community who disagreed with the design, but it was committed
anyway.
Even here, you didn't do this initial work *in trunk*. You thought
about the idea for a while, tried some things, got an initial set, then
eventually committed to trunk.
For the next few months, there were a lot of commits to fix bugs that
cropped up after the initial commit. Scott and David helped with the
bug fixes and improvements.
Eventually, the new feature was working well - but there were still
hundreds of properties files that needed to be converted to the new
format. That was done over a period of several years by many different
people.
Another concrete example.
Currently, I'm working on fixes to the entityengine crypt system.
2 years ago, I implemented key-encrypting-key support(read up on the
credit card number security docs). I worked on the code in isolation,
on behalf of a customer. Once it worked there, I then directly pushed
to ofbiz trunk. This is the Commit-Review workflow.
No review happened. None. If such review had happened, it might have
discovered that direct lookup of encrypted fields would have been broken
by my new code(a random salt is prepended to each encrypted value, which
prevents lookups).
Even if that review *had* happened, after the commit, or even *before*
the commit, and I didn't add that random salt, it *still* wouldn't have
fixed the direct lookup problem. This was due to direct-lookup being
broken as far back as release4.0, and probably even all the way back to
2005(!). This points to a general lack of review, at *all* levels.
It's been my experience, that the Review-Commit workflow will get you
some small about of early reviews; those people who are keenly
interested in your new idea will possibly wake up and comment on it.
However, the Commit-Review can get you *more* reviewers; in addition to
those who are interested in the new stuff, you'll find completely random
people might speak up, and offer some un-thought-of problem. Even
simple things, like a null pointer check happening after a dereference
can be found this way.
