feat: silent success if nothing has changed #138
Conversation
🦋 Changeset detectedLatest commit: e49d1af The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughCLI now logs success only when hooks were actually changed. Core hook functions were made idempotent and return change flags; CLI checks that flag. Tests were added to assert silent second-run behavior and a changeset was added for a minor release. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User / prepare script
participant CLI as cli.js
participant SGH as setHooksFromConfig()
participant HOOK as _setHook / _removeHook
participant FS as File System
U->>CLI: run simple-git-hooks (cwd, argv)
CLI->>SGH: setHooksFromConfig(cwd, argv)
SGH->>HOOK: for each configured hook -> ensure or remove
HOOK->>FS: read existing hook file
alt content differs or file existed when removing
HOOK->>FS: write or remove file
HOOK-->>SGH: { hookChanged: true, success: true } or true
else identical / nothing to remove
HOOK-->>SGH: { hookChanged: false, success: true } or false
end
SGH-->>CLI: { isHookChanged }
alt isHookChanged === true
CLI->>U: log per-hook success & "Successfully set all git hooks"
else
CLI-->>U: (no success output)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
simple-git-hooks.js (2)
183-191: Micro: collapse boolean accumulationSlightly simplify and avoid temporary vars.
- const isHookSet = _setHook(hook, config[hook], projectRootPath) - if (isHookSet) { - hasContentsChanged = true - } + hasContentsChanged = _setHook(hook, config[hook], projectRootPath) || hasContentsChanged ... - const isHookRemoved = _removeHook(hook, projectRootPath) - if (isHookRemoved) { - hasContentsChanged = true - } + hasContentsChanged = _removeHook(hook, projectRootPath) || hasContentsChanged
242-243: Silent no-git behavior: good alignment with goalReturning false when no .git is found ensures CLI stays silent. Consider demoting the info log to debug in future if users request stricter silence in CI.
simple-git-hooks.test.js (1)
561-568: Broaden “silent” assertion to cover console.info and validate CLI pathLibrary path is fine, but logs here are via console.info in _setHook; also consider asserting CLI stdout.
- const consoleSpy = jest.spyOn(console, 'log').mockImplementation(() => {}); + const logSpy = jest.spyOn(console, 'log').mockImplementation(() => {}); + const infoSpy = jest.spyOn(console, 'info').mockImplementation(() => {}); await simpleGitHooks.setHooksFromConfig(PROJECT_WITH_CUSTOM_CONF, args); - expect(consoleSpy).not.toHaveBeenCalled(); + expect(logSpy).not.toHaveBeenCalled(); + expect(infoSpy).not.toHaveBeenCalled(); + logSpy.mockRestore(); + infoSpy.mockRestore();Optionally add a true CLI integration variant:
const { execSync } = require('child_process'); const out = execSync(`node ${require.resolve('./cli')} ${args[2] || './git-hooks.js'}`, { cwd: PROJECT_WITH_CUSTOM_CONF, stdio: ['ignore','pipe','pipe'] }).toString(); expect(out).toBe(''); // second run emits nothing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cli.js(1 hunks)simple-git-hooks.js(5 hunks)simple-git-hooks.test.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cli.js (1)
simple-git-hooks.js (1)
hasContentsChanged(179-179)
🔇 Additional comments (6)
simple-git-hooks.js (5)
179-197: Return change summary: API looks good; confirm callers/docssetHooksFromConfig returning { hasContentsChanged } is a clean way to drive CLI silence. Please verify external consumers (if any) weren’t relying on void and update README/API notes accordingly.
254-259: Idempotency check prevents rewrites — niceSkipping write when contents match is correct and avoids chmod churn, FS noise, and flaky diffs.
264-264: Explicit success booleanReturning true only on actual writes keeps the accumulator accurate. Good.
291-303: Remove hook now returns a boolean — goodClear signal to callers; integrates well with hasContentsChanged.
254-259: Unable to verify hook-path resolution for worktrees
I couldn’t find the implementation of the hookPath logic in this codebase to confirm it handles Git worktree “.git” files correctly. Please verify that the default hooks directory is derived from the resolved gitDir (via getGitProjectRoot) rather than always using projectRoot/.git/hooks, while still honoring any core.hooksPath override.cli.js (1)
14-20: Silent success wiring: LGTMOnly logging when hasContentsChanged is true meets the objective with minimal surface area.
toplenboren
left a comment
toplenboren
left a comment
There was a problem hiding this comment.
Hey!
Thanks for adressing this issue :-)
Overall your PR looks very good, but please fix my minor nitpicks :-)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
simple-git-hooks.js (1)
179-194: Good consolidated change tracking; minor naming nit.The isHookChanged aggregator is clear. For consistency with prior discussion, consider aligning naming (Deleted vs Removed).
- } else if (!preserveUnused.includes(hook)) { - const isHookRemoved = _removeHook(hook, projectRootPath) - isHookChanged = isHookChanged || isHookRemoved + } else if (!preserveUnused.includes(hook)) { + const isHookDeleted = _removeHook(hook, projectRootPath) + isHookChanged = isHookChanged || isHookDeleted }
🧹 Nitpick comments (1)
simple-git-hooks.js (1)
237-241: Explicit return object on missing .git root.Clear and non-throwing; aligns with idempotent behavior. Consider gating the info log by a verbosity flag later if you want fully quiet runs in non-git folders.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cli.js(1 hunks)simple-git-hooks.js(5 hunks)simple-git-hooks.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- simple-git-hooks.test.js
🧰 Additional context used
🧬 Code graph analysis (1)
cli.js (2)
simple-git-hooks.js (1)
isHookChanged(179-179)postinstall.js (1)
postinstall(9-34)
🔇 Additional comments (6)
cli.js (1)
14-20: Silent no-change behavior implemented correctly.The CLI now suppresses success output when nothing changed and logs once when changes occur. Matches the PR objective.
simple-git-hooks.js (5)
252-260: Efficient no-op on identical hook content.Early return avoids unnecessary writes. LGTM.
265-268: Return metadata on successful write.The { hookChanged: true, success: true } shape looks good and aligns with the aggregator.
295-295: Doc updated to reflect boolean return.Accurate and helpful.
304-307: Boolean return for _removeHook implemented cleanly.Direct and readable.
191-193: Return shape OK — no changes required.Verified usages: cli.js destructures ({ isHookChanged }); postinstall.js awaits the call but doesn't use the return; tests call it without relying on a boolean. No callers expect a plain boolean.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
simple-git-hooks.js (1)
206-224: Critical: wrong hooks dir when.gitis a file (worktrees/submodules) — may mkdir the wrong path.defaultHooksDirPath is derived from
projectRoot/.git/hooks; when.gitis a file, this is incorrect and can fail or create a bogus path. UsegitRootfromgetGitProjectRoot()and resolve relativecore.hooksPathagainst that.Apply:
-/** - * Returns the absolute path to the Git hooks directory. - * Respects user-defined core.hooksPath from Git config if present; - * otherwise defaults to <gitRoot>/.git/hooks. - * - * @param {string} gitRoot - The absolute path to the Git project root - * @returns {string} - The resolved absolute path to the hooks directory - * @private - */ -function _getHooksDirPath(projectRoot) { - const defaultHooksDirPath = path.join(projectRoot, '.git', 'hooks') +/** + * Returns the absolute path to the Git hooks directory. + * Respects user-defined core.hooksPath; defaults to <gitRoot>/hooks. + * + * @param {string} gitRoot - Absolute path to the .git directory + * @param {string} [projectRoot] - Working tree root (cwd for `git config`) + * @returns {string} + * @private + */ +function _getHooksDirPath(gitRoot, projectRoot = path.dirname(gitRoot)) { + const defaultHooksDirPath = path.join(gitRoot, 'hooks') try { const customHooksDirPath = execSync('git config --local core.hooksPath', { - cwd: projectRoot, + cwd: projectRoot, encoding: 'utf8' }).trim() if (!customHooksDirPath) { return defaultHooksDirPath } - return path.isAbsolute(customHooksDirPath) - ? customHooksDirPath - : path.resolve(projectRoot, customHooksDirPath) + return path.isAbsolute(customHooksDirPath) + ? customHooksDirPath + : path.resolve(gitRoot, customHooksDirPath) } catch { return defaultHooksDirPath } } @@ - const hookDirectory = _getHooksDirPath(projectRoot) + const hookDirectory = _getHooksDirPath(gitRoot, projectRoot) @@ -function _removeHook(hook, projectRoot=process.cwd()) { - const hookDirectory = _getHooksDirPath(projectRoot) +function _removeHook(hook, projectRoot=process.cwd()) { + const gitRoot = getGitProjectRoot(projectRoot) + if (!gitRoot) { + console.info('[INFO] No `.git` root folder found, skipping') + return false + } + const hookDirectory = _getHooksDirPath(gitRoot, projectRoot)Also applies to: 245-247, 300-305
♻️ Duplicate comments (1)
simple-git-hooks.js (1)
266-269: Return object aligns with prior review feedback.
{ hookChanged: true, success: true }is explicit and easy to consume.
🧹 Nitpick comments (2)
simple-git-hooks.js (2)
253-261: Idempotency check is good; consider normalizing EOL to avoid false diffs across OS.Windows vs POSIX line endings can cause unnecessary rewrites.
Apply:
- if (fs.existsSync(hookPath)) { - const existingHook = fs.readFileSync(hookPath, { encoding: 'utf-8' }) - if (existingHook === hookCommand) { + if (fs.existsSync(hookPath)) { + const normalizeEol = (s) => s.replace(/\r\n/g, '\n') + const existingHook = fs.readFileSync(hookPath, { encoding: 'utf-8' }) + if (normalizeEol(existingHook) === normalizeEol(hookCommand)) { return { hookChanged: false, success: true } } }
292-297: JSDoc wording nit.Function removes any hook, not specifically “pre-commit”.
Apply:
-/** - * Removes the pre-commit hook +/** + * Removes a hook file
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
simple-git-hooks.js(5 hunks)
🔇 Additional comments (3)
simple-git-hooks.js (3)
190-194: Return shape looks good.Explicit
{ isHookChanged }is clear and future-proof.
238-241: Graceful return when no git root.Structured
{ hookChanged:false, success:false }is clear. Good.
296-308: Boolean return from _removeHook is appropriate.The new
booleanaligns with how you computeisHookDeleted.
|
Seems that some tests are failing .. lets fix them and we are ready to merge |
|
Thanks for the detailed reviews! I'll take a look and fix it |
|
Hey, good job fixing the tests :-) Merging the PR and including it to the release later this week Thank you for the contribution! |
2 participants
resolves #110
This is just a simple implementation, no CLI args were added. We can take it simple or add more abilities like:
I'm willing to push it further
Summary by CodeRabbit
New Features
Tests
Chores