Sep 05

Tools like perltidy and perlcritic are great for cleaning and validating code, but how do you ensure they get run consistently?

One way is to add an enforcement check at commit time. That is, don’t allow a commit unless the code is tidied and valid.

This prevents the “alternating tidy commit” phenomenon, as in

swartz> git log --pretty=oneline CHI.pm 
5cbbf6c    oops, run perltidy again
055a43a    specify append return value
d26a2c0    run perltidy
b0a97f8    add stats
2cb0a4b    run perltidy
de34b12    fix bug

and eliminates useless stylistic differences between revisions of files.

Using hooks

The latest tidyall distribution contains hooks for running tidyall whenever you commit/push to svn or git. If a file has not been tidied or is deemed invalid, then
the operation is aborted and you must fix the problem before retrying.

In each case, you should commit a tidyall.ini file at the top of your project specifying
which tidiers/validators to apply to which files.

Code::TidyAll::SVN::Precommit

A subversion pre-commit hook. In hooks/pre-commit in your svn repo:

#!/usr/bin/perl
use Code::TidyAll::SVN::Precommit;
use Log::Any::Adapter (File => "/path/to/hooks/logs/tidyall.log");
use strict;
use warnings;

Code::TidyAll::SVN::Precommit->check();

and then

% svn commit -m "fixups" CHI.pm CHI/Driver.pm 
Sending        CHI/Driver.pm
Sending        CHI.pm
Transmitting file data ..svn: Commit failed (details follow):
svn: Commit blocked by pre-commit hook (exit code 255) with output:
2 files did not pass tidyall check
lib/CHI.pm: *** 'PerlTidy': needs tidying
lib/CHI/Driver.pm: *** 'PerlCritic': Code before strictures are enabled
  at /tmp/Code-TidyAll-0e6K/Driver.pm line 2
  [TestingAndDebugging::RequireUseStrict]

In an emergency the hook can be bypassed by prefixing the comment with “NO
TIDYALL”, e.g.

% svn commit -m "NO TIDYALL - this is an emergency!" CHI.pm CHI/Driver.pm 

Code::TidyAll::Git::Precommit

A git pre-commit hook. In .git/hooks/pre-commit:

#!/usr/bin/perl
use Code::TidyAll::Git::Precommit;
use strict;
use warnings;

Code::TidyAll::Git::Precommit->check();

and then

% git commit -m "fixups" CHI.pm CHI/Driver.pm 
2 files did not pass tidyall check
lib/CHI.pm: *** 'PerlTidy': needs tidying
lib/CHI/Driver.pm: *** 'PerlCritic': Code before strictures are enabled

In an emergency the hook can be bypassed by passing –no-verify:

% git commit --no-verify ...

This hook must be explicitly placed in every copy of the repo, although you can partially automate this process. There is (unlike the other two hooks here) no way to require or enforce that the hook is in place, so it may not be ideal for large groups.

Code::TidyAll::Git::Prereceive

A git pre-receive hook, which runs whenever something is pushed to a repo. You might use this to validate pushes from multiple developers to a shared repo on a remote server.

In .git/hooks/pre-receive in the shared repo:

#!/usr/bin/perl
use Code::TidyAll::Git::Prereceive;
use strict;
use warnings;

Code::TidyAll::Git::Prereceive->check();

and then

% git push
Counting objects: 9, done.
...
remote: [checked] lib/CHI/Util.pm        
remote: Code before strictures are enabled on line 13 [TestingAndDebugging::RequireUseStrict]        
remote: 1 file did not pass tidyall check        
To ...
 ! [remote rejected] master -> master (pre-receive hook declined)

Unlike the git pre-commit hook above, this can be enforced, and in fact there is no current way to skip the check without going in and disabling the hook. I’d like to add a flag, but not sure how that would get passed to the hook; advice welcome.

Using a commit alias

A problem with all of the hooks above is that they won’t actually tidy your files for you. They’ll simply tell you what hasn’t been tidied, then send you off to fix things. It’s all a bit tedious.

Unfortunately, modifying your code from svn/git hooks is a no-no; see here, here and here for explanations. (How did we manage before stackoverflow?)

So what I like to do is create an alias for my commit commands, like so:

alias svc='tidyall --svn && svn commit -m '
alias gic='tidyall --git && git commit -m '

Now, if I type

gic "fix some bugs" -a

It will run tidyall --git and only proceed with the commit if that succeeds. The --git and --svn flags mean “process all files that have been added/modified according to git/svn status”. This might be overkill if you’re only committing some of the files, but it’s more efficient than using --all.

As long as I use these aliases, my files should always be tidied by the time the hooks are checking them. But the hooks are still useful as a double-check, and an enforcement layer
that’s harder to skip accidentally.

Counter-arguments

Some will argue that commits should never be blocked by correctness checks. The “commit early and often” philosophy suggests that a commit might be valuable even if the code is currently untidy or invalid; this is especially true in the case of git, where commits are not shared and are designed to be performed often.

Moreover, if you’re in a technical emergency and need to commit code to deploy a fix, it would be unfortunate to be delayed by a nagging validator. (Jeff Thalhammer, perlcritic author, has said that he dislikes running perlcritic on commit for this reason.)

My responses to these arguments are (1) I’ve never personally seen a situation where it was important to commit untidy or invalid code, and (2) the escape-hatches built into the first two hooks (“NO TIDYALL” and –no-verify) will hopefully allow you to proceed during an emergency. But we can agree to disagree. :)

An alternative to running tidyall on commit is to run it during unit tests via Test::Code::TidyAll. In fact, if you’ve got a smoke tester that runs after every commit, this might end up being about the same.

3 Responses to “Tidying and checking code on commit”

  1. Sebastian Says:

    We’re using similar checks to enforce tidy, critic and aperl -c run. Tidy sometimes fails because double-tidied files seem to be differ in some rare cases.
    We had one big problem because the svn server doesn’t always have all dependencies but this is project-related and we solved it.
    The source became much more maintainable and much less errors got online since we establoshed the hooks.

  2. Jonathan Swartz Says:

    Sebastian: perltidy has an –it/–iterations flag that tells it to do multiple iterations. I submitted this patch precisely because of the problem you mention. –it=2 seems to take care of it.

    I will probably add a similar flag to tidyall that can be turned on for specific plugins, because it’s possible any tidier (jstidy, csstidy, etc.) would have the same problem.

  3. Bob Kuo Says:

    At work we have a code review process that requires all checkins to be reviewed before submitting. We use Review Board (http://www.reviewboard.org/) and have a post commit hook that runs a script that checks out the previous version of your files, runs perl critic, applies the patch, runs perl critic again, and shows you the new violations. This is nice since we’ve only recently implemented perl critic and we have a large legacy code base. Also, we haven’t completely refined our perl critic configuration so many of the warnings can be ignored.

Leave a Reply

preload preload preload