Review workflow

Daniel Silverstone dsilvers at netsurf-browser.org
Tue Jun 23 11:43:13 BST 2009


Hiya,

This mail is intended to provide a very very basic hash-up of my review
workflow.

Firstly, there are three tools in SVN in the tools project:

svn://svn.netsurf-browser.org/trunk/tools

In the svn/ directory in there is netsurf-review-{diff{,-list},merge}

To use the tools effectively, check these out somewhere and ensure that
either that dir is on the PATH, or that you symlink the tools into
somewhere which is. (I use symlinks in ~/bin)

The three tools are:

netsurf-review-merge

This tool takes a branch name (of the form person/branch) checks that
the CWD has no local changes, and then determines the point at which the
branch was made and merges that onto the current branch. Don't use this
if you're using svnmerge -- use svnmerge instead. This tool is only
meant for trivial feature branches, not for long-term branches.

netsurf-review-diff

This tool takes the status and formats up a review diff. Review diffs
list each new file, each deleted file and each changed file in different
parts, it also provides a diffstat and if there's a precis for the
branch in /tmp/precis then it includes that in the output.

You can change to a trunk dir, merge your branch, and then use this tool
to look at your change in the same way that a reviewer will look.

netsurf-review-diff-list

This tool uses netsurf-review-diff, offers to allow you to write a
precis if you want (and haven't already) and then attempts to submit it
to the netsurf-dev mailing list with a subject line composed of 'Review:
' and then anything you put on the command line.

It assumes that your local username can be stuffed onto
@netsurf-browser.org to use as a "from" address. If it can't, set
NETSURF_USERNAME to your @netsurf-browser.org local-part. If you don't
have such a local part then you shouldn't be issuing review diffs.
It also assumes that your local gecos is good. If that isn't good, set
NETSURF_REALNAME to your real name (no brackets, no anglebrackets, etc)

You can pass --test as the first argument to netsurf-review-diff-list to
get it to put the mail it would have submitted on stdout instead.

Note that netsurf-review-diff-list relies on a local MTA which
functions. If all else fails, you ought to be able to use it on the
machine which hosts netsurf-browser.org if you have a login.

My workflow
^^^^^^^^^^^

1. Prepare a pristine trunk
2. Merge the branch into this trunk

If there are conflicts which aren't trivial, reject the merge at this
point.

3. Check that the branch does as advertised. i.e. verify it builds,
passes tests, tests any new functionality if appropriate. Ensure it
builds both in release and in debug if appropriate.

If it fails any of this, reject the merge at this point.

4. Run netsurf-review-diff | less and have a very cursory glance over
the diff to ensure there's nothing *huge* like comments with rude words
in, etc.

If there's anything huge, reject the merge at this point.

5. Run netsurf-review-diff-list and give it a suitable subject and
precis.

6. On-list, in your MUA, perform your review as a reply. This ensures
that the review diff is quoted properly etc.


Once the branch is in a state that it's worth merging...

a. Follow 1/2 above

b. Update a version number if needed.

c. Do a final build/run-test in debug and release

d. commit with a message along the lines of: 

"merge person/branch rBLAH:OTHER: Feature short desc. r=myusername"

The rfoo:bar is optional if it's a single merge and then the branch is
considered dead. the r=myusername means that we can see if the reviewer
differs from the person doing the merge. E.g. it's quite possible for me
to make a branch, issue a review to the list, jmb could then review it
for me, and then I can commit it r=jmb. Providing the review is done and
a reviewer says "OK" then there's no requirement for the reviewer to be
the person who commits it, providing all of 1 to 6 above is followed,
and then a to d.

I hope this helps with other people who are thinking of doing reviews in
a similar way.

D.

-- 
Daniel Silverstone                       http://www.netsurf-browser.org/
PGP mail accepted and encouraged.            Key Id: 2BC8 4016 2068 7895





More information about the netsurf-dev mailing list