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" <innocentkiller(a)gmail.com> wrote in message
news:CADn73rP=5Uiaa878mHaZjdjdciTmhYvG4+nR_0EnsdYxjsZ3nA@mail.gmail.com...
On Tue, Jul 26, 2011 at 2:08 PM, Daniel Friesen
<lists(a)nadir-seen-fire.com> 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