On Thu, Oct 2, 2008 at 3:00 PM, Roan Kattouw <roan.kattouw(a)home.nl> wrote:
I'd like to suggest a system in which different
people are responsible
for reviewing commits to different parts of the code, and mark revisions
as OK when they've reviewed them. The tag history I suggested on the
TODO list would be useful there.
I think it would be a must to keep track of who's marking stuff as
reviewed. As things stand, we probably want every commit to be
reviewed by either Tim or Brion before a scap. We might also want
certain classes of commits to be reviewed by other people, like you
reviewing API changes, but in that case either Brion or Tim will still
want to at least glance at them.
This is where pull-only DVCSes are pretty neat. Intrinsic to the
process is that something's review status can be inferred from where
it is: patches in anyone's tree will have been reviewed by them, or
someone they trust. So we could all maintain our own trees and just
post patches for Brion/Tim to review, and accept or not. You could
also do something similar with a centralized VCS, the way Mozilla did
while they still used SVN -- only allow reviewers to commit, and
require everyone else (even established patch authors) to submit
patches. But it's more of a headache with those, for a variety of
reasons.
So actually, I think it would be good to think about whether we want
to continue our current system of "commit then review" altogether,
spiffy new tools or not, or whether we want to move to a "review then
commit" system like some other projects have had for a long time
(e.g., Mozilla or the Linux kernel). Major advantages of "review then
commit":
1) No big barrier of commit access. Everyone has to submit patches
just the same, with no direct commit access to a central repo, so
there *has* to be a functional patch-review system. We've always been
terrible about reviewing third-party patches, but that couldn't happen
if practically all regular developers had to use the same
patch-submission mechanism.
2) With "commit then review", a reviewer who spots a minor problem and
who wants to fix it has to go to the trouble of committing a fix, or
reverting the commit. With "review then commit", a reviewer who spots
a problem just has to say so, and gets to *avoid* bothering to make a
commit. This should encourage more meticulous coding standards, which
is a good thing.
There are also more minor advantages, like trunk being more likely to
actually work; fewer reverted commits cluttering up the logs and
mailing lists; the ability to easily put off review of big features
without having to deal with the mess of an SVN branch; and the ability
to put off review of unimportant changes when there's an important one
that needs to be cherry-picked and made live, on a per-commit basis
rather than a per-file basis (which is what caused the file loss
lately, wasn't it?).
Of course, if it were decided that there were some people who were
trusted to review certain parts of the code, say Roan with the API
code, in a DVCS, you could have that person maintain their own tree,
and have people submit patches for that tree to them. The core
maintainers would then regularly pull from the subsystem tree to their
private box, briefly review all the commits, possibly undo any bad
ones, and then push the good ones to the main tree. This could be
applied to more people too, but preferably not too many, to avoid
undoing the positive effects of (1).
On the other hand, I like git a lot more than Mercurial, so I'm all
for holding off on the DVCS craze until git works acceptably on
Windows. :P Apparently it sort of works now, with a GUI and
everything, just not quite as mature:
http://kylecordes.com/2008/04/30/git-windows-go/
And yeah, so I realize that I'm raining on weeks of hard work by
saying we should chuck it out and go with a totally different system.
So sue me. :) Moving to a DVCS would be a much bigger change anyway.
Since only you and Tim are coders right now, could you
mark a few other
devs (like me) as coders so this virus actually starts spreading?
Bureaucrats can also add people. I'm assuming that people who have
had commit access for a reasonably long time and have shown they know
how to use it should be marked coders, but I'm still really uncertain
given that nobody's actually said what the various statuses are
supposed to actually mean. Are people going to actually scap on the
basis of nothing other than the fact that every commit is marked
ok/resolved? If so, it's probably a bad idea for people other than
Tim or Brion to add those markings, at least on a regular basis,
unless we really want that.