[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.

Comments

[Gerrit Updater]

Oleg Drokin ([e-mail]) merged in patch https://review.whamcloud.com/25804/
Subject: LU-3434 scripts: check for mdsfilesystemtype= option
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: 1b75e853af52ab99b831d32b828a38dd446b61b5

[Gerrit Updater]

Andreas Dilger ([e-mail]) uploaded a new patch: https://review.whamcloud.com/25804
Subject: LU-3434 scripts: check for mdsfilesystemtype= option
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: f125d721c34b7b64f6192a8b67aecaff3cf4b1f2

[Gerrit Updater]

Oleg Drokin ([e-mail]) merged in patch http://review.whamcloud.com/8644/
Subject: LU-3434 misc: style check whole patch when rebasing
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: af437d84a3be785f25aaf4ccad6e1313ffa3cde1

[Jodi Levi (Inactive)]

Patch landed to Master. Let me know if more work is needed in this ticket and I will reopen.

[Andreas Dilger]

We've decided to do this via a Jenkins hook when the patch is pushed to Gerrit. It is still recommended to install the commit hooks into Git, but it will no longer be done automatically - http://review.whamcloud.com/7046.

[Ned Bass (Inactive)]

Or would you have different trees for development and patch submission?

In effect, yes, because I typically submit from my workstation, but I test and develop on VMs and clusters. What works best for me is a post-commit hook to find style problems, and a commit-msg hook installed only in my submit tree to check the commit message format and add a Change-Id.

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.

Whether post-commit or prepare-commit-msg is more effective depends on work flow and attitude. Someone with a lax attitude toward style correctness is equally likely to ignore comments appended to their commit message as console warnings, so trying to accommodate them is a waste of effort.

For the rest of us, I think prepare-commit-msg only works better if you always abort the commit when you notice the style errors. Personally, I rarely abort a commit, either because

• I've already formulated the wording of the commit message, so I want capture it, or
• I am checkpointing my work in progress, not preparing a commit for upstream submission.

Instead, I tend to use commit --amend to go back and fix things. The prepare-commit-msg hook works poorly for me because

• The original checkpatch.pl output is discarded when I quit the editor.
• Only the new changes are checked on the subsequent amend commit, so unfixed style issues are missed.
• The comments appended below the default comments that git adds are easy to miss entirely.

A post-commit hook, on the other hand, checks the entire patch for each commit. The console output is highly noticeable and gets preserved beyond the commit message editing session.

All of this boils down to personal preference, however. I made the comment about leaving it up to the individual because any choice we make about which hooks to install is going to piss off inconvenience somebody. It's not a huge deal since it won't overwrite existing hooks, though at the very least autogen.sh should tell you what it's doing.

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.

It's easy to detect an amend commit, as the SHA1 of the preceding commit will be available as a 3rd parameter. However, I'm not aware of an easy way to merge the commit with the staged changes in order to check the entire patch. I get the feeling trying to do that would add too much complexity to the script, when what we want can be done easily from post-commit.