-
Notifications
You must be signed in to change notification settings - Fork 1k
fix(git-hooks): prefer core.hooksPath and avoid symlink on Windows #5438
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
Open
Shiven0504
wants to merge
5
commits into
clockworklabs:master
Choose a base branch
from
Shiven0504:fix/git-hooks-installer
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+724
−262
Open
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
4cd8837
Replace forked jsonwebtoken crates
Shiven0504 94ce98d
fix(git-hooks): prefer core.hooksPath and avoid symlink on Windows
Shiven0504 24cfec1
docs: document installing Git hooks (use core.hooksPath)
Shiven0504 7a394b7
reverted to jsonwebtoken = { version = "10.4.0", features = ["rust_cr…
Shiven0504 b3b8a49
Delete .github/ISSUES/0001-git-hooks-installer-bug.md
Shiven0504 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| Title: Hook installer removes .git/hooks and uses symlink — breaks on Windows; prefer git core.hooksPath | ||
|
|
||
| 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 | ||
| bash git-hooks/install-hooks.sh | ||
| ``` | ||
|
|
||
| 2. 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. | ||
|
|
||
| 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. | ||
|
|
||
| Acceptance criteria | ||
| ------------------- | ||
| - Running `bash git-hooks/install-hooks.sh` configures hooks for normal Git clients on Linux/macOS/Windows without requiring symlink privileges. | ||
| - Existing `.git/hooks` is not accidentally deleted without fallback/config instructions. | ||
| - The repository documentation includes a short note about installing hooks (optional follow-up). | ||
|
|
||
| Suggested labels | ||
| ---------------- | ||
| - bug | ||
| - platform-windows | ||
| - good first issue | ||
|
|
||
| Suggested assignee | ||
| ------------------ | ||
| - Leave unassigned or assign to core-maintainers | ||
|
|
||
| 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`. | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?