On Mon, Aug 18, 2008 at 9:00 PM, Robert Greig <[EMAIL PROTECTED]> wrote:
> 2008/8/18 Aidan Skinner <[EMAIL PROTECTED]>: > >> Not for post-commit reviews, at least no better than we currently >> have. The good thing about pre-commit reviews in this instance is that >> the onus is on the person assigned the bug to get it reviewed before >> it goes in. With post-commit reviews there's a lot less technical >> pressure to get it done, and it's harder to identify who's currently >> responsible for reviewing it. > > I don't follow - surely we just assign the jira to the reviewer when > the issue is in the "awaiting review" stage so you can immediately see > who is responsible for reviewing it? Is that not the case irrespective > of whether it is pre or post commit review? Ah, that wasn't how I was envisaging this working, I've only ever seen this implemented where you have a pool of reviewers who review specific areas (I am, for instance, unlikely to review Ruby patches), but the bug always remains assigned to the person doing the work. Otherwise you end up bouncing the thing around a lot and it's not always clear who's on the hook. >> That requires a lot more intervention and may not scale as we build >> diversity, review then commit places the responsibility for getting >> the review done on the person doing the work and their peers, which >> should be much more scalable. > > This is certainly true, the developer of the code will have an > incentive to get the review done asap. Although the counter to that > would be that someone who was otherwise snowed under may do a less > thorough review than if he could do it at a time of his choosing. But > whatever the process you can come up with scenarios like that. That's why having a pool is better, if one person is busy with other stuff then others can pick up the load. If everybody is crunching like a maniac, it's probably a sign that careful review is even more necessary - I know the quality of my patches has an inverse relationship to the amount of Irn Bru consumed. ;) >> In any case, it doesn't sound like you have fundamental objections to >> review-then-commit, just practical concerns about the tooling? > > Yes that is correct. I'm just concerned that we will try to adopt a > process that is impractical due in the main to limitations of our > toolset and it will therefore break down. That's why I suggested Review Board at the start, which is explicitly designed to cope with this. Extra tooling does seem like a bit of a burden though, I think we can probably survive with Jira+patch, although it'll be a little irritating at times - there isn't really a perfect solution available right now (git + review-board + a patch queue bot that actually does the commits after building and running the tests would be just about ideal I reckon, but would need a significant chunk of infrastructure to support it). I don't think there's a significant difference in the toolset between commit-then-review and review-then-commit, and certainly not a compelling one that outweighs the other advantages of reviewing first. - Aidan -- Apache Qpid - World Domination through Advanced Message Queueing http://cwiki.apache.org/qpid "Nine-tenths of wisdom consists in being wise in time." - Theodore Roosevelt
