Support local pre-commit checks.

Review Request #1883 — Created March 7, 2015 and submitted

jsirois
pants
jsirois/commit_hook/lints
1222
bace88d...
pants-reviews
benjyw, dturner-tw, lahosken, zundel
This change adds a setup script that will install or re-install a
pre-commit hook to run pants lints.  The pre-commit script is also
runnable directly to support a centralized stable script developers can
run to execute all lints independent of performing a commit.

 build-support/bin/check_header.sh         |  2 +-
 build-support/bin/check_packages.sh       |  2 +-
 build-support/bin/ci.sh                   | 11 ++++-------
 build-support/bin/isort.sh                |  2 +-
 build-support/bin/pre-commit.sh           | 11 +++++++++++
 build-support/bin/setup.sh                | 37 +++++++++++++++++++++++++++++++++++++
 build-support/common.sh                   |  4 ++++
 build-support/pants_venv                  |  4 ----
 src/python/pants/docs/howto_contribute.md | 12 ++++++++++++
 9 files changed, 71 insertions(+), 14 deletions(-)

Local testing:

# Install
$ rm -f .git/hooks/pre-commit
$ ./build-support/bin/setup.sh 
Pre-commit checks installed from /home/jsirois/dev/3rdparty/jsirois-pants3/build-support/bin/pre-commit.sh to /home/jsirois/dev/3rdparty/jsirois-pants3/.git/hooks/pre-commit
$ ./build-support/bin/setup.sh 
Pre-commit checks up to date.

# Unanticipated drift
$ rm -f .git/hooks/pre-commit
$ touch /home/jsirois/dev/3rdparty/jsirois-pants3/.git/hooks/pre-commit
$ ./build-support/bin/setup.sh 
A pre-commit script already exists, replace with /home/jsirois/dev/3rdparty/jsirois-pants3/build-support/bin/pre-commit.sh? [Yn]n
Pre-commit checks not installed
$ ./build-support/bin/setup.sh 
A pre-commit script already exists, replace with /home/jsirois/dev/3rdparty/jsirois-pants3/build-support/bin/pre-commit.sh? [Yn]
Pre-commit checks installed from /home/jsirois/dev/3rdparty/jsirois-pants3/build-support/bin/pre-commit.sh to /home/jsirois/dev/3rdparty/jsirois-pants3/.git/hooks/pre-commit
$ ./build-support/bin/setup.sh 
Pre-commit checks up to date.

# Dogfood on this commit
$ git commit
Checking packages
Checking imports
Checking headers
Success
[jsirois/lints/commit_hooks 8458f3e] Support local pre-commit checks.
 9 files changed, 71 insertions(+), 14 deletions(-)
 create mode 100755 build-support/bin/pre-commit.sh
 create mode 100755 build-support/bin/setup.sh

CI went green here:
https://travis-ci.org/pantsbuild/pants/builds/53506978

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
JS
ZU
  1. Nice!

    I patched this in and checked it out. Here are some thoughts I had while doing so.

    My first impression is that it slows down git commit:

    $ time git commit -a -m "testing"
    Checking packages
    Checking imports
    Checking headers
    Success
    real    0m4.188s
    user    0m2.278s
    sys 0m2.491s
    

    Its mostly the header check that is slow. I think that could be easily sped up by rewriting it in python like isort is.

    $ git commit -a -m "testing bad commit"
    Checking packages
    Checking imports
    ERROR: /Users/zundel/Src/pants/src/python/pants/backend/codegen/targets/java_protobuf_library.py Imports are incorrectly sorted.
    
    To fix import sort order, run `build-support/bin/isort.sh -f`
    

    What do you think about having the commit script just run isort.sh -f? A bit scary, I know, but I think its just as fast, and after using it for a while I kind of trust it.

    ~/Src/Pants rb1883 * git commit -a -m "testing bad commit"
    Checking packages
    Checking imports
    Checking headers
    ERROR: All .py files other than __init__.py should start with the following header
    # coding=utf-8
    # Copyright YYYY Pants project contributors (see CONTRIBUTORS.md).
    # Licensed under the Apache License, Version 2.0 (see LICENSE).
    
    from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                            unicode_literals, with_statement)
    ---
    The following files don't:
    
    src/python/pants/backend/codegen/targets/java_protobuf_library.py
    

    It would be nice to have a quick fix for this too.

    1. I'm completely happy with both speed and the isort default of not fixing.  I'l leave these changes to you to champion in follow ups.
    2. That's totally fair, I just wanted to share my experiences.

  2. 
      
ZU
  1. 
      
  2. build-support/bin/setup.sh (Diff revision 1)
     
     

    OK, I switched back to master and got some unpleasant behavior:

    $ git commit -a -m "Updated"
    Checking packages
    fatal: Not a git repository: '.git'
    find: src: No such file or directory
    find: tests: No such file or directory
    find: pants-plugins: No such file or directory
    find: examples: No such file or directory
    find: contrib: No such file or directory
    

    and the commit fails.

    At a minimum, could you add a message to the top as follows?

    echo "Running pre-commit hooks in $0"

    When this script gets stale, its going to create a bug we can't reproduce outside the users machine. Also, if we change it we will have to have the developer run a new command each time.

    How about instead of a hard link, we just echo out a small wrapper script to .git/host/pre-commit

    1
    2
    3
    4
    5
    6
    #!/usr/bin/env bash
    
    SCRIPT=build-support/bin/pre-commit.sh
    echo "Running $SCRIPT as pre-commit hook in $0"
    
    exec $SCRIPT
    

    Then we can update the pre-commit hook as we see fit by just checking in changes to build-support/bin/pre-commit.sh, or a user can edit the hook and still get updates that we make to our hook script.

    1. Good catch on the history bit - fixed.
      
      The hardlink accounts for edit build/support/pre-commit.sh to edit already-installed .git/hooks/pre-submit.  I only showed the `# Unanticipated drift` section in testing done for truly exceptional cases where the answer would always be - "if you didn't intend to muck with the hook, just run setup and say 'Y'.
      
      The issue you discovered is the 1 time historical edge of introduction of this pre-commit.  If you rewind before this commit, the scripts the pre-commit hook runs all have a derivation of REPO_ROOT that is incompatible with the hook (which is why you see "fatal: Not a git repository: '.git'").
      I can fix this historical edge case by using a symlink instead because a broken symlink hook is skipped by git when a commit runs.  So if you travel back in time before this commit with the symlinked hook the hook won't run, forward in time and the hook always runs whatever is in build-support/bin/pre-commit.sh just as in the hardlink case.
    2. I like the soft link solution better. I think hard links are cool, but they are so infrequently used that they can be confusing. The hard link would mean that my custom changes to the git hook would have also put my custom edits into the pre commit hook only to have them show up in the pants git repo and it might take a while to figure out why. Also, if you (for some reason) remove the file in your repo and add it back, your hard link relationship would be broken.

  3. 
      
JS
JS
JS
LA
  1. 
      
  2. build-support/bin/setup.sh (Diff revision 3)
     
     
    # set up something something
    
    Where I "something something" might be something like "development environment, so far consisting of pre-commit hooks"
    1. Yeah, setup.sh is pretty generic - fixed.
  3. 
      
JS
JS
ZU
  1. Thanks for making this easier.

  2. 
      
JS
  1. Thanks guys - submitted @ https://github.com/pantsbuild/pants/commit/360f964cd475a5dcbbd192bb640d2c31be5b23e2 and doc site re-published.
  2. 
      
JS
Review request changed

Status: Closed (submitted)

DT
  1. 
      
  2. build-support/bin/setup.sh (Diff revision 4)
     
     

    Why not just use cmp to compare the files?

    (also, md5 should never be used; I know it is unlikely to be an issue here but it seems better to avoud promoting bad practices)

    1. I did not know of cmp - I'll send a new RB shortly to switch.
      As far as the inadvisability of md5 ... the hashing here is not pretending or trying to be attack resistant.  Any low-probability under non-malicious use hash will do.
    2. https://rbcommons.com/s/twitter/r/1892/
  3. 
      
DT
  1. Ship It!
  2. 
      
Loading...