Re: Re: Patch review process

As a 10-year CVS veteran, I'll chime in:

FreeBSD uses some simple undocumented conventions that I have found to 
be remarkably effective:

CVS commit logs are mailed to committers, so there is visibility of 
who's changing what.

Each commit log is expected to have a "Reviewed by: <email>" (if 
committed by the author) or "Submitted by: <email>" (if committed by the 
reviewer) at the start.
	This single simple rule requires that every patch is signed off by 
someone else.  Anyone can be a reviewer, but people tend to make good 
choices for social reasons, since their choice is visible in the mailed 
commitlogs.  Reviewers don't sign off on bad stuff because their name 
will be on it.  Authors tend to write better code when they know they 
will have to get someone else to bless it.  This is social engineering 
at its best.

--

All CVS users should master "patch -p0":

"cvs diff -Nau" is great for generating patches to mail for review.
"patch -p0 < patchfile" in the same directory will apply the patch.
"cvs diff -Nau | patch -p0 -R" will throw away all of your uncommitted 
edits, which is great for starting over.
"cvs diff -Nau | tee filename | patchfile -p0 -R" is great for setting 
aside edits while you work on something else.

If you use patch to share code long enough, you will run into cases
where patch generates vacuous rejects because your code already includes 
some of the edits in the patch.  The fix is:
	patch -p0 < patch
	patch -p0 -R < patch
	patch -p0 < patch
.  The last patch command will magically generate only non-vacuous 
rejects.  The reason is a useful exercise for the reader.

I use this trick so often that I wrapped it in the attached script:
	gpatch -p0 < patch
will do the above even more cleanly.

--

FreeBSD also has a very nice CVS server setup:

Commit log mailings are aggregated to one-per-commit (instead of CVS's 
default one-per-directory).  [However, duplicating the FreeBSD 
aggregation is a major pain, and not worth it for small projects.]

When "cvs commit" opens an editor to enter the commit log, the default
file includes empty "Reviewed by:" etc. lines as a reminder.

--

We used to use CVS branches regularly, but found that overuse of 
branches makes the server crawl and eats disk space.  CVS's "join" 
feature has lots of odd misbehaviours.  There is no way to refer to the 
start of a CVS branch, only the end, which causes no end of trouble. 
Checking out the main branch can be orders of magnitude faster than 
checking out a branch.  Now we avoid branches wherever possible, and 
life is good.

--

To commit using a file as a CVS commit log, use
	cvs commit -m "`cat filename`"
.  This is great for reusing the code description you sent to your 
reviewer.  You did get your code reviewed, didn't you?

--

If you are a novice to CVS, check out
	http://www.myri.com/staff/glenn/CVS_cheat_sheet.pdf
, but avoid branches where possible, and master patch.

2c,
--Glenn

#! /bin/sh

# Print error and exit with error status.

die ()
{
    echo ${1+"$@"}
    exit 1
}

# Cleanup on exit

cleanup ()
{
  rm -f "$the_patch"
  rm -f "$rejects"
}
trap cleanup EXIT

# Store the patch in a file so we can use it more than once.

the_patch="/tmp/gpatch.$$"
rejects="/tmp/gpatch_rejects.$$"
cat > "$the_patch" || die "Could not record patch file"

# Apply and unapply the patch, ignoring conflicts.  This removes any
# identical changes that exist both in the patch and the files being
# patched.

patch -N    -r "$rejects" ${1+"$@"} < "$the_patch" 2>&1 > /dev/null
patch -N -R -r "$rejects" ${1+"$@"} < "$the_patch" 2>&1 > /dev/null

# Reapply the patch, generating only genuine rejects.

patch -N ${1+"$@"} < "$the_patch"