Runs on contents which aren't staged for commit

[asottile]

Here's a minimal example:

FROM ubuntu:bionic
RUN : \
    && apt-get update \
    && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
        ca-certificates \
        curl \
        git \
    && apt-get clean
RUN : \
    && curl --location --silent --output /tmp/lefthook.gz https://github.com/Arkweid/lefthook/releases/download/v0.6.3/lefthook_0.6.3_Linux_x86_64.gz \
    && echo '0876ae7a862cb26aa5bd50173502dfd836cd16317f7af18be0797deeb3e1cfcd  /tmp/lefthook.gz' | sha256sum --check \
    && gunzip /tmp/lefthook.gz \
    && mv /tmp/lefthook /usr/local/bin \
    && chmod +x /usr/local/bin/lefthook \
    && which lefthook

WORKDIR /src

COPY lefthook.yml ./

ENV \
    GIT_AUTHOR_EMAIL=a@example.com \
    GIT_AUTHOR_NAME='A A' \
    GIT_COMMITTER_EMAIL=a@example.com \
    GIT_COMMITTER_NAME='A A'

RUN : \
    && git init . \
    && echo -n "staged contents" > t \
    && git add . \
    && : unstaged trailing whitespace, commit should succeed \
    && echo ' ' >> t \
    && lefthook install

CMD ["git", "commit", "-m", "test"]

pre-commit:
  commands:
    no-trailing-whitespace:
      files: git diff --staged --name-only
      run: if grep ' $' {files}; then exit 1; fi

docker build -t test .
docker run --rm -ti test

$ docker run --rm -ti test
Lefthook v0.6.3
RUNNING HOOKS GROUP: pre-commit

  EXECUTE > no-trailing-whitespace
t:staged contents 

SUMMARY: (done in 0.00 seconds)
🥊  no-trailing-whitespace

the commit should have succeeded as the trailing whitespace was only in the unstaged contents

A similar case exists but in the worse direction (commit allowed but the staged contents are incorrect)

adjusting the part of the dockerfile above:

$ diff -u Dockerfileold Dockerfile
--- Dockerfileold	2019-08-07 09:07:14.589180456 -0700
+++ Dockerfile	2019-08-07 09:09:58.543863227 -0700
@@ -26,10 +26,11 @@
 
 RUN : \
     && git init . \
-    && echo -n "staged contents" > t \
+    && : staged trailing whitespace should not be allowed \
+    && echo "staged contents " > t \
     && git add . \
-    && : unstaged trailing whitespace, commit should succeed \
-    && echo ' ' >> t \
+    && : remove the trailing whitespace but without staging \
+    && sed -i 's/ *$//g' t \
     && lefthook install
 
 CMD ["git", "commit", "-m", "test"]

$ docker run --rm -ti test
Lefthook v0.6.3
RUNNING HOOKS GROUP: pre-commit

  EXECUTE > no-trailing-whitespace

SUMMARY: (done in 0.00 seconds)
✔️  no-trailing-whitespace
[master (root-commit) 2e9d396] test
 2 files changed, 6 insertions(+)
 create mode 100644 lefthook.yml
 create mode 100644 t

this should have failed, but instead allowed trailing whitespace to enter the repo

[aaronjensen]

for reference: https://medium.com/hackernoon/announcing-lint-staged-with-support-for-partially-staged-files-abc24a40d3ff

[ahimta]

Not really sure why this issue is closed as I still have the same problem:

1. "the commit should have succeeded as the trailing whitespace was only in the unstaged contents"
2. "A similar case exists but in the worse direction (commit allowed but the staged contents are incorrect)"

This compromises the integrity of the pre-commit mechanisms as implied by @asottile excellent points above.

What this means, in not so many words, is that the pre-commit script result depends not on the commit (as in pre-commit) but on whatever was there on the filesystem. Not being too fond of the filesystem is why we use version-control in the first place (I think). It could be argued that not having the pre-commit checks could be better as it currently gives false confidence which means bigger landmines. At least without the pre-commit checks I'd know where I stand, whereas, with the pre-commit checks I'd think I know where I stand (a very big difference).

I typically use pre-commit checks as a local CI so that I can guarantee that each and every commit passes them. This has some huge benefits but, unfortunately, they are the kind that you only value if you don't have tolerance for landmines that can waste days and can be prevented with some simple mechanisms. And the most important features of a product is those that users don't know about since they can't advocate for them and maintainers can't know them easily.

For me, Lefthook right now is like ./scripts/<pre-commit-check-script> && commit. For some people it might be a convenience for the script to run in all cases (e.g: committing from a UI).

These unpopular but crucial issues are one reason that I often write tools myself in a shorter time than finding a reliable tool (which doesn't always exist).

For example, I have written a tool that simply copies the repo and discards all changes that aren't going to be committed and run the pre-commit checks and then remove the copy. I hoped that Lefthook would make it possible to replace my own tool but this doesn't seem to be the case.

I don't really understand if this issue isn't at the core of what Lefthook does then what is. I'm definitely biased as this is my main usecase for hooks and would appreciate your dismissing of my elitist and slightly arrogant style and focus on the point at hand.

As an aside, I see the other major feature requested is to run the pre-commit checks only on changed files and I've worked on projects that benefit greatly from this. I think these two major features should be taken very seriously and not be treated like another couple of open issues.

Here are some of the related issues that I came across along the way:

• Documentation/Feature request: git stash on pre-commit and pre-push #131
• run hook against changed content only #239
• Feature Requests: Lint only changed lines, not whole files; Warn, don't Fail #69
• Add stage_fixed flag for hooks #109