[LU-3434] improve git commit hooks for code style checking

• Type: Improvement
• Resolution: Fixed
• Priority: Minor
• Fix Version/s: Lustre 2.5.0, Lustre 2.9.0, Lustre 2.10.0
• Affects Version/s: Lustre 2.5.0
• Assignee: John Hammond
• Reporter: Andreas Dilger

Description

In http://review.whamcloud.com/6339 there are complaints about the current git commit-msg and prepare-commit-msg hooks:

• they are disruptive during development
• they don't find style issues in the original patch if --rebase or --amend is used
• the checkpatch.pl script does not verify the formatting of shell scripts (tabs, spaces around operators, etc)

My goal is to catch problems in patches before they are submitted to Gerrit with gratuitous style issues that put load on the build/test systems and on inspectors for patches that will be rejected. Also, catching style issues earlier reduces the cycle time for developers, who don't have to wait for a human to inspect the patch just to catch problems that they could have found trivially on their own system. The Lustre code style enforcement has been lax in the past, with mixed results in the quality of the formatting, and with the push to submit the code into the upstream kernel I'd like to move the code continuously closer to the upstream style rather than continuing to diverge.

I'm definitely open to suggestions on how this might be accomplished more efficiently than it is today.

Depending on the workflow, these may or may not be issues. If the patch is style checked each time it is committed, and the issues are addressed at that time, the --rebase or --amend issues do not manifest. If there is a way to detect in the commit hook that this is a rebase/amend and the full patch is checked, that would be great. I think that checking it in the post-commit hook is problematic, since the patch has already been committed at this time, and developers are lax to actually look at and fix problems at that time. I'm also reluctant to hard enforce the code style, since there are also some cases where it is better to ignore the guidelines and do what is easier to read.

I'm not dead set on the current behaviour, and I'm definitely open to changes to the scripts (e.g. add "SKIP_VERIFY" check in commit hook and onus is on developer to do manual checking), but be forewarned that patches with formatting issues will be rejected if you take it on yourself to do the checking.