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

[WIP] Don't run hooks on submodule roots #254

Closed

Conversation

chriskuehl
Copy link
Member

git tracks submodules like files. In particular, the roots of submodules appear in the output of git ls-files. This means we get weird errors like:

ckuehl@anthrax:~/tmp$ git init derp && cd derp
ckuehl@anthrax:~/tmp/derp$ git submodule add git@github.com:chriskuehl/dotfiles herp
ckuehl@anthrax:~/tmp/derp$ cat > .pre-commit-config.yaml
-   repo: https://github.com/pre-commit/pre-commit-hooks.git
    sha: 5dd2605fbe26937c721a8ecc0e53949d3eea4f12
    hooks:
    -   id: detect-private-key
ckuehl@anthrax:~/tmp/derp$ pre-commit run --all-files
Detect Private Key.............................................................Failed
Traceback (most recent call last):
  File "/home/c/ck/ckuehl/.pre-commit/repokqqh2jfj/py_env-default/bin/detect-private-key", line 9, in <module>
    load_entry_point('pre-commit-hooks==0.4.2', 'console_scripts', 'detect-private-key')()
  File "/home/c/ck/ckuehl/.pre-commit/repokqqh2jfj/py_env-default/local/lib/python2.7/site-packages/pre_commit_hooks/detect_private_key.py", line 15, in detect_private_key
    content = open(filename, 'rb').read()
IOError: [Errno 21] Is a directory: 'herp'

The only way to avoid this right now is to explicitly exclude submodule paths in you .pre-commit-config.yaml (or to modify every hook to check if an input file is actually a file, which seems bad).

We can exclude these with a slightly more complicated ls-files command.

I didn't test this extensively or even check if the tests still pass, mostly just looking to see if this looks like a reasonable solution. If it does I'll go ahead and write real tests (and test it on some older git versions to make sure it still works).

return [
path for mode, path in [
split(line) for line in output.splitlines()
] if mode != GIT_MODE_SUBMODULE
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also exclude symlinks here if desired. I didn't because I can imagine some useful hook (e.g. checking for symlinks pointing outside of the repo), but the current behavior could also potentially be dangerous (e.g. we could end up running a hook on a symlink pointing outside of the git repo, transparently to the hook, which then "fixes" a file outside of the git repo).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also maybe useful hooks to run against submodules, but imo those should just read .gitmodules instead. Random hooks shouldn't have to worry that they'll be passed in a directory.

@asottile
Copy link
Member

This seems similar to the approach that I wanted to take. One thing that I wanted though is a way for hooks to opt into checking submodules / symlinks in hooks.yaml

@chriskuehl
Copy link
Member Author

What would you think about adding a per-hook option (defined in hooks.yaml or overriden in .pre-commit-config.yaml) like:

types: ['files', 'symlinks', 'submodules']

where the default (if omitted) is just ['files']? Could also separate executable files but not sure that's really helpful.

@asottile
Copy link
Member

Yeah that sounds good, also makes it easy to implement #220

@chriskuehl
Copy link
Member Author

Abandoning this PR, I'll make another that takes care of #191, #220, and #257.

@chriskuehl chriskuehl closed this Aug 7, 2015
droctothorpe pushed a commit to droctothorpe/pre-commit that referenced this pull request Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants