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

git unadd changes lost if hook fails on windows #570

Closed
RunningToTheEdgeOfTheWorld opened this issue Jul 31, 2017 · 19 comments · Fixed by #575
Closed

git unadd changes lost if hook fails on windows #570

RunningToTheEdgeOfTheWorld opened this issue Jul 31, 2017 · 19 comments · Fixed by #575

Comments

@RunningToTheEdgeOfTheWorld
Copy link

RunningToTheEdgeOfTheWorld commented Jul 31, 2017

D:\CubeadProjects\devops [test +0 ~2 -0 | +0 ~1 -0 !]> git cm "asd"
[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to C:\Users\56929\.pre-commit\patch1501482991.
run pylint...............................................................Failed
hookid: python-pylint

************* Module install
C: 10, 0: Exactly one space required around assignment
a=1
 ^ (bad-whitespace)
C: 46, 0: Line too long (108/100) (line-too-long)
W: 39, 4: Unused variable 'stylelint_root' (unused-variable)
W: 37, 4: Unused variable 'node_root' (unused-variable)
W: 24, 8: Unused variable 'checks' (unused-variable)

[WARNING] Stashed changes conflicted with hook auto-fixes... Rolling back fixes...
An unexpected error has occurred: CalledProcessError: Command: ('C:\\Program Files\\Git\\mingw64\\libexec\\git-core\\git.exe', 'apply', 'C:\\Users\\56929\\.pre-commit\\patch1501483011')
Return code: 1
Expected return code: 0
Output: (none)
Errors:
    error: patch failed: svnchecker_stylelint_support/checks/Stylelint.py:20
    error: svnchecker_stylelint_support/checks/Stylelint.py: patch does not apply


Check the log at ~/.pre-commit/pre-commit.log

~/.pre-commit/pre-commit.log

An unexpected error has occurred: CalledProcessError: Command: ('C:\\Program Files\\Git\\mingw64\\libexec\\git-core\\git.exe', 'apply', 'C:\\Users\\56929\\.pre-commit\\patch1501483011')
Return code: 1
Expected return code: 0
Output: (none)
Errors: 
    error: patch failed: svnchecker_stylelint_support/checks/Stylelint.py:20
    error: svnchecker_stylelint_support/checks/Stylelint.py: patch does not apply
    

Traceback (most recent call last):
  File "c:\python27\lib\site-packages\pre_commit\error_handler.py", line 48, in error_handler
    yield
  File "c:\python27\lib\site-packages\pre_commit\main.py", line 231, in main
    return run(runner, args)
  File "c:\python27\lib\site-packages\pre_commit\commands\run.py", line 273, in run
    return _run_hooks(repo_hooks, args, environ)
  File "c:\python27\lib\contextlib.py", line 24, in __exit__
    self.gen.next()
  File "c:\python27\lib\site-packages\pre_commit\staged_files_only.py", line 58, in staged_files_only
    cmd_runner.run(('git', 'apply', patch_filename), encoding=None)
  File "c:\python27\lib\site-packages\pre_commit\prefixed_command_runner.py", line 38, in run
    return cmd_output(*replaced_cmd, __popen=self.__popen, **kwargs)
  File "c:\python27\lib\site-packages\pre_commit\util.py", line 189, in cmd_output
    returncode, cmd, retcode, output=(stdout, stderr),
CalledProcessError: Command: ('C:\\Program Files\\Git\\mingw64\\libexec\\git-core\\git.exe', 'apply', 'C:\\Users\\56929\\.pre-commit\\patch1501483011')
Return code: 1
Expected return code: 0
Output: (none)
Errors: 
    error: patch failed: svnchecker_stylelint_support/checks/Stylelint.py:20
    error: svnchecker_stylelint_support/checks/Stylelint.py: patch does not apply

Then, I open the patch file. (C:\Users\56929\.pre-commit\patch1501483011),it looks like

diff --git a/svnchecker_stylelint_support/checks/Stylelint.py b/svnchecker_stylelint_support/checks/Stylelint.py
index 4422b4d..f85ecb1 100644
--- a/svnchecker_stylelint_support/checks/Stylelint.py
+++ b/svnchecker_stylelint_support/checks/Stylelint.py
@@ -20,3 +20,5 @@ def run(transaction, config):
             return ('{}\n{}'.format(stdoutdata, stderrdata), 1)^M
^M
     return ("", 0)^M
^M
^M
^M
@asottile
Copy link
Member

asottile commented Jul 31, 2017

Strange, that patch doesn't look like a patch at all! I recently fixed something in this code section (during I think 0.14.2), can you include your version information (both for pre-commit and for git)?

Other information that may be useful:

  • ~/.gitconfig
  • .git/config
  • what the change actually was

I have a sneaking suspicion that one or more of the configuration changes caused one of the following to happen:

  • git checkout -- . didn't actually remove changes
  • git diff didn't actually create a patch but at the same time exited nonzero

@asottile asottile added the bug label Jul 31, 2017
@asottile
Copy link
Member

fwiw, with the default installation of git on windows, this is working for me and is rather heavily tested also in ci with appveyor.

I'm curious to see what settings cause this to happen (I'll be trying some combinations myself)

@asottile
Copy link
Member

Also the output of the following commands would be useful for debugging

git diff --ignore-submodules --binary --exit-code --no-color --no-ext-diff

# with changes made, and with nothing you're concerned about losing, does the following remove all changes to you working directory
git checkout -- .

@asottile
Copy link
Member

this pr may help with this issue as it switches to the more-lower-level commands

@RunningToTheEdgeOfTheWorld
Copy link
Author

RunningToTheEdgeOfTheWorld commented Aug 1, 2017

D:\CubeadProjects\devops [test]> git version
git version 2.13.2.windows.1
D:\CubeadProjects\devops [test]> pre-commit -V
pre-commit 0.15.4

local git config

[core]
	repositoryformatversion = 0
	filemode = false
	bare = false
	logallrefupdates = true
	symlinks = false
	ignorecase = true
	autocrlf = true
	whitespace = fix
[branch "master"]
[branch "devops_cubead_dev"]
[remote "origin"]
	url = git@github.com:CubeADGroup/devops.git
	fetch = +refs/heads/*:refs/remotes/origin/*
[user]
	name = RunningToTheEdgeOfTheWorld
[branch "master"]
	remote = origin
	merge = refs/heads/master

global git config

[filter "lfs"]
	process = git-lfs filter-process
	required = true
	clean = git-lfs clean -- %f
	smudge = git-lfs smudge -- %f
[user]
	name = ZJ
[user]
	email = 569290339@qq.com
[gui]
	recentrepo = D:/CubeadProjects/omms
[alias]
	s = status
	cm = commit -m
	co = checkout
    a = add
    b = branch
    p = push
    d = diff
    dt = difftool
[core]
	autocrlf = true
[diff]
	tool = gvimdiff
[difftool]
	prompt = No
[apply]
	whitespace = nowarn

@RunningToTheEdgeOfTheWorld
Copy link
Author

RunningToTheEdgeOfTheWorld commented Aug 1, 2017

D:\CubeadProjects\devops [test +0 ~1 -0 | +0 ~1 -0 !]> git status
On branch test
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

        modified:   svnchecker_stylelint_support/install.py

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

        modified:   svnchecker_stylelint_support/checks/Stylelint.py

D:\CubeadProjects\devops [test +0 ~1 -0 | +0 ~1 -0 !]> git diff --ignore-submodules --binary --exit-code --no-color --no-ext-diff
diff --git a/svnchecker_stylelint_support/checks/Stylelint.py b/svnchecker_stylelint_support/checks/Stylelint.py
index 4422b4d..83fe854 100644
--- a/svnchecker_stylelint_support/checks/Stylelint.py
+++ b/svnchecker_stylelint_support/checks/Stylelint.py
@@ -20,3 +20,6 @@ def run(transaction, config):
             return ('{}\n{}'.format(stdoutdata, stderrdata), 1)

     return ("", 0)
+
+
+

@asottile
Copy link
Member

asottile commented Aug 1, 2017

That's super helpful! I should be able to reproduce this now and get a fix for you :)

@RunningToTheEdgeOfTheWorld
Copy link
Author

hint:
while I delete the golbal config whitespace-nowarn, i got "trailing whitespace" error .

@asottile
Copy link
Member

asottile commented Aug 1, 2017

Interesting, I can trigger a fatal error by configuring as follows:

[apply]
    whitespace = error

So there's something to fix at least, I'll make a patch for that and see if it fixes your error (crosses fingers)

@RunningToTheEdgeOfTheWorld
Copy link
Author

thank you , and I found I can not apply anypatch( use --ignore-submodules --binary --exit-code --no-color -
-no-ext-diff, or --color=never ), got error: unrecognized input .
I think it maybe a git BUG

@asottile
Copy link
Member

asottile commented Aug 1, 2017

If you're using powershell it might be because of this -- though that shouldn't affect pre-commit since there aren't any shells anywhere.

Here's the patch I've written for whitespace.error: #574

@asottile
Copy link
Member

asottile commented Aug 1, 2017

Can you try the latest master and see if that patch helped?

@RunningToTheEdgeOfTheWorld
Copy link
Author

i am tyring

@RunningToTheEdgeOfTheWorld
Copy link
Author

it doesn't work. but I found the problem.
whether I set the whitespace( cr-at-eol or true ), git diff output to a file is CRLF linesep,
so in staged_files_only.py line39:
patch_file.write(diff_stdout_binary.replace('\r', ''))
it worked.

@asottile
Copy link
Member

asottile commented Aug 1, 2017

Hmm, there has to be some commandline option to make that happen correctly -- I'll look at this in the morning (I still can't get it to reproduce under test!)

@asottile
Copy link
Member

asottile commented Aug 1, 2017

OK! I can finally reproduce this.

Here's my minimal reproduction (also reproducing on linux, but I don't think anyone uses autocrlf=true on linux):

Script

#!/bin/bash
set -ex

rm -rf foo
git init foo
cd foo

# Commit crlf into repository
git config --local core.autocrlf false
python3 -c 'open("foo", "wb").write(b"1\r\n2\r\n")'
git add foo
git commit -m "Initial commit with crlf"

# Change whitespace mode to autocrlf, "commit lf, checkout crlf"
git config --local core.autocrlf true
python3 -c 'open("foo", "wb").write(b"1\r\n2\r\n\r\n\r\n\r\n")'

# Run pre-commit
head -4 ~/workspace/pre-commit/.pre-commit-config.yaml > .pre-commit-config.yaml
~/workspace/pre-commit/venv*/bin/pre-commit

Output

+ rm -rf foo
+ git init foo
Initialized empty Git repository in /tmp/foo/.git/
+ cd foo
+ git config --local core.autocrlf false
+ python3 -c 'open("foo", "wb").write(b"1\r\n2\r\n")'
+ git add foo
+ git commit -m 'Initial commit with crlf'
[master (root-commit) 6644acc] Initial commit with crlf
 1 file changed, 2 insertions(+)
 create mode 100644 foo
+ git config --local core.autocrlf true
+ python3 -c 'open("foo", "wb").write(b"1\r\n2\r\n\r\n\r\n\r\n")'
+ head -4 /home/asottile/workspace/pre-commit/.pre-commit-config.yaml
+ /home/asottile/workspace/pre-commit/venv/bin/pre-commit
[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to /home/asottile/.pre-commit/patch1501609946.
Trim Trailing Whitespace.............................(no files to check)Skipped
[WARNING] Stashed changes conflicted with hook auto-fixes... Rolling back fixes...
An unexpected error has occurred: CalledProcessError: Command: ('/usr/bin/git', 'apply', '/home/asottile/.pre-commit/patch1501609946', '--whitespace=nowarn')
Return code: 1
Expected return code: 0
Output: (none)
Errors: 
    error: patch failed: foo:1
    error: foo: patch does not apply
    

Check the log at ~/.pre-commit/pre-commit.log

@asottile
Copy link
Member

asottile commented Aug 1, 2017

The best approach I can come up with is to temporarily set core.autocrlf = false when applying patches. Replacing \r characters out of a patch will likely break other things, and using git apply --ignore-whitespace causes it to incorrectly modify other line endings.

I'll try and make a patch for this -- I also think this is probably a bug in git (a patch generated by git diff-index --patch cannot be applied by git apply)

@asottile
Copy link
Member

asottile commented Aug 1, 2017

Good news! #575 should solve this issue -- thanks for the patience :)

@RunningToTheEdgeOfTheWorld
Copy link
Author

success, thank you very much!

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

Successfully merging a pull request may close this issue.

2 participants