Project

General

Profile

Task #648

Use of commit hook scripts

Added by Mark Abraham almost 10 years ago. Updated over 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
Gerrit
Target version:
-
Close

Description

On gmx-developers there has been a discussion (http://lists.gromacs.org/pipermail/gmx-developers/2011-January/004987.html) about policies for commit messages and enforcement thereof. The git way of doing this is to use either or both of client- and server-side "commit hook scripts" (or just "hooks") that reject commits, or offer templates for commit messages, or run external programs, or send emails, etc.

Some useful reference information:
http://progit.org/book/ch7-3.html (general intro)
http://www.kernel.org/pub/software/scm/git/docs/githooks.html (list of possible hooks)
http://progit.org/book/ch7-4.html (Ruby example of hooks that check for a custom commit message format, enforce fast-forward-only pushes, and allow only certain users to modify certain subdirectories in a project)
http://benjamin-meyer.blogspot.com/2007/03/git-and-hooks.html (list of useful hook possibilities)
http://benjamin-meyer.blogspot.com/2008/10/git-hooks.html (suggestion for hook to enforce coding style)
http://stackoverflow.com/questions/2293498/git-commit-hooks-global-settings (how to do per user and per machine global hooks)

Borrowing from the third link above, here's a list of issues that I think we may want to consider creating hooks for. I'll edit and rearrange this according to the discussion. The list:

Essential
  • Spell check the commit message
    - is GNU ispell standard?
  • Verify that the commit message format is used
    - need to agree a format
  • Send out an e-mail to the mailing list
    - done already
  • Update/close bugs in a bug tracking system
    - Redmine takes care of some of this automatically, and whether commits can close bugs is configurable
Nice to have
  • Check that the code in the patch conforms to a code style
    - need to agree a coding style beyond the existing emacs magic line - there was stuff on the old wiki, I recall
    - downside is that we'll need third-party software like http://astyle.sourceforge.net on the server to enforce it
  • Spell check user strings, documentation, etc
    - is GNU ispell standard?
  • Verify any new files have a copyright header
  • Check for calls to functions that are known to be deprecated or inefficient
    - need file in repo to list such functions
  • Automatically remove (trailing) whitespace
  • Prevent an accidental rebase from occurring on a master branch that everyone shares
Overkill / Obstructive
  • Check that the project compiles
  • Check for compiler warnings
To introduce later
  • Require test code to exist
  • Require tests to pass before allowing a commit
  • Check that any public API is documented and has no errors

I think we should roll some of this out ASAP, and probably have a fairly slow update cycle thereafter - updating local client-side hooks to conform to upstream policies rates to be somewhat irritating.


Related issues

Related to GROMACS - Task #845: Format white space correctlyClosed11/25/2011
Related to GROMACS - Task #818: Clean up source file headers and set up automatic generationClosed09/22/2011

History

#1 Updated by Mark Abraham almost 10 years ago

  • Assignee deleted (Erik Lindahl)

Users can bypass client-side hooks with git commit --no-verify. This might be very useful in some workflows. I know when I'm debugging local code that the ability to make temporary commits before each compilation, then compile and test, and then rebase through various permutations is useful. Later, I have to clean up with a local interactive rebase.

#2 Updated by Mark Abraham almost 10 years ago

Users will have to install their own client-side hooks; git clone does not propagate the .git/hooks directory that contains the hooks. That means the main development team needs to supply a tarball of client hook scripts that users can unpack into (say) $PREFIX/share/git-core/templates/hooks so that new clones on that host get the new set of hook scripts automatically, and old clones can use git init to get/update those hooks. Users can further customize their own hooks to suit their workflows (see http://stackoverflow.com/questions/2293498/git-commit-hooks-global-settings). Another possibility is to have a git repo of the hook scripts, and people can have their local $PREFIX/share/git-core/templates/hooks as a clone, and so get updates readily with git pull.

One downside is that $PREFIX/share/git-core/templates/hooks may not be available for customization on (e.g.) central computing clusters.

#3 Updated by Mark Abraham almost 10 years ago

The prepare-commit-msg hook inserts text before the usual commit message text to create something like this suggestion:


# Enter a one-line 50-character commit summary above here.
# This limit will be enforced.
# Use '#456' to refer to Redmine issue number 456
#                                                |<--50

# Leave the above line blank

# Optionally, insert a longer description on multiple lines above here.
# If any line has more than 72 characters, it will be automatically
# wrapped.
#                                                                      |<--72
#
# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
# On branch release-4-5-patches
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#       modified:   CMakeLists.txt
#
Then the commit-msg hook enforces:
  • First line has at most 50 chars (reject the commit otherwise)
  • Then a blank line (if there is any other text; reject the commit if there's further text and no blank line)
  • Then free-format text (but with 72-character line-wrapping)
  • No trailing whitespace on any line (script will silently trim)
  • No trailing blank lines (script will silently trim)

I'm happy to make up some combo of bash and perl for those hooks if someone will create a git-commit-hooks repository.

#4 Updated by Teemu Murtola almost 10 years ago

On the possible commit hooks:
  • For client-side hooks, we could provide all kinds of things like spell checking etc., but I would keep the server-side scripts to a minimal, easily enforceable and predictable set. E.g., commit message format (with not too strict checks), disallowing tabs and trailing whitespace (git diff --check is convenient for trailing whitespace), and disallowing non-fast-forward updates on major branches. Nothing would be more annoying that having 100 local commits, where the first one had some small omission making it fail an overly strict heuristic check, and forcing the developer to rebase the whole history to be able to push it to the server...
  • I would hate a template like the one in comment 3, because my editor already provides very clear visual cues for anything that breaks the rules.
  • Although it makes everyone's life easier if every commit compiles, I agree it would be overkill (and probably also too slow) to check for the code compiling/passing tests in a commit hook. These things would be better handled through a separate build/test server where builds with different options (MPI, no MPI, different compilers, ...) are triggered by new commits, and if the tree does not build in some configuration, it reports it somewhere. Same for tests.

#5 Updated by Mark Abraham almost 10 years ago

Teemu Murtola wrote:

On the possible commit hooks:
  • For client-side hooks, we could provide all kinds of things like spell checking etc., but I would keep the server-side scripts to a minimal, easily enforceable and predictable set. E.g., commit message format (with not too strict checks), disallowing tabs and trailing whitespace (git diff --check is convenient for trailing whitespace), and disallowing non-fast-forward updates on major branches. Nothing would be more annoying that having 100 local commits, where the first one had some small omission making it fail an overly strict heuristic check, and forcing the developer to rebase the whole history to be able to push it to the server...

Sounds reasonable. Certainly the client-side hooks should not pass anything that will fail on the server.

Actually spell-checking commit messages sounds less reasonable now that I think about it. We'd need a massive private dictionary of variable/function/macro/etc. names, file names, git branch names, MD acronyms...

  • I would hate a template like the one in comment 3, because my editor already provides very clear visual cues for anything that breaks the rules.

You'd be free to not use that client-side hook, of course. Being client-side, these are "opt-in" tools - so long as you're avoiding server-side "gotchas" in another way, that's fine. I do think we need a template that is not dependent on editor magic, particularly for irregular developers.

#6 Updated by Teemu Murtola almost 8 years ago

  • Category deleted (mdrun)

I would suggest moving this issue to the Support Platforms project, as this has very little to do with actual Gromacs source code. If there are no objections, I will do that in the near future.

#7 Updated by Teemu Murtola almost 8 years ago

  • Project changed from GROMACS to Support Platforms

#8 Updated by Teemu Murtola almost 8 years ago

  • Category set to Gerrit

Although this is mostly git-related, set the category to Gerrit, since that is very closely integrated with how we use git.

#9 Updated by Teemu Murtola almost 8 years ago

Added a few related issues where it has been suggested that commit hooks could be used for some purpose instead of manual work.

#10 Updated by Szilárd Páll over 3 years ago

Anything unresolved here?

#11 Updated by Mark Abraham over 3 years ago

  • Status changed from New to Closed

I think we've done enough. open if there's further thoughts!

Also available in: Atom PDF