Also, with some coordination, branches could be merged at a time when: * Reviewers will looking at it and testing it *right* after the merge (in addition to any branch review) * The author is around to make fixes as it gets final review
People should try to be available after any large changes (from a branch or from their local code). However, it may be the case that no one has the time to specifically focus on reviewing a certain change set right after commit. Maybe, for example, changes will only get seriously looked at a month after the commit. If the author was available in the weeks after the commit, but not when it gets serious review, then we still have a problem. This is also were some advance notification and coordination could help (especially looking at the examples Chad gave).
^demon wrote:
All,
While spending the past few days/weeks in CodeReview, it has become abundantly clear to me that we absolutely must get away from this idea of doing huge refactorings in our working copies and landing them in trunk without any warning. The examples I'm going to use here are the RequestContext, Action and Blocking refactors.
We've gotten into a very bad habit recently of doing a whole lot of work in secret in our respective working copies, and then pushing to trunk without first talking to the community to discuss our plans. This is a bad idea for a bunch of reasons.
Firstly, it skips the community feedback process until after your code is already in trunk. By skipping this process--whether it's a formal RfC, or just chatting with your peers on IRC--you miss out on the chance to get valuable feedback on your architectural decisions before they land in trunk. Once code has landed in trunk it is almost always easier to follow up and continue to "fix" the code that should've been fully spec'd out before checkin.
Also, the community *must* have the chance to call you crazy and say "don't check that in, please." Going back to my examples of Actions, had the community been consulted first I would've raised objections about the decisions made with Actions (I think they should be moved to special pages and the old action urls made to redirect for back-compat...rather than solidifying the old and crappy action interface with a new coat of paint). Looking at RequestContexts, had we talked about this in an RfC first...we probably could've skipped the whole member variable vs. accessor debate and the several months of minor cleanups that's entailed (__get() magic is evil, IMHO)
Secondly, this increases the load on reviewers, myself included. When you land a huge commit in trunk (like the Block rewrite), it takes *forever* to review the original commit + half a dozen or more followups. This drains reviewer time and leads to longer release cycles. I think I speak for everyone when I say this is bad. Small incremental changes are infinitely easier to review than large batch changes.
If you need to make huge changes: do them in a branch. It's what I did with the installer and maintenance rewrites, what Roan and Trevor did with ResourceLoader and what Brian Wolff did with his metadata improvements. Of course after landing your branches in trunk there will inevitably be some cleanup required, but it keeps trunk more stable until the branch merge and makes it easier to back out if we decide to scrap the feature/rewrite.
I know SVN branches suck. But the alternative is having a constantly unstable trunk due to alpha code that was committed haphazardly. Nobody wins in that scenario.
So please...I beg everyone. Discuss your changes first. It doesn't have to be formal (although formal spec'ing is always useful too!), but even having a second set of eyes to glance over your ideas before committing never hurts anyone.
-Chad
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l