Yipit Django Blog: Why You Need a Git Pre-Commit Hook | Hacker News Reader

Yipit Django Blog: Why You Need a Git Pre-Commit Hook

tech.yipit.com

108 pointsnantes19 comments

This is what I use with django+south to prevent forgetting to commit migrations:

  MIGRATIONS_DIRECTORIES="$(find -type d -iname migrations)"
  UNTRACKED_MIGRATIONS="$(git ls-files --exclude-standard --others -- $MIGRATIONS_DIRECTORIES | egrep '(.*)[.]py$')"
  
  if test -z "$UNTRACKED_MIGRATIONS"; then
      # If there are no untracked .py files in the migrations directory, do nothing, allow commit.
      true;
  else
      # If there are untracked files in the migrations directory print a warning message.
      echo "Warning -- Untracked files in the migrations directory"
      echo 'The commit may be forced with "git commit --no-verify"'
      echo 
      echo "$UNTRACKED_MIGRATIONS"
      exit 1
  fi

I can't get behind pre-commit hooks.

1) Sometimes (usually the worst times) you just need to check something in, maybe it doesn't pass pep8 but when service is down nobody gives a shit.

2) Instead of promoting thinking and caution it promotes attitude of "hey check it in and see if it sticks".

Post commit hooks that report broken tests, poor code, etc preferable to whole team are far superior. After first couple of times being shamed people will actually look over their code, run tests, run pep8, etc to verify the commit isn't bogus.

`(Note: A pre-commit hook can be bypassed by passing the --no-verify argument.)`

neat(ish) that definitely solves the one critical issue.

Tool I use doesn't have that AFAIK. My comment was focused on universal concept "pre-commit hook" not a particular tool.

You could get the warnings and still exit with a 0 status and continue, or override the validations. Pre-commit hooks can still get you valuable information.

It seems like pre-commit hooks are an excellent solution, based on your reasons.

First, they're easy to skip with one flag. (But do you really want to be skipping tests when things are already broken? Assuming pre-commit hooks don't take many minutes to run – if they do, I'd consider those hooks too long for a pre-commit – it seems wise to sanity check your emergency commit and help ensure it doesn't make the problem worse.)

Second, having standard sanity/smoke tests run as pre-commit hooks means you can easily run those tests with one command – `git commit` – and not have to worry about either forgetting a test or forgetting to test entirely. Doing those tests as a post commit simply means you've possibly made a record of broken code and you'll probably just rebase before pushing. Doing test post push just means you're wasting others time with your noise or broken code. It's always best to expose problems as early as possible and to keep the influence of those problems as localized as possible.

This isn't to denigrate post push testing. I think it's extremely smart to have long running tests, ideally running on build or test servers, exercise a code base after a push and I think it's may be reasonable to notify many people when those tests fail, depending on the severity and scope of the failure. I just think that making it easy to run sanity/smoke tests and baking that into the infrastructure so it happens quickly and automatically is a smart move as well.

This is my pre-commit hook for Django projects: https://gist.github.com/858223

It just runs pyflakes and does some sanity checking. Best of all, it doesn't go around stashing files or otherwise messing with your working directory or repository. It just uses some git plumbing commands to copy the current index (what you're about to commit) to a temporary directory.

I can't remember where I copied it from but it works perfectly, only copying whatever I'm about to commit:

     git diff --cached --name-only --diff-filter=ACMR | xargs git checkout-index --prefix=$TMPDIR/ --

I can't say how many times this saved me from leaving a pdb.set_trace() in a view somewhere...

Cool. We had looked at doing it that way, but decided that we wanted to have the flexibility of accessing the entire repo so that we can run things like "./manage.py validate" and quick unit tests.

A leftover pdb.set_trace() that snuck into production is the exact reason we first added this :)

So wouldn't:

git checkout-index --prefix=../temp/ --all

work?

Absolutely. Stashing seemed like the cleaner choice to me, but either would work.

You seem to suggest that the index consists of only the files that were modified / added in the commit, which is not the case.

>> the current index (what you're about to commit)

[snip]

> You seem to suggest that the index consists of only the files that were modified / added in the commit, which is not the case.

To see that the parent's parenthesis doesn't suggest that, consider that in Git you commit the state of the whole tree and not just the modified / added files. The Git manual puts it this way:

[git commit] Stores the current contents of the index in a new commit along with a log message from the user describing the changes.

EDIT: Sorry, now I see what you mean:

>> only copying whatever I'm about to commit

... while the code actually only copies those files that have changes about to be committed.

That's pretty cool. I've been using something a bit similar for a while too, just to run my changes through PEP8 (http://tech.myemma.com/python-pep8-git-hooks/). Mine uses the output of `git status --porcelain` to get the changed files, and writes them to a temporary directory with `git show`. Similar result to yours, but I think I like your `git diff` way better.

Personally, I find pre-commit hooks to be a throw-back to older RCSes. For modern systems like Git I would prefer to have a pre-merge hook. That is to say, commit any old crap you want to your local branch but you're not merging with the main development branch until your code is proven to pass the test suite. And only code that is in the mainline dev branch can be merged to main, etc.

Here's my bash pre-commit hook to check for PHP syntax errors and accidentally committed merge conflicts: https://github.com/dave1010/scripts/blob/master/git-hooks/pr...

I think it's a bit overkill to say any pre-commit hook that doesn't stash before running is 'wrong'. If nobody thought to stash before checking the files, maybe nobody else has a workflow that would require it. For me, staging and committing happen in immediate succession, so there wouldn't be any edits that aren't staged. If you work in a way that requires it then go right ahead, but what's the deal lately with calling everyone else 'wrong'?

Here is mine:

https://github.com/lbolla/dotfiles/blob/master/githooks/pre-...

It PEP8-fies and pyflakes .py files and gjslint-ifies .js files. Other actions can be easily added!

Reply on news.ycombinator.com