Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trailing whitespace not working #250

Closed
revolter opened this issue Nov 26, 2017 · 14 comments
Closed

Trailing whitespace not working #250

revolter opened this issue Nov 26, 2017 · 14 comments

Comments

@revolter
Copy link
Contributor

I just installed pre-commit, added this .pre-commit-config.yaml file:

# See http://pre-commit.com for more information
# See http://pre-commit.com/hooks.html for more hooks
repos:
-   repo: https://github.com/pre-commit/pre-commit-hooks
    sha: v1.1.1
    hooks:
    -   id: trailing-whitespace

and added a trailing space to a line in a file in the project, staged it, and when I commit it, it commits with the space, instead of removing it.

@asottile
Copy link
Member

Can you produce the full output of your git commit as well as git show of that commit?

@chriskuehl
Copy link
Member

Also, did you actually install the pre-commit hook to your git repo (e.g. run pre-commit install) after installing pre-commit?

@revolter
Copy link
Contributor Author

revolter commented Nov 27, 2017

@asottile

git commit -m "test":

[feature/git-hooks d80bc2bb4] test
 1 file changed, 2 insertions(+), 2 deletions(-)

git show:

commit d80bc2bb4f58a7c6ec93c345bd6f5252537c6d5a
Author: Iulian Onofrei <iulian.onofrei@yahoo.com>
Date:   Mon Nov 27 20:16:42 2017 +0200

    test

diff --git a/Libraries/IOUtilities/NSString+IOUtilities.m b/Libraries/IOUtilities/NSString+IOUtilities.m
index 07192cc25..1d9d469ea 100644
--- a/Libraries/IOUtilities/NSString+IOUtilities.m
+++ b/Libraries/IOUtilities/NSString+IOUtilities.m
@@ -394,8 +394,8 @@
 						}
 					}
 
-					if ([nextChar hasSuffix:@" --"]) {
+					if ([nextChar hasSuffix:@" --"]) { (before this is a space)
 					}
 
 					if ([nextCharMutable isEmpty]) {

The exact string (before this is a space) was added by me.

@chriskuehl, Yes, ls -al .git/hooks:

total 8
drwxr-xr-x   3 revolt  staff    96 Nov 26 16:24 .
drwxr-xr-x@ 18 revolt  staff   576 Nov 27 20:17 ..
-rwxr-xr-x   1 revolt  staff  1585 Nov 26 23:32 pre-commit

@asottile
Copy link
Member

@revolter perplexing! It seems that pre-commit isn't even being run (I'd expect at the very least some output before the commit message)

Can you try this while a file is staged

bash -x .git/hooks/pre-commit

@asottile
Copy link
Member

also can you produce:

env | grep -i '^git'

@revolter
Copy link
Contributor Author

bash -x .git/hooks/pre-commit:

++ dirname .git/hooks/pre-commit
+ pushd .git/hooks
++ pwd
+ HERE=/Users/revolt/Development/ProjectName/.git/hooks
+ popd
+ retv=0
+ args=
+ ENV_PYTHON=/usr/local/Cellar/pre-commit/1.4.1/libexec/bin/python3.6
+ SKIP_ON_MISSING_CONF=false
+ which pre-commit
+ exe=pre-commit
+ run_args=
+ '[' -x /Users/revolt/Development/ProjectName/.git/hooks/pre-commit.legacy ']'
++ git rev-parse --show-toplevel
+ CONF_FILE=/Users/revolt/Development/ProjectName/.pre-commit-config.yaml
+ '[' '!' -f /Users/revolt/Development/ProjectName/.pre-commit-config.yaml ']'
+ pre-commit run --config .pre-commit-config.yaml
Trim Trailing Whitespace.................................................Failed
hookid: trailing-whitespace

Files were modified by this hook. Additional output:

Fixing Libraries/IOUtilities/NSString+IOUtilities.m

+ retv=1
+ exit 1

and it removed the trailing space of that file in the working tree.

env | grep -i '^git' outputs nothing.

@asottile
Copy link
Member

ok so the script works as expected, now to figure out why git isn't committing as expected :S

Maybe it's something in gitconfig? cat ~/.gitconfig + cat .git/config (make sure to strip anything sensitive!)

Also might be useful:

GIT_TRACE=1 git commit -m "test"

@revolter
Copy link
Contributor Author

revolter commented Nov 27, 2017

Found the culprit, I previously (some months ago) changed the git hooksPath config trying to create my own hooks, so it was looking for hooks in another folder, thank you a lot for the help!

@revolter
Copy link
Contributor Author

Also, the desired behaviour is for it to change the working tree, and after manually reviewing the changes that pre-commit did, stage them and commit again?

@asottile
Copy link
Member

Yep, this approach was taken as hooks can sometimes be wrong and it allows the human some chance to double check the machine.

@revolter
Copy link
Contributor Author

Maybe this could be added to a FAQ?

@asottile
Copy link
Member

fwiw, I think we're going to handle core.hooksPath better, I've opened an issue for it: pre-commit/pre-commit#636

As for the modification faq bit I'd love to add something more to the documentation about this. The current docs briefly mention that modification is considered a failure, but don't really go in depth on the rationale: http://pre-commit.com/#new-hooks

If you'd like to contribute something in that direction, the docs are hosted via github pages: https://github.com/pre-commit/pre-commit.github.io

@asottile
Copy link
Member

Also, big thanks for this issue, definitely something I hadn't even considered before 👏

@revolter
Copy link
Contributor Author

Neither did I 😂 We go lucky I found it by mistake and fixed it easily, so others wouldn't suffer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants