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

end-of-file-fixer fails against a ".git/MERGE_MSG" file #315

Closed
revolter opened this issue Sep 26, 2018 · 16 comments
Closed

end-of-file-fixer fails against a ".git/MERGE_MSG" file #315

revolter opened this issue Sep 26, 2018 · 16 comments

Comments

@revolter
Copy link
Contributor

revolter commented Sep 26, 2018

I'm running git merge --no-ff release-7.1.2 -m "Merge branch 'release-7.1.2' into master" and it fails because it's incorrectly checking files inside the .git directory:

Fix End of Files.........................................................Failed
hookid: end-of-file-fixer

Fixing .git/MERGE_MSG
pre-commit 1.11.1
git version 2.19.0
macOS 10.14 (18A391)
@asottile
Copy link
Member

ah, probably want to stages: [commit, push] for that hook since it doesn't really make sense during commit-msg?

@revolter
Copy link
Contributor Author

revolter commented Sep 26, 2018

Is it enabled for commit-msg by default? Do you know why it worked before? Is it because of Mojave (that probably comes with a new version of git)?

@asottile
Copy link
Member

yeah it's always been enabled for all of the stages (or, more specifically it takes the default value which is then later treated as any stage)

I wonder if the git version caused this file to be written differently 🤔

@revolter
Copy link
Contributor Author

revolter commented Sep 26, 2018

It contains 3 lines:

  1. Merge branch 'release-7.1.2' into master

@revolter
Copy link
Contributor Author

But why is the hook receiving this file as an input?

@asottile
Copy link
Member

trailing-whitespace matches all files that are "text' (considered "non binary")

The commit-msg pre-commit implementation uses the commit message filename as the "files" considered for hooks. Since that file is text it gets fed through.

Question: are you intending to use the commit-msg hooks? (via pre-commit install --hook-type commit-msg / pre-commit uninstall --hook-type commit-msg?)

@asottile
Copy link
Member

for what it's worth, I can also reproduce your findings on git 2.17.1 (ubuntu bionic):

$ git merge origin/wat --no-ff -m 'Merge into master'
Already up to date!
Trim Trailing Whitespace.................................................Passed
Fix End of Files.........................................................Failed
hookid: end-of-file-fixer

Fixing .git/MERGE_MSG

Not committing merge; use 'git commit' to complete the merge.
$ cat .git/MERGE_MSG 
Merge into master

$

@asottile
Copy link
Member

This diff "fixes" it but I imagine this isn't quite satisfactory 😆

diff --cc .pre-commit-config.yaml
index 9c5d7e5,9c5d7e5..04191b2
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@@ -4,3 -4,3 +4,4 @@@ repos
      hooks:
      -   id: trailing-whitespace
      -   id: end-of-file-fixer
++        stages: [commit, push]

@revolter
Copy link
Contributor Author

I'm using commit-msg for a private hook from a private repo.

@revolter
Copy link
Contributor Author

But I think that diff is quite correct, as it doesn't make sense for end-of-file-fixer to run against the commit message, that's not a file in the project.

@asottile
Copy link
Member

I've been meaning to make a top level default_stages: [...] setting for pre-commit that makes this work ~better. I wonder if the default stages should just be [commit, push] (though it seems weird to special case :S)

@revolter
Copy link
Contributor Author

We could only default the end-of-file-fixer hook to the commit and push stages, not for every hook. So the question is, is there any reason not to do it, and keep the commit-msg enabled for this hook?

@asottile
Copy link
Member

I say let's do it! Probably all the hooks in this repo should have that setup

@asottile
Copy link
Member

Released this as v1.4.0-1

@asottile
Copy link
Member

Thanks again for the issue and fix 🎉

@revolter
Copy link
Contributor Author

And thank you for the release ❤️

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

2 participants