Description

domodwyer

opened on Apr 28, 2020

Issue body actions

Hi there!

Most important thing first: pre-commit is great, it makes managing git hooks trivial and when combined with static analysis hooks, helps our team immeasurably - thanks for all the hard work.

We have pre-commit hooks that run a bunch of checks, and thanks to #1339, a post-checkout hook that does some workspace analysis and prints a few warnings if needed.

Unfortunately this seems to have broken the "stash unstaged changes" behaviour when committing - the stash runs a git checkout to clear the working directory before running the hooks, and that fails because of the post-checkout hook. I'd expect that the working directory to be cleared as normal, commit OK and then the working directory to be restored as it does without the post-checkout hook, as the actual mechanism behind removing the unstaged changes and restoring them is hidden from the user.

Below is a quick test to reproduce it:

#!/usr/bin/env bash

mkdir test
cd test || exit
git init

# Configure a pre-commit and post-checkout hook
cat << EOF > .pre-commit-config.yaml
repos:
  - repo: local
    hooks:
      - id: pre-commit-ok
        name: Pre-commit hook that always passes
        stages: [commit]
        language: system
        entry: sh -c 'exit 0'
        always_run: true

  - repo: local
    hooks:
      - id: post-checkout-fail
        name: Post-checkout hook that fails
        stages: [post-checkout]
        language: system
        entry: sh -c 'exit 1'
        always_run: true

EOF

echo "bananas" > great-things.txt
git add .
git commit -m "init"

# Install the hooks
pre-commit install -t pre-commit
pre-commit install -t post-checkout

# Do a checkout - this is expected to print an error from the lint
git checkout -b test

# Change the original file
echo "platanos" >> great-things.txt

# Add another file and stage it
echo "42" > answer.txt
git add answer.txt

# Attempt to commit the new, staged file.
#
# Without the post-checkout hook, this causes the unstaged changes to be
# stashed and the commit works as normal.
echo ""
echo ""
echo "    Attempting to commit with unstaged changes"
echo "    This will fail when attempting to checkout as part of the commit hook:"
echo ""
echo ""
git commit -m "more info"

Unfortunately there seems to be no easy way to skip the post-checkout hook when clearing the unstaged files with git checkout.

Happy to open a PR to help fix this, just unsure what to do - I could swap the git checkout for a git stash save --keep-index --include-untracked but I assume it is using checkout for reason.

Activity

1. asottile commented on Apr 28, 2020

   

   asottile

   on Apr 28, 2020

   Member

   More actions

   hmmm indeed, there's probably two approaches to fixing this -- pre-commit could set a special environment variable that it will skip running everything (basically pingpong to itself)

   # underscore prefixed as it is not a public interface
   _PRE_COMMIT_SKIP_POST_CHECKOUT=1

   or it could force the hooks to not run by pointing to some path which does not exist

   git -c core.hooksPath=/dev/null checkout ...

   iirc the latter doesn't work in older versions of git so you might need to try some stuff out there with building git 1.8 from source 🤔
2. asottile

   added

   bug

   good-first-issue

   on Apr 28, 2020
3. asottile commented on Apr 28, 2020

   

   asottile

   on Apr 28, 2020

   Member

   More actions

   alternatively, it could apply the patch it acquired in reverse (git apply) or figure out a git plumbing command which replicates git checkout
4. domodwyer commented on Apr 28, 2020

   

   domodwyer

   on Apr 28, 2020

   ContributorAuthor

   More actions

   It seems the hooksPath config key was added in Git 2.9.0 - how far back does pre-commit expect to support? I didn't find any Git version info on the docs site.

   Is the git stash method a no-go? It looks like it'll work as far back as Git 1.7.7 and has the added benefit of not scarring the life out of the user if the restore doesn't work as it's right there in git (I've hit this a couple times in older versions and eventually learned where my changes were tucked away).
5. asottile commented on Apr 28, 2020

   

   asottile

   on Apr 28, 2020

   Member

   More actions

   git stash has a number of problems:

   1. it affects the user's stash space
   2. on conflict things are lost
   3. it is not transparent enough for a disaster scenario (pre-commit saves the stash patch currently so a user can recover if something were to go very wrong)

   2.9 is too recent so core.hooksPath is out
6. asottile commented on Apr 28, 2020

   

   asottile

   on Apr 28, 2020

   Member

   More actions

   it seems the plumbing equivalent is git checkout-index --all --force
7. asottile

   closed this as completedin #1422on May 2, 2020

Issue actions