After reading about both the commit-then-review [1] and review-then-commit [2] policies it seems to me that neither of these policies captures what currently happens on Artemis.
It's pretty obvious that anything involving pull-requests would not adhere to a commit-then-review scheme since the pull-request itself ensures some kind of review is happening between the work and the final commit. However, the review-then-commit policy described on the Apache website requires formal consensus approval [3]. What currently happens on Artemis is something between these two options which I think combines most of the benefits of both. The down-side to commit-then-review is that problems can enter the code-base which have to be addressed retroactively. I'm thinking here mainly of checkstyle problems, license header problems (commonly seen when new tests are added), and the occasional large change that simply has a bad design (this has happened a few times on Artemis in the last few months that I can remember off the top of my head). The cost of fixing problems is always lower on the front-end so fixing problems sooner rather than later is a significant win. The process that Artemis follows with the automated and verifiable PR builds as well as a human review of the code (but *not* "consensus approval") mitigates these risks quite well in my experience. The down-size to the review-then-commit is that "consensus approval" is slow. As noted previously, Artemis doesn't employ "consensus approval." Typically just one other committer reviews the PR and merges it. Sometimes we makes notes in the PR regarding who we think should review it based on the expertise of another committer. Overall this process is efficient enough not to bog down but strict enough to catch almost all the low-hanging problems. I'm certainly not married to this process, and I'm willing to change. Yet I commend it to you nevertheless. Justin [1] http://www.apache.org/foundation/glossary.html#CommitThenReview [2] http://www.apache.org/foundation/glossary.html#ReviewThenCommit [3] http://www.apache.org/foundation/glossary.html#ConsensusApproval ----- Original Message ----- From: "Clebert Suconic" <clebert.suco...@gmail.com> To: dev@activemq.apache.org Sent: Tuesday, June 9, 2015 11:24:28 AM Subject: Re: Git workflow for committers >> >> You should be responsible for testing your commits before they enter >> the build then. Please keep an eye on the build you created. > > Agreed. Please use the -Pdev profile before you push, as it will validate checkstyle
