Skip to content

fix(git-hooks): prefer core.hooksPath and avoid symlink on Windows#5438

Open
Shiven0504 wants to merge 5 commits into
clockworklabs:masterfrom
Shiven0504:fix/git-hooks-installer
Open

fix(git-hooks): prefer core.hooksPath and avoid symlink on Windows#5438
Shiven0504 wants to merge 5 commits into
clockworklabs:masterfrom
Shiven0504:fix/git-hooks-installer

Conversation

@Shiven0504

@Shiven0504 Shiven0504 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary

git-hooks/install-hooks.sh unconditionally removes ../.git/hooks and attempts to create a symlink to ../git-hooks/hooks. On Windows (and some restricted environments) creating symlinks requires elevated privileges or is unsupported, and the script can unintentionally remove existing hooks. This causes the installer to fail or damage developer setups.

Steps to reproduce

  1. On a Windows machine without symlink privileges, run from repo root:
bash git-hooks/install-hooks.sh
  1. Observe that ln -s fails (or that .git/hooks was removed), and that hooks are not installed.

Expected behavior

  • Installer should set up hooks in a cross-platform way without requiring symlink privileges and without deleting existing hooks unless explicitly requested.

My Proposed fix

  • Use git config core.hooksPath git-hooks/hooks by default (works cross-platform and avoids symlinks).
  • Only attempt a symlink as a fallback when git is not available, and show a clear warning.
  • Only run rustup component add rustfmt if rustup is installed.

Patch / PR

I have a proposed patch that updates git-hooks/install-hooks.sh to prefer git config core.hooksPath and to guard rustup usage. The patch is ready in a branch fix/git-hooks-installer.

Comment thread Cargo.toml Outdated
itoa = "1"
json5 = "0.4"
jsonwebtoken = { version = "10.4.0", features = ["rust_crypto"] }
jsonwebtoken = "10.4.0"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

these changes seem unrelated - can you revert?

@@ -0,0 +1,45 @@
Title: Hook installer removes .git/hooks and uses symlink — breaks on Windows; prefer git core.hooksPath

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can you remove this file?

@bfops bfops left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I appreciate the issue and the intention here, but I'm not sure that this is the right way to address it. This would mean that if I want to add (or modify) any hooks for my own workflow, I would need to put them into the repo-tracked directory and constantly have unstaged changes.

Maybe the right solution is to copy the files instead of symlink, but honestly we don't have an active pain point with the way it is right now (most contributors and many team members don't install the hooks in the first place) so I'm not sure that we want to spend time iterating on a solution.

…ypto"] } at line 236

Signed-off-by: Shiven Garia <shivengaria@gmail.com>
Signed-off-by: Shiven Garia <shivengaria@gmail.com>
@Shiven0504

Copy link
Copy Markdown
Contributor Author

Hi @bfops, that's a valid concern I hadn't fully thought through. I realized that pointing core.hooksPath at a tracked directory does make local customization messy.

The copy approach avoids that hence contributors can freely modify their local copies without polluting the working tree. The catch is that hook updates don't actually propagate automatically, but given that most contributors skip installation
entirely it seems like the right call.

@Shiven0504

Copy link
Copy Markdown
Contributor Author

Happy to close this if the team doesn't see it as an active pain point!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants