[Twisted-Python] Re: [Twisted-commits] r17325 - merge sob-491, fixes #491

[Andrew Bennetts]

On Wed, Jun 21, 2006 at 11:00:05PM -0400, glyph at divmod.com wrote:
[...]
> 
> I've received this suggestion a few times privately and I'm still mulling 
> it over.  On the face of it, it's a good idea, buuuuuut:
> 
> 1. I've had a series of disastrous experiences with commit hooks rejecting 
> changes.  The guy who wrote the commit hook goes on vacation for 3 days, 
> there's a bug in it, and nobody can figure out how to merge code or turn it 
> off for 8 hours; meanwhile the guy trying to commit in bangalore goes to 
> sleep, then doesn't show up again for a week, and the changes are lost.  
> (Having changes on a branch prior to trunk would alleviate this somewhat, I 
> think, but it would be just as much of a pain to be blocked on some stupid 
> merge-script bug.)

Two things make this a non-issue, hopefully:
  - a script that checks that a commit message matches a regex is pretty damn
    simple.
  - in case of emergency, it shouldn't be hard to just disable the offending
    commit-hook until such time as it can be fixed properly.

> 2. The most important thing, really, is a sufficiently descriptive 
> explanation for the change.  Any programmatic attempt to enforce this is 
> sure to drive artificial gaming of the metric; i.e. if we require 3 
> sentences, "fixed random junk in foo module" will become "Fixed random. 
> Junk in. Foo module."

Well, I was really only suggesting that the presence of mandatory metadata is
checked for, not that we write an AI to check that the text of the messages make
sense :)

> 3. 99% of the time (e.g. all "normal" commits) should be in this format; 
> however, the underlying tools, to be honest, are still kind of primitive, 
> and we still have to leave a little wiggle room to deal with their foibles. 
> At least it isn't the masochistic chewing-gum-and-duct-tape perpetual 
> catastrophe of CVS, but SVN still does annoying things like screwing up 
> merges.  If there is a commit with a nice long description and a 'fixes' 
> and everything, which needs to be reverted and reapplied for some dumb 
> technical reason, I'd like to see
> 
>  r1234: "author: foo; reviewer: bar; wonderful wonderful description ..."
>  r1235: "crap, reverting r1234 because of svn bug #9813741"
>  r1236: "reapplying r1234 merged in utterly retarded way to work around svn 
>  bug #9813741"
> 
> not the entire description of r1234 repeated twice -- or, more 
> realistically, three times, since the revert in the middle would also have 
> to have a conformant commit message.

Good point.  So there either needs to be a way to explicitly flag that this
message is ok despite not being in the usual format, or we need to anticipate
all possible forms of exceptional commits.  Requiring e.g. "[override=reason]"
in the text of the commit message somewhere could do this.  This of course
complicates the checker slightly, which relates to point 1, but not impossibly
so.

> 4. precommit hooks keep the SVN SSH transport alive to report errors to the 
> client for the duration of the hook.  When I last investigated (although 
> this was in an SVN pre-release, a good 2+ years ago) if the connection 
> drops while you're validating the commit message, it rejects the commit.  
> This would be especially annoying if you were committing an important fix 
> over some intermittent link, like a GPRS modem.  That's not a common 
> occurrance, but then again, if something were important enough that someone 
> needed to commit code on a *GPRS modem* that is probably the time they 
> would _least_ like something technical to go wrong with the process.

The time it takes to validate a commit message against a regex should be
negligible.  I doubt this will be a real problem.

> This is basically just the same reason that, despite our hardcore attitude 
> towards tests, we don't have buildbot automatically reject or revert 
> commits based on automated test failures.  The tools (in that case, 
> buildbot, trial, and our test suite) are too primitive to rely on without 
> some level of human judgement.  We've been moving to a stricter model so 
> that the human judgements can be as mechanical and apolitical as possible 
> (e.g. "was this a test failure due to too much load on the buildbot / known 
> intermittent failure", not "is this an important enough area of code to 
> worry about test failures in") but we still need it there.

There are important differences between a commit message checker and our test
suite.  Our test suite gives way too many false positives, and does so
unpredictably.  Our test suite takes many minutes to run, at best.  Our test
suite should be run on multiple platforms for full effectiveness.  Our test
suite is extremely complex.  A simple "does the commit message match this regex"
check has none of these problems, assuming the regex isn't insane.

> I am eager to be corrected however; the more work our tools can reliably do 
> for us the better.  Has anybody had a good experience with 
> automatically-rejected commits in another reasonably sized project before?

Yes, the Launchpad development process automatically rejects commits, both for
malformed commit messages[1] and for test failures.  It's generally worked quite
well.  The test suite takes similar lengths of time to Twisted's to run (but
without the portability concerns, thankfully), but we submit requests to merge
to trunk asynchronously (via gpg-signed email), so this isn't a killer for poor
connections.

We have occasionally had problems where a system upgrade or other supposedly
unrelated change causes the test suite on trunk to start failing, effectively
blocking all merges, requiring admin intervention.  Old processes from previous
test runs can also be an issue.  It's generally worked very well for us, though.
And it means we are forced to have zero tolerance of failing tests.  Developers
get unhappy if there's an intermittently failing test that's blocking their
merges :)

We use PQM ("Patch Queue Manager", bzr branch available here:
http://people.ubuntu.com/~robertc/pqm/trunk/, bugs etc here:
https://launchpad.net/products/pqm) to do this.

-Andrew.

[1] We require either "[trivial]", "[r=reviewer]", or "[rs=approver]" to be in the
    message.  "r=" means "reviewed and approved by" and "rs=" means "rubber
    stamped by", i.e. not closely reviewed by the person (generally a manager)
    approving it anyway is happy with it being merged despite that and is
    responsible for the result.  "trivial" should be obvious ;).  Regardless of
    commit message, the full test suite is always run.