I have mixed reactions to what's been said here, although some points are certainly good. I don't think the "size" of a change is really the important factor for determining both how easy it is to review, and how easy it would be to branch and merge; a much better metric is "complexity", how tightly it integrates with the rest of the codebase, and how many 'external' codepoints must be updated to complete the change.
ResourceLoader, the Installer, the Metadata, are all examples of changes which may be quite large, but which are quite isolated within the codebase and do not really sprawl across it in a particularly complex way. the Blocking and Skin rewrites and (particularly) RequestContext are examples of the opposite, changes which are *not* actually particularly large in terms of number-of-lines (the blocking rewrite rewrote four files, RequestContext two, compared to about forty for ResourceLoader and 25 for Installer). Having code together in a few files totally-rewritten or added afresh is both easier to review, *and* easier to branch and merge. A branch merge of a complicated change (like the Login rewrite I did in summer 2009, which bounced three times and is now bitrotted into oblivion) looks incredibly scary and is essentially impossible to review, you have to take it on faith assuming that the review *of the code while it was in the branch* was good enough. Yes, it will contain iterated fixes for simple errors, but the sequence being compressed into one revision is a double-edged sword: rewrites are inevitably done progressively, and the easiest path for the reviewer to take is the same one as the coder. When everything is compressed into one branch-merge-revision, there is no context from which to reconstruct that path. So, and I think most people are saying the same thing, the review will still have to happen in the branch, on the individual revisions, with all the false-starts and follow-ups, and merged in one (probably very messy) branch merge. But making complicated changes in a branch doesn't make them any less complicated, or any easier to review in the branch. Complicated changes are difficult *because* of their extensive interaction with other code elements, not because of a particularly grand scope. There is no such thing as a "small incremental change" when changing one interface in the actual subject of the rewrite requires changing fifty callers, the other interfaces on the target also called by the fifty callers, and the 200 other places that call *them*. The list of actual *changes* to the blocking code is fairly minimal: internalise variables, rewrite the way blocks are constructed from a target or targets, separate and modernise the frontend interfaces for blocking, unblocking and listing blocks. It is complicated, difficult to review and with a huge number of follow-up commits, because of the large number of *other* places that blocks are referenced and manipulated. Those other places still exist in a branch, are still just as difficult to *find* in a branch, and still just as time-consuming to fix and review in a branch. Thus I don't agree that doing rewrites like the Blocking stuff in a branch would have made it any easier, or any less time-consuming, to review. On the other hand, you make an excellent point about *discussion* of changes before implementation; that needs to happen, and it needs to be thorough. I again don't think that a branch is particularly useful for that purpose because to 'discuss' something in code is to go through exactly the same process of writing, rewriting and discarding as if you just went ahead and did it. An RfC, on the other hand, allows (and requires) a spec to be developed in natural language, with great saving of time and effort. *If* it gets the participation it deserves. The people who would be spending their time calling "crazy" on checked-in code *must* take the attitude that participating in RfCs *is* the alternative and is time well spent, that they are not making problems magically go away, only moving them to another venue where they can be nipped in the bud. There are only three options: those two, and dismissing the proposal altogether *regardless of its merits*. We currently have ten RfCs listed; three are over a year old, five are between three and six months, and two are younger than a month. Five of them, including *all* of the old ones, have no substantive contributions by *anyone* apart from the author. Two more have exactly one other contributor, and two (Math and Configuration) are swarming with commenters. This is not a healthy system. It could *become* a healthy system, but only if *everyone* is prepared to make it an integral part of their development work, not just the people who are being diligent enough to be, or being pressured into, writing proposals. I don't think that anyone really thinks RFC is the magic bullet, but it's even less than what you think it is: it's *another* broken system that's in need of a lot of TLC and structural changes to get working again. We have plenty enough of those on our plate already. You could say quite legitimately that it's part of the bigger problem and the fix will fit naturally into the bigger solution. But now you're getting dangerously close to the non sequitur than when nothing is broken, everything will be fixed. --HM PS: sorry for TLDR-ness... :-( "Chad" <[email protected]> wrote in message news:CADn73rP=5uiaa878mhazjdjdcitmhyvg4+nr_0ensdyxjsz...@mail.gmail.com... > On Tue, Jul 26, 2011 at 2:08 PM, Daniel Friesen > <[email protected]> wrote: >> RequestContext WAS an RFC. >> No one commented on it... >> > > An e-mail to the list announcing the RfC would've been nice ;-) Also > waiting for more than 1 other person to edit it would've been nice too. > If you're not getting responses: pester people for them. > >> When it was originally committed it was a small change. It just grew as >> people improved it. >> > > It started out small enough, yes...but it quickly grew to permeate a good > chunk of core code. And then the architecture was changed halfway through, > leading to the crap with __get(). Having more eyes on the RfC would've > avoided this, IMHO. > >> Frankly if RequestContext and features of similar size had been >> committed to a branch instead of core, they would have bit-rotted there >> and never made it into core. Not to mention the relative size compared >> to things like ResourceLoader and Installer makes the negatives of svn >> branches relatively worse. Dozens of med-small changes getting shoved >> into branches and atempted to be merged into core? >> > > Medium to small changes should still be done in trunk, and at no point in > my original e-mail did I say otherwise. What I'm asking for here is for > large > refactors/rearchitecturings to be done in branches. RequestContext may > not have *started out* as a large refactor, but the architecture > implications > are huge in the long run. > >> I'd love to do work in branches, have everyone actually try the branches >> out and add their improvements there, then get them reviewed into trunk. >> But that workflow... that's really git, not this svn mess we have now. >> > > Let's please not turn this into a Git v. SVN debate again. Git is cool, > and > will perhaps solve some of our problems, but it's not a magic bullet to > review/merging. I believe first and foremost the debt of currently > unreviewed > code should be near-zero before we even consider a migration. It would be > very easy to work ourselves into a very similar situation using Git (tons > of > unreviewed pull requests--waiting in their branches for some love), and > anyone who thinks we couldn't get backlogged in Git is kidding themselves. > >> Even if we kept this in the environment of svn and tried to use RFCs... >> people would have to actually discuss those RFCs... looking over the >> RFCs we have it looks like only 10% of them even get anyone discussing >> them. The rest just bit rot... So we're supposed to move from having a >> volunteer willing to code a feature and putting it into trunk, to having >> them putting up an RFC or putting it into a branch, and letting the code >> bit rot or RFC just sit there forever, and neither ever get into core? >> > > I would rather people take the time to do it right than have a constantly > unstable trunk that makes backporting damn near impossible. For example, > you landing your Skin overhauls a day or two after branching REL1_17 was > a poor decision on your part...and made backporting stuff and releasing > 1.17 > a huge pain. > > This kind of comes back to the idea of code freezes (boo, I know...). I > don't > want to ever completely freeze trunk; that discourages contributions as > we've > discussed before. I'm just asking for a little bit of common sense from > all > around so we can keep trunk stable and push for more regular deploys and > releases. > > -Chad _______________________________________________ Wikitech-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikitech-l
