cloud-gov / caulking

Prevent leaks with gitleaks, and use tests to validate
Other
32 stars 11 forks source link

Caulking breaks per-repo git hooks #4

Closed tammersaleh closed 4 years ago

tammersaleh commented 4 years ago

The issue with the core.hooksPath setting is that it uses the global directory instead of the per-repository $GIT_DIR/hooks directory:

$ man git-config
...
core.hooksPath
  By default Git will look for your hooks in the $GIT_DIR/hooks directory. 
  Set this to different path, e.g.  /etc/git/hooks, and Git will try to 
  find your hooks in that directory, e.g.  /etc/git/hooks/pre-receive 
  instead of in $GIT_DIR/hooks/pre-receive.

This breaks a number of plugins like git-crypt (and maybe git-LFS) and doesn't allow for pre-commit linters, etc. Even if we don't use any of those capabilities now, it feels like a recipe for confusion in the future. I can think of two solutions:

  1. Use a much more complicated structure such as pivotal-cf/git-hooks-core. This would be very flexible and would allow us to add all kinds of nifty global git-hook behavior. It would also be a huge change, and somewhat complicated to introduce. Or...
  2. Add this snippet to the existing ~/.git-support/hooks/pre-commit script:

    git_dir=$( git rev-parse --git-dir )
    if [ -f "${git_dir}/hooks/pre-commit" ]; then
     "${git_dir}/hooks/pre-commit" $@
    fi

    This is a simple (and only slightly dirty) solution.

Thoughts?

pburkholder commented 4 years ago

Is the current use of core.hooksPath a breaking change for you? If not, I'd rather not try to anticipate various other use patterns, but wait for those to emerge and then accommodate them.

I don't think caulking breaks per-repo git hooks. My understanding and some simple tests is that if you have a repo/.git/hooks/pre-commit script, that will be executed instead of the one at core.hooksPath. That's why check-repos.sh does a simple check for gitleaks in any per-repo pre-commit script.

So, I don't think your "slightly dirty" solution is needed, as that's already the behavior.

Over to you...

pburkholder commented 4 years ago

@tammersaleh Let me know if I can close this, as I don't think it's the case that caulking break per-repo git hooks. Thx.

tammersaleh commented 4 years ago

I'm fairly confident it does, but I'm also not passionate about fixing it. Feel free to close.

pburkholder commented 4 years ago

Please re-open when you come across any situations that causes breakage for you. Thanks