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

[Egbert Eich]

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.