Details
-
Improvement
-
Resolution: Fixed
-
Minor
-
Lustre 2.5.0
-
None
-
8553
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.
Attachments
Activity
Andreas Dilger (andreas.dilger@intel.com) 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
Oleg Drokin (oleg.drokin@intel.com) 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
Patch landed to Master. Let me know if more work is needed in this ticket and I will reopen.
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.
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.
The difficulty I have with leaving it entirely up to the developer is that I am the one that has to spend time inspecting patches and then find half-way through that there are style issues that mean the patch should be rejected.
The automatic installation of the git hooks is relatively new, but I don't know if that makes a difference of not. The git hook has to be installed to check the code style and add the Commit-Id: line. Does this mean you are going to install and remove the commit hook for each patch and then refresh it before submission? Or would you have different trees for development and patch submission?
Andreas, understood. In the meantime, I really think the most reasonable policy is to leave it up to each individual to perform style-checking according to his or her preferred workflow. We already provide the tools to make it easy for people to do this, in the form of checkpatch.pl, example git hooks, and documentation describing how to use them.
Participation in any community entails a responsibility to know and follow the accepted standards and conventions within that community. The right way to combat lax standards is to consistently send the message the we take this responsibility very seriously. Rejecting patches with style violations is a very effective way to achieve this objective. The cost of inspecting, building, and testing the rejected patch is warranted by the important cultural precedent that the rejection sets. This practice makes it clear that both reviewer and submitter have a responsibility to apply and enforce our standards. Contributors will either learn to conform or leave, and the percentage of initial submissions that are stylistically-correct should increase over time.
Installing git hooks without permission is a far less effective tactic. It may frustrate and alienate contributors, encouraging them to subvert the system rather than work within it. It also undermines the cultural attitude that I advocate above, as reviewers may assume that patches have already been properly checked for style, and may therefore be less inclined to look for and flag violations.
Ned,
I'm not against a server-side hook, but having the exception processing adds complexity, and needs more development effort that hasn't been done yet.
Perhaps we could add an automated reviewer in gerrit that checks code style before hudson and maloo run. I see it working something like
- Style-bot added as reviewer on initial push.
- Automatic -1 and skip hudson/maloo if style check fails.
- If guideline exception is desired, include special parameters in the commit message to override the style check. These parameters must identify the location of each exception along with an explanatory comment. This way non-excepted portions of the patch are still checked.
Oleg Drokin (oleg.drokin@intel.com) merged in patch https://review.whamcloud.com/25804/
Subject:
LU-3434scripts: check for mdsfilesystemtype= optionProject: fs/lustre-release
Branch: master
Current Patch Set:
Commit: 1b75e853af52ab99b831d32b828a38dd446b61b5