svn pre-commit hook: help needed

Anyone out there interested in helping out with a subversion pre-commit hook to:

  • remove trailing whitespace,
  • expand tabs to spaces,
  • detect 80-col violations,

as well as detect other style guideline breakage?

I just ran into the trailing whitespace problem: Eclipse and other editors like to trim excess whitespace from source. However, when one commits a patch with trailing whitespace removed, the extraneous diffs make reading the patch more difficult.

Reply to me privately if you’re interested in helping out.

-scooter

I'd argue for not changing anything, just fail it.

Trimming whitespace is innocuous, at best. Expanding tabs to spaces, I might be inclined to agree is a ‘fail’ since weird formatting can result. 80-col violations are absolutely a ‘fail’.

-scooter

I’d recommend just making everything be a fail.

-Chris

Unless you're doing a testcase that wants to verify a feature.

All I’m hoping for is a clean diff. Fail commits for trailing whitespace if they’re source? Easy to detect .ll files.

Hi Scott,

Anyone out there interested in helping out with a subversion pre-commit hook
to:

- remove trailing whitespace,
- expand tabs to spaces,
- detect 80-col violations,

as well as detect other style guideline breakage?

I just ran into the trailing whitespace problem: Eclipse and other editors
like to trim excess whitespace from source. However, when one commits a
patch with trailing whitespace removed, the extraneous diffs make reading
the patch more difficult.

from the subversion manual:

"While hook scripts can do almost anything, there is one dimension in which hook script authors should show restraint: do not modify a commit transaction using hook scripts. While it might be tempting to use hook scripts to automatically correct errors, shortcomings, or policy violations present in the files being committed, doing so can cause problems. Subversion keeps client-side caches of certain bits of repository data, and if you change a commit transaction in this way, those caches become indetectably stale. This inconsistency can lead to surprising and unexpected behavior. Instead of modifying the transaction, you should simply validate the transaction in the pre-commit hook and reject the commit if it does not meet the desired requirements. As a bonus, your users will learn the value of careful, compliance-minded work habits."

Ciao,

Duncan.

Yet another _fun_ way of doing this is to setup a buildbot slave just
for that. The slave can fix minor stuff like tabs and trailing
whitespaces on its own (checking the changes back in), and yell for
things like 80-col violations and whatnot where the changes would not be
so trivial.

People who don't care are not bothered too much (their code might be
changed by the slave), and fascis^H^H^H^H^H^Hpeople who care can quickly
find out where the errors are.

By setting the tree stable timer for the slave doing the check lower
than the slaves doing the actual builds, no extra build is generated in
case the code is modified.

Beware, some people might try to ddos your buildbot after seeing all
their code rewritten, and some other will simply hate you for all the
yelling :wink:

Julien,

If you're going to change anything then this is the best alternative, otherwise I can live with status quo.

Do not reject commit just because of formatting issues. It can have serious -ve impact on productivity.

To folks who prefers to reject commits due to formatting errors -- You already rely on a some kind of "tool" to make your day to day life easier. [ Most likely you've your editor automatically replacing tabs into spaces. Your terminal window is only 80 col. wide or your editor is displaying a vertical line to warn you about 80 col. and so on... ]. The build bot suggested by Julien is yet another "tool" that accomplishes the same. One the slave bot can use clang static analyzer ... :slight_smile:

For the complete truth in advertising, this was pretty much a trial balloon to gauge reaction. I’m not a big fan of rejecting commits for style violations, but the dev guide has certain guidelines regarding formatting and style. And we’re all supposed to be good citizens…

My biggest nit, however, was contemplating a commit where 80%+ was trailing whitespace trimming. Yeah, my editor happens to practice good hygiene. I could have been a complete *$$**le and committed a global hygiene patch. I only touched the files that I’ll end up committing in another day or two.

Since there are style rules, I also decided to see how much reaction there would be if they were enforced at commit time. Evidently, it’s as popular as a skunk at a garden party. So, it’s not something I’m personally looking to invest much time into.

-scooter

On a related note, I wrote a few scripts to detect and correct some types of such style errors, see llvm/utils/lint/* .
I also added a function to llvm/utils/vim/vimrc to delete trailing whitespace and highlight existing trailing whitespace – if anyone’s an Emacs-lisp hacker, please add it to the emacs config file as well.

Sure, this doesn’t enforce anything, but I’m hoping folks will start to use these tools and will over time clean up the style in the entire code base.

2009/2/20 Scott Michel <scooter.phd@gmail.com>

For the complete truth in advertising, this was pretty much a trial balloon to gauge reaction. I’m not a big fan of rejecting commits for style violations, but the dev guide has certain guidelines regarding formatting and style. And we’re all supposed to be good citizens…

My biggest nit, however, was contemplating a commit where 80%+ was trailing whitespace trimming. Yeah, my editor happens to practice good hygiene. I could have been a complete *$$**le and committed a global hygiene patch. I only touched the files that I’ll end up committing in another day or two.

I’ve been fixing things on a directory-by-directory basis as I come across style violations while browsing the code. I’m not in favor of a single global change to fix everything everywhere; I think this can be done gradually over time and the diff will be easier to read if it’s smaller, so you can verify that the script (or your editor) did not mangle anything.

Misha

I also added a function to llvm/utils/vim/vimrc to delete trailing whitespace and highlight existing trailing whitespace -- if anyone's an Emacs-lisp hacker, please add it to the emacs config file as well.

Usually this is done via develock minor mode (one can google for 'develock.el')

I've got a fairly simple perl script that trims trailing whitespace and it's remarkably effective. It even works over recursive directories (not hard to do, but it's the way to do this globally.) I'm sure it's not too much of a stretch to translate tabs to spaces, although that's controversial.

-scooter

2009/2/20 Scott Michel <scottm@aero.org>

I’ve got a fairly simple perl script that trims trailing whitespace […]

% cat llvm/utils/lint/remove_trailing_whitespace.sh
#!/bin/sh

Deletes trailing whitespace in-place in the passed-in files.

Sample syntax:

$0 *.cpp

perl -pi -e ‘s/\s+$/\n/’ $*

Yep, it’s a one-liner.

[…] and it’s remarkably effective. It even works over recursive
directories (not hard to do, but it’s the way to do this globally.)

With recursion into subdirectories:

% remove_trailing_whitespace.sh find . -name \*\.h

I’m sure it’s not too much of a stretch to translate tabs to spaces,
although that’s controversial.

Good point, I should add a verifier to the lint tool to check for tabs in non-Makefiles.