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

Setting a global hooks path causes "Cowardly refusing to install" everywhere #1198

Closed
janosh opened this issue Oct 29, 2019 · 24 comments
Closed

Setting a global hooks path causes "Cowardly refusing to install" everywhere #1198

janosh opened this issue Oct 29, 2019 · 24 comments
Labels

Comments

@janosh
Copy link

@janosh janosh commented Oct 29, 2019

I recently setup a global spell checking commit hook that lives in

git config --global core.hooksPath ~/dotfiles/gitHooks

Just noticed that due to this, attempting pre-commit install now fails with

[ERROR] Cowardly refusing to install hooks with `core.hooksPath` set.

in every repo.

Is there actually a reason to fail in the presence of global hooks? Local hooks are still executed unless they're being shadowed by a global hook of the same name, i.e. a global commit-msg hook would prevent a local commit-msg hook from running. In that case, it's the user's responsibility to make sure the global hook calls the local one if so desired.

If there is indeed no reason not to let global and local hooks coincide, fixing this might be as easy as modifying

def has_core_hookpaths_set():
-    _, out, _ = cmd_output_b('git', 'config', 'core.hooksPath', retcode=None)
+    _, out, _ = cmd_output_b('git', 'config', '--local', 'core.hooksPath', retcode=None)
    return bool(out.strip())

Happy to submit a PR for that.

@asottile
Copy link
Member

@asottile asottile commented Oct 29, 2019

pre-commit is nonfunctional with that setting set -- see #718 #663 pre-commit/pre-commit-hooks#250

you may find git's init.templateDir more amenable

@janosh
Copy link
Author

@janosh janosh commented Oct 29, 2019

@asottile I think there's a misunderstanding here. None of the issue you mentioned concern global hooks.

I still don't see how those would be an issue except as explained above (which is user responsibility and doesn't concern pre-commit):

Local hooks are still executed unless they're being shadowed by a global hook of the same name, i.e. a global commit-msg hook would prevent a local commit-msg hook from running. In that case, it's the user's responsibility to make sure the global hook calls the local one if so desired.

@asottile
Copy link
Member

@asottile asottile commented Oct 29, 2019

with core.hooksPath set

$ rm -rf /tmp/x
$ mkdir -p /tmp/x/y
$ git clone -q git@github.com:asottile/astpretty /tmp/x/astpretty
$ cd /tmp/x/astpretty
$ pre-commit install
pre-commit installed at .git/hooks/pre-commit
$ git config --global core.hooksPath /tmp/x/y
$ git commit --allow-empty -m foo
[master 9f1af98] foo

with core.hooksPath unset

$ rm -rf /tmp/x
$ mkdir -p /tmp/x/y
$ git clone -q git@github.com:asottile/astpretty /tmp/x/astpretty
$ cd /tmp/x/astpretty
$ pre-commit install
pre-commit installed at .git/hooks/pre-commit
$ git commit --allow-empty -m foo
Trim Trailing Whitespace.............................(no files to check)Skipped
Fix End of Files.....................................(no files to check)Skipped
Fix double quoted strings............................(no files to check)Skipped
Check docstring is first.............................(no files to check)Skipped
Check Yaml...........................................(no files to check)Skipped
Debug Statements (Python)............................(no files to check)Skipped
Tests should end in _test.py.........................(no files to check)Skipped
Fix requirements.txt.................................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
autopep8.............................................(no files to check)Skipped
Reorder python imports...............................(no files to check)Skipped
pyupgrade............................................(no files to check)Skipped
Add trailing commas..................................(no files to check)Skipped
[master db6914a] foo

@janosh
Copy link
Author

@janosh janosh commented Oct 29, 2019

So a global hook directory automatically shadows all local hooks?

@asottile
Copy link
Member

@asottile asottile commented Oct 29, 2019

that's what my output appears to show, yes -- that's just how git works it seems

this directly contradicts your claim above so I'm confused where you've gotten that from :S

@janosh
Copy link
Author

@janosh janosh commented Oct 29, 2019

I thought I read that about two weeks ago but apparently I misread or the source was mistaken. Can't remember/find the source now.

you may find git's init.templateDir more amenable

The problem with templates AFAIU them is that changes made to the template are not reflected in previously created repos, correct? Or could I place a symlink in the template directory and git would copy that symlink into each repo which would all point to a master file that's always up to date?

@asottile
Copy link
Member

@asottile asottile commented Oct 29, 2019

presumably that would work, yeah

@janosh
Copy link
Author

@janosh janosh commented Oct 30, 2019

The problem with templates AFAIU them is that changes made to the template are not reflected in previously created repos, correct? Or could I place a symlink in the template directory and git would copy that symlink into each repo which would all point to a master file that's always up to date?

This actually also doesn't help me because it doesn't retrofit the hook into existing repos.

In any case, I just did a little more testing and it turns out setting a global hooks directory only shadows local hooks if no local hooks path has been set (i.e. if it's still the default .git/hooks).

If a local hooks directory is set (it can even be set to the default location), local hooks are in fact called (which is why it didn't notice the behavior you showed above).

What was the reason to not just install the hooks in the user specified local hooks directory if one was specified and have pre-commit itself set it to the default .git/hooks otherwise? Seems to me like that would enable joint use of local and global hooks.

@asottile
Copy link
Member

@asottile asottile commented Oct 30, 2019

What was the reason to not just install the hooks in the user specified local hooks directory if one was specified and have pre-commit itself set it to the default .git/hooks otherwise? Seems to me like that would enable joint use of local and global hooks.

pre-commit is not going to muck with user's settings -- or install outside the working repository. This is why it cowardly refuses


could you demo what you're talking about? I still can't reproduce

@janosh
Copy link
Author

@janosh janosh commented Oct 30, 2019

pre-commit is not going to muck with user's settings

You mean git config --local core.hooksPath .git/hooks in case no local hooks path has been set yet? That's not strictly changing any settings, just applying the default.

or install outside the working repository

Why not? Setting a local hooks path outside the repo is very unusual and if a user goes to such lengths, they'll most likely have a good reason. Not sure what the point would be of second guessing the user here.

could you demo what you're talking about? I still can't reproduce

mkdir test && cd test
echo '#!/bin/sh\n\necho foobar' >> .git/hooks/pre-commit && chmod +x .git/hooks/pre-commit
git config --global core.hooksPath /tmp/x
git config --local core.hooksPath .git/hooks
git commit --allow-empty -m 'foobar?'
> foobar
> [master 00ce0f4] foobar?

As you can see in the above output, the local hook is called even with a global hooks path set.

@asottile
Copy link
Member

@asottile asottile commented Oct 30, 2019

pre-commit is not going to muck with user's settings

You mean git config --local core.hooksPath .git/hooks in case no local hooks path has been set yet? That's not strictly changing any settings, just applying the default.

it isn't applying the default, it's overriding whatever the user set

or install outside the working repository

Why not? Setting a local hooks path outside the repo is very unusual and if a user goes to such lengths, they'll most likely have a good reason. Not sure what the point would be of second guessing the user here.

A tool should not just silently write to paths it doesn't control, especially dotfiles :)

could you demo what you're talking about? I still can't reproduce

mkdir test && cd test
echo '#!/bin/sh\n\necho foobar' >> .git/hooks/pre-commit && chmod +x .git/hooks/pre-commit
git config --global core.hooksPath /tmp/x
git config --local core.hooksPath .git/hooks
git commit --allow-empty -m 'foobar?'
> foobar
> [master 00ce0f4] foobar?

As you can see in the above output, the local hook is called even with a global hooks path set.

well yes of course, the git config --local setting overrides the git config --global setting -- but they cannot live in harmony:

$ rm -rf /tmp/x/
$ mkdir -p /tmp/x/y
$ git init /tmp/x/x
Initialized empty Git repository in /tmp/x/x/.git/
$ cd /tmp/x/x
$ echo 'echo pre-commit' > .git/hooks/pre-commit; chmod +x .git/hooks/pre-commit
$ echo 'echo commit-msg' > /tmp/x/y/commit-msg; chmod +x /tmp/x/y/commit-msg
$ git config --global core.hooksPath /tmp/x/y
$ git config --local core.hooksPath .git/hooks
$ git commit -m "commit message" --allow-empty
pre-commit
[master (root-commit) 569547d] commit message

the "global hook" doesn't get run

@janosh
Copy link
Author

@janosh janosh commented Oct 30, 2019

it isn't applying the default, it's overriding whatever the user set

I feel like we're talking at cross purposes somehow. As I wrote above ("in case no local hooks path has been set yet"), if the user has already set a custom local hooks path, you wouldn't need to run this command at all since then local hooks will already not be shadowed by global ones. You only run git config --local core.hooksPath .git/hooks to make this setting explicit if the user did not already specify a different directory. Hence you wouldn't be overriding what the user set.

A tool should not just silently write to paths it doesn't control, especially dotfiles :)

Who said anything about silent? In that case, just tell the user and/or ask for confirmation. 😄

the "global hook" doesn't get run

I'm aware of that. But consider pre-commit's design choice right now. Failing "cowardly" as soon as a global hooks path is set makes it impossible to use for anyone who wants to have a bunch of global hooks that apply to repos without local hooks while repos that do have local hooks can implement custom behavior on a case-by-case basis.

@asottile
Copy link
Member

@asottile asottile commented Oct 30, 2019

pre-commit currently is entirely non-interactive and I'm definitely not changing that to accommodate this use case. It could maybe suggest the other command you're suggesting but I don't think the current outcome changes all that much so I'm not sure what you want. pre-commit itself is not going to git config (set) or write to core.hooksPath as I consider both of those unacceptable.

@janosh
Copy link
Author

@janosh janosh commented Oct 30, 2019

pre-commit itself is not going to git config (set) or write to core.hooksPath as I consider both of those unacceptable.

I see. Thanks for carrying on the conversation with me regardless! I appreciate that.

It could maybe suggest the other command you're suggesting

Which other command? I don't quite follow.

@asottile
Copy link
Member

@asottile asottile commented Oct 30, 2019

git config --local core.hooksPath .git/hooks (though even that's not necessarily correct in submodules, subtrees, etc.)

@damienrj
Copy link

@damienrj damienrj commented Jan 28, 2020

Thanks for the help, it appears that maybe our global pre-commits are still interfering . I will see if I can fix it on that side since when I do pre-commit init-templatedir ~/.git-template pre-commit isn't triggered.

@asottile
Copy link
Member

@asottile asottile commented Jan 28, 2020

oh, so maybe that patch didn't help and the error message is correct? there can only be one location for git's core.hooksPath so you'd need to copy the scripts pre-commit creates into that directory

@damienrj
Copy link

@damienrj damienrj commented Jan 29, 2020

I played around with it for awhile this afternoon, and it appears that hooksPath, even locally in the repo with .git/config that it wasn't triggering. Not quite sure why. Probably best to have the error as per patch. What ended up working was an alias pre-commit && git commit

@asottile
Copy link
Member

@asottile asottile commented Jan 29, 2020

🤷‍♂ ah well, I can possibly revert that later if someone mentions it -- for now it's been released as part of 2.0.0

bschiela added a commit to bschiela/dotrc that referenced this issue Jul 23, 2020
The pre-commit framework is incompatible with core.hooksPath:
pre-commit/pre-commit#1198.

The git hooks workflow is therefore reworked as follows:
1. Custom "global" hooks are copied into all new repos by default on
   `git init` using init.templateDir.
2. To use pre-commit "locally" in some repo, delete the custom default
   pre-commit hook and `pre-commit install`. Custom hooks can still be
   used by adding them to the repo's `.pre-commit-config.yaml` (see the
   config added here for an example).

Hooks are symlinked into the templateDir so that each new repo gets a
copied pointer to a shared, version-controlled hook. This enables
changes to the hook to be automatically and immediately reflected across
all repos. The symlinking is performed at `./install` to avoid commiting
platform/user-specific paths and keep the solution portable.

A `.pre-commit-hooks.yaml` configures this repo to house hooks used by
pre-commit. The `.pre-commit-config.yaml` includes an example of how to
use these hooks in another repo through pre-commit.
benmezger added a commit to benmezger/dotfiles that referenced this issue Mar 11, 2021
benmezger added a commit to benmezger/dotfiles that referenced this issue Mar 18, 2021
@erinaceous
Copy link

@erinaceous erinaceous commented May 19, 2021

Hi!

I understand the reason for not wanting to install in the presence of core.hooksPath, however I feel like I've got a valid use-case for it:

  • I've got a global pre-commit script, which I want to run on every repo. Currently, it's just an interactive Y/N checklist reminding me to do things I often forget to do ("have you added the right files, did you run the unit tests, did you write a descriptive commit message").
  • At the end of the script, it runs the local hooks for that repo.
  • In the global hooks dir I've got a default script which all other scripts are symlinked to and it defaults to running the local scripts, so all the other scripts (i.e. pre-push) will continue to run.
  • The global script(s) are something I'd keep in my own dotfiles and copy between my own dev machines, and are categorically not something that should be shared to repos, or referenced in .pre-commit-config.yaml per-repo -- they're very much "personal preference" things, but are very useful to me.
  • Git templates wouldn't work here, as again, personal scripts which I always want to run, and I'm usually cloning already-established repos from my work or 3rd parties.

As a workaround, I've tested mixing global hooks with pre-commit via unsetting core.hooksPath globally, running pre-commit install and then setting --global core.hooksPath back to what it was. It works fine afterwards.

Could another CLI argument be added? --skip-global-hook-check or something?

@asottile
Copy link
Member

@asottile asottile commented May 19, 2021

no sorry, your workaround is fine and your usecase is not strong enough to warrant a complexity increase and a safety decrease

@asottile
Copy link
Member

@asottile asottile commented May 19, 2021

pre-commit also can't find the hooks directory when core.hooksPath is set because git will tell us about the setting and not the target directory we're looking for

@sesponda
Copy link

@sesponda sesponda commented Aug 4, 2021

@asottile

no sorry, your workaround is fine and your use case is not strong enough to warrant a complexity increase and a safety decrease

Yes, I agree with this. Would you consider turning this into a WARN, e.g. "core.hooksPath is set, which overwrites your repo hooks. Use it at your own risk and be sure you understand the implications of this message.", and let it run?...

Why I'm asking: we use precommit and global hooks extensively, applying @erinaceous 's workaround. This will make our life a lot easier, and be a good balance between not messing with files outside the local repo, but still empowering devs to support more complex cases (at their own risk).

@asottile
Copy link
Member

@asottile asottile commented Aug 4, 2021

as stated above, the answer is no -- I am not willing to have a safety decrease

@pre-commit pre-commit locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants