CVS access policy, branching/tagging, code review, etc.

[Jim Gettys]

I don't consider my own experience in dealing with CVS based
release cycles enough to have much opinion to add to this
discussion.

I do, for future thought, want to point out the policies
that Mozilla uses.  I don't think they necessarily apply to 
our circumstances at the moment, but when we succeed in forming
a much larger developer community, we may want to consider somewhat
similar policies, at least in parts of the overall tree.

See:
http://www.mozilla.org/hacking/reviewers.html

At least some of the rules and tips section should immediately be
useful.
                             - Jim


On Tue, 2004-03-02 at 13:44, Egbert Eich wrote:
> Keith Packard writes:
>  > 
>  > I'd like someone to review a couple of sample patches before granting 
>  > commit access; that way we can check that the general coding conventions 
>  > are being followed; that's hard to check any other way.
> 
> That's even stricter than my proposal. I'd just expect a rough idea
> on what the person has planned to do. Code review may be good as
> it gives an indication if the person who asks for access is sufficiently
> 'sane' to grant him access. People who write decend code generally know
> what they do. Others may screw up the cvs as they screw up coding.
> 
>  > 
>  > > - what the official do's and dont's are in the tree
>  > 
>  > There will probably be a lot of these, here's some suggestions:
>  > 
>  >  *	Do new scary work on a branch
>  > 
>  >  *	But, integrate scary work into HEAD with #ifdefs to avoid divergence
>  > 
>  >  *	Announce release schedules and limit HEAD changes after feature 
>  > 	freeze.  Permit branch changes.
> 
> When the feature freeze is in effect the release version should 
> probably be branched off.
> 
>  > 
>  >  *	After RC1, limit HEAD changes to fixes for major bugs.
>  > 
>  >  *	Separate code cleanup from functional changes.  Mark cleanup 
>  > 	patches clearly.  For major cleanups, get peer review before 
>  > 	committing.
> 
> There has been the suggestion to generate a bugzilla entry for every
> commit and limit the commit to a single issue. This requires a lot
> of discipline however it helps to track down problems even after
> a long time.
> 
>  > 
>  >  *	Get Bonsai and Tinderbox working together; require commits that
>  > 	break the build to be fixed or get backed out within a short time
>  > 	frame (~days, not ~weeks)
>  > 
>  >  *	Don't commit huge changes and then go on vacation (see above).
>  > 
>  >  *	Tag and release on a regular basis.
>  > 
>  > > - what happens if someone commits something non-trivial without 
>  > >   proper code review, which others believe should not have been 
>  > >   committed, or which horribly breaks the tree, etc.
>  > 
>  >  *	Any changes for HEAD which don't have rough concensus should be reviewed > 	and voted on.  I don't know who should vote.  I don't know if the
>  > 	vote should be a simple majority, or if we should require rough 
>  > 	concensus and a czar that accepts or rejects the patch.  If a 
>  > 	change is committed to HEAD which generates more heat than light, it
>  > 	should be reverted until the issue is resolved.
> 
> I don't think a majority principle here is appropriate.
> A rough consensus among the people who are involved in similar
> areas should suffice. One could turn it around: if there is no
> strong objection it can go in. If there is it should be discussed.
> 
>  > 
>  >  *	Any changes to HEAD which break the build should be reverted unless 
>  > 	fixed in a timely fashion
>  > 
>  >  *	Changes to HEAD which generate negative rough concensus should be 
>  > 	reverted by the committer and discussed.
> 
> It's preferrable to do it the other way around: get positive consensus
> before things are merged into head.
> 
>  > 
>  >  *	Changes to branches are pretty much anything goes, unless they
>  > 	conflict with the law or PSU policy (i.e. don't append MP3s 
>  > 	to CVS files).
> 
> Creation of directories should not happen without giving this some
> thought. It may have negative side effects on other things.
> 
>  > 
>  > > - A specific guideline for the naming of CVS tags, branches, etc.
>  > 
>  > always tricky.  For development branches, I'd like to see them tagged with 
>  > userids of the person doing the branch along with a date in ISO standard 
>  > format (yyyy-mm-dd).  An additional tag indicating the scope of the work 
>  > might be nice.  So, perhaps
>  > 	
>  > 	keithp-2004-03-02-hacking-damage
>  > 
>  > Using dates without dashes makes them really hard to read for some of us.  
>  > Mixing hyphens and underscores makes them prone to typos.
> 
> I don't see the benefit of dates and names. Certainly project names
> should have precedence. They are not necessarily attached to a single
> person but to a group of people.
> If someone needs a 'personal branch' to play around this should
> be one branch per person. There is generally no need to branch off
> every time somebody starts something new.
> 
>  > 
>  > I don't know whether such branches should also be tagged with the base 
>  > revision number, I'm thinking that that won't be necessary.
>  > 
>  > For tags along the main branch that indicate release points, let's follow
>  > a pretty simple convention.  I suggest that there are two kinds of tags, 
>  > there are 'module' releases and 'product' releases, and 'products' are a 
>  > collection of 'modules'.  When releasing a 'module',  tag with
>  > 
>  > 	modulename-major.minor.revision
>  > 
>  > Similarly, when releasing a product, tag with
>  > 
>  > 	productname-major.minor.revision
>  > 
>  > Tags along a branch should be prefixed by the branch name.  yes, this will 
>  > make them rather long.  I hope that will encourage people to get branches 
>  > merged back to head more often.
>  > 
>  > > - What type of checkins are permitted into particular branches
>  > 
>  > I'd like to see most 'mainline' development occur right on HEAD; otherwise 
>  > one spends a lot of time merging stuff back.  Unlike DRI, I don't see a 
>  > huge number of module interface changes which would require careful 
>  > coordination, so I don't want to force people to make branches just to fix 
>  > minor bugs or implement minor new functionality.
> 
> I'd prefer if 
> a. not everyone is allowed to work in HEAD right away.
> b. work at central pieces in the HEAD is limited to minor
>    changes and bug fixes.
> 
>  > However, I see the release process as three phases:
>  > 
>  >  1)	liquid
>  > 
>  > 	Add code to HEAD as long as it is has rough concensus to be
>  > 	"good".
>  > 
>  > 	Speculative code, or 
>  > 
>  >  2)	slush
>  > 
>  > 	Add code to HEAD as long as it addresses planned release features
>  > 	or bug fixes.
>  > 
>  >  3)	ice
>  > 
>  > 	Add code to HEAD as long as it fixes major bugs.
>  > 
>  > > - What happens if someone consistently violates the accepted 
>  > >   written or defacto policies and shows disregard for the 
>  > >   established guidelines repeatedly.
>  > 
>  > Failure to roughly follow the policy should result in probationary access 
>  > to the tree where patches would be reviewed by another committer before 
>  > being applied.  This would last for a while (two weeks?) and for a number 
>  > of patches (3?).  No mechanical enforcement of this should be needed, but 
>  > violation of probation should result in revocation of CVS access.
> 
> Do you think strict procedures like these are necessary?
> Peer pressure should solve much of this. Only people who are
> immune to peer pressure will get their access revoked.
> 
>  > 
>  > Note that CVS access is completely separate from freedesktop.org account
>  > access; we haven't established a policy for that and perhaps it is time to
>  > consider doing so.  About the only thing that would cause people to lose
>  > their fd.o accounts is illegal or obviously inappropriate activities
>  > involving the fd.o machine or the PSU network.
>  > 
>  > > - What types of fixes/checkins are considered ok to do without 
>  > >   review (and on which branches, and during what phases of 
>  > >   development).
>  > 
>  > On branches, anything goes.  Merging branches back to HEAD should follow 
>  > the review requirements as if the merge was one giant commit.
>  > 
>  > On HEAD, no review is required for:
>  > 
>  >  1)	Bug fixes that are "obvious to a skilled practitioner"
>  > 
>  >  2)	Changes previously agreed upon.  I'd like to see this in the form
>  > 	of a mail message laying out the scope of the work, large changes
>  > 	should be documented in the wiki and referenced from the message.
>  > 	Lack of dispute should be taken as concensus.
> 
> Right.
> 
>  > 
>  > > - What types of changes ALWAYS require review either via bugzilla 
>  > >   filing or email list posting and having one or more people 
>  > >   "ACK" (acknowledge) the fix is acceptable for the tree, or 
>  > >   "NACK" it indicating it is unacceptable, and providing a reason 
>  > >   why.
>  > 
>  >  1)	Bug fixes which are non-obvious, especially in really stale code.
>  > 
>  >  2)	ABI/API changes.
> 
> These should also require documentation.
> 
>  > 
>  >  3)	Changes which don't generate rough concensus when they zip by.
>  > 
>  > > - What types of changes are good ideas, or worth exploration in 
>  > >   the future, but are not acceptable changes for the current 
>  > >   goals of releasing a stable release as soon as possible, such 
>  > >   as Phase2/Phase3 type of work, source code beautification, 
>  > >   lindent, etc.
>  > 
>  > The monolithic tree should be considered to already be 'ice', and 
>  > receiving only major bug fixes.
>  > 
> 
> Egbert.
> 
> _______________________________________________
> Release-wranglers mailing list
> Release-wranglers@freedesktop.org
> http://freedesktop.org/mailman/listinfo/release-wranglers
-- 
Jim Gettys <Jim.Gettys@hp.com>
HP Labs, Cambridge Research Laboratory