fix the issue when gitRoot is not projectRoot #133
Conversation
|
| const hookDirectory = _getHooksDirPath(projectRoot) | ||
| let finalCommand = command; | ||
| if(hookDirectory !== path.join(projectRoot, '.git', 'hooks')) { | ||
| finalCommand = `pushd . && cd ${projectRoot} && ${command} && popd` |
There was a problem hiding this comment.
looking into it, i think the failing test comes from this and the test needs to be adapted? The test expects this command without changes in the resulting hook file, but finds it with this
other than that, the fix looks good to me
|
@coderabbitai wanna make a review? |
|
@toplenboren I'll review the changes to see how this PR fixes the gitRoot vs projectRoot issue. ✅ Actions performedReview triggered.
|
WalkthroughThe changes update internal logic for resolving the Git hooks directory in the script. The hooks directory is now determined relative to the Git project root, with early exits and info logs if no Git root or hooks directory is found. Hook command execution context is adjusted when hooks are outside the default location. Tests were added to cover monorepo scenarios and verify command wrapping for non-default hooks paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll take a look at this PR! Let me examine the changes to understand how you've fixed the gitRoot vs projectRoot issue. 🏁 Script executed: Length of output: 3218 @toplenboren Great work on fixing this gitRoot vs projectRoot issue! I've reviewed the changes and the approach looks solid. Here's my analysis: ✅ What's Working Well:
🤔 Potential Considerations:
// Instead of shell commands, consider:
const originalCwd = process.cwd();
try {
process.chdir(projectRoot);
// Execute command here
} finally {
process.chdir(originalCwd);
}
Let me check how 🏁 Script executed: Length of output: 2051 Now I understand the issue! After examining the 🚨 Critical Issue Found:The const defaultHooksDirPath = path.join(gitRoot, 'hooks') // This would be `.git/hooks` But since Actually, looking at the diff again, I can see that your implementation is correct! The ✅ Correction - The Implementation is Actually Correct:Your fix properly handles the git directory structure. The logic flow is:
🔍 Let me verify with a test scenario:The implementation looks solid overall! The fix correctly addresses the issue where hooks were being created in the wrong location when the git root and project root differ. Summary:
The only minor suggestion I have is about the cross-platform compatibility of Great job fixing this issue! 🐰✨ 🏁 Script executed: Length of output: 104 🏁 Script executed: Length of output: 180 Let me examine the tests to understand the current coverage and see if any tests need to be updated for this fix: 🏁 Script executed: Length of output: 28112 After examining the test suite, I can see that your fix addresses a real gap in both functionality and test coverage! ✅ Current Test Analysis:All existing tests assume that
🧪 Missing Test Coverage:Your fix addresses a critical scenario that isn't tested at all. I strongly recommend adding a test case like this: Based on the test structure, here's what I'd recommend adding: describe("Git root vs Project root scenarios", () => {
it("creates git hooks in correct location when git root differs from project root", async () => {
// Create a scenario like issue #132:
// - .git (git root)
// - npm_project (project root with package.json)
const GIT_ROOT = path.join(testsFolder, "monorepo_git_root");
const PROJECT_ROOT = path.join(GIT_ROOT, "npm_project");
// Setup git root
if (!fs.existsSync(GIT_ROOT)) fs.mkdirSync(GIT_ROOT);
if (!fs.existsSync(GIT_ROOT + "/.git")) fs.mkdirSync(GIT_ROOT + "/.git");
if (!fs.existsSync(GIT_ROOT + "/.git/hooks")) fs.mkdirSync(GIT_ROOT + "/.git/hooks");
// Setup npm project in subdirectory
if (!fs.existsSync(PROJECT_ROOT)) fs.mkdirSync(PROJECT_ROOT);
fs.writeFileSync(path.join(PROJECT_ROOT, "package.json"), JSON.stringify({
"simple-git-hooks": { "pre-commit": "exit 1" }
}));
// Run from project root (subdirectory)
await simpleGitHooks.setHooksFromConfig(PROJECT_ROOT);
// Hooks should be installed in git root, not project root
const gitRootHooks = getInstalledGitHooks(path.join(GIT_ROOT, ".git", "hooks"));
const projectRootHooks = fs.existsSync(path.join(PROJECT_ROOT, ".git")) ?
getInstalledGitHooks(path.join(PROJECT_ROOT, ".git", "hooks")) : {};
expect(Object.keys(gitRootHooks)).toEqual(["pre-commit"]);
expect(Object.keys(projectRootHooks)).toEqual([]);
// Cleanup
fs.rmSync(GIT_ROOT, { recursive: true, force: true });
});
});🏃♂️ Next Steps:
Your fix is solid and addresses a real user pain point. The only thing missing is test coverage for this scenario! 🐰💭 🏁 Script executed: Length of output: 4060 🏁 Script executed: Length of output: 840 Perfect! The test project structures confirm my analysis - all current tests assume the project root contains the Since I can't run the tests in this sandbox environment (jest isn't available), but based on my code analysis, here's my final assessment: ✅ Overall Assessment: LGTM!Your fix correctly addresses the gitRoot vs projectRoot issue:
🧪 Testing Recommendations:For you to run locally: npm test # Ensure no regressionsMissing test case to add: // Add this to simple-git-hooks.test.js
describe("Monorepo scenarios", () => {
it("installs hooks in git root when executed from project subdirectory", async () => {
const MONOREPO_ROOT = path.join(testsFolder, "monorepo_scenario");
const PROJECT_SUBDIR = path.join(MONOREPO_ROOT, "npm_project");
// Setup: Git root with npm project in subdirectory
fs.mkdirSync(MONOREPO_ROOT, { recursive: true });
fs.mkdirSync(path.join(MONOREPO_ROOT, ".git", "hooks"), { recursive: true });
fs.mkdirSync(PROJECT_SUBDIR, { recursive: true });
fs.writeFileSync(path.join(PROJECT_SUBDIR, "package.json"), JSON.stringify({
"simple-git-hooks": { "pre-commit": "exit 1" }
}));
// Execute from subdirectory
await simpleGitHooks.setHooksFromConfig(PROJECT_SUBDIR);
// Verify hooks installed in git root, not project subdirectory
const gitRootHooks = getInstalledGitHooks(path.join(MONOREPO_ROOT, ".git", "hooks"));
expect(Object.keys(gitRootHooks)).toContain("pre-commit");
// Verify no .git created in subdirectory
expect(fs.existsSync(path.join(PROJECT_SUBDIR, ".git"))).toBe(false);
// Cleanup
fs.rmSync(MONOREPO_ROOT, { recursive: true, force: true });
});
});🚀 Ready to Merge:This fix solves a real user pain point and the implementation is solid. Once you've:
This PR will be ready to merge! Great work on identifying and fixing this issue! 🐰✨ |
![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 (1)
simple-git-hooks.test.js (1)
744-772: Excellent test coverage for the core monorepo scenario.This test directly validates the main fix described in the PR objectives - ensuring hooks are installed in the git root's
.git/hooksdirectory when executed from a project subdirectory. The test setup, execution, and verification logic are all sound.Consider these minor enhancements:
- it("installs hooks in git root when executed from project subdirectory", async () => { + it("installs hooks in git root when executed from project subdirectory and wraps commands with directory change", async () => {And add verification of the command wrapping:
// Verify hooks installed in git root, not project subdirectory const gitRootHooks = getInstalledGitHooks(path.join(MONOREPO_ROOT, ".git", "hooks")); expect(Object.keys(gitRootHooks)).toContain("pre-commit"); + + // Verify hook contains directory change wrapper + const expectedWrappedCommand = wrapCommandWithDirectoryChange(PROJECT_SUBDIR, "exit 1"); + expect(gitRootHooks["pre-commit"]).toContain(expectedWrappedCommand);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
simple-git-hooks.js(3 hunks)simple-git-hooks.test.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- simple-git-hooks.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
simple-git-hooks.test.js (1)
simple-git-hooks.js (3)
dir(74-74)path(2-2)fs(1-1)
🔇 Additional comments (2)
simple-git-hooks.test.js (2)
137-141: LGTM! Helper function accurately reflects the main application logic.This helper function correctly implements the directory change wrapping logic that mirrors the behavior in the main
simple-git-hooks.jswhen hooks are installed outside the default.git/hookspath. The use ofpushd,cd, andpopdensures commands execute in the correct directory context.
528-534: Correctly updated test to verify command wrapping for custom hooks paths.The test modification appropriately changes from exact equality to containment checking, which is necessary now that hook scripts include directory change wrapper commands when installed outside the default
.git/hookspath. This ensures the test validates the core functionality while being flexible about the exact script format.
4 participants
Fix the issue 132
There is a bug in the install script when the gitRoot is not the same as the projectRoot. My Project looks like:
npx simple-git-hookshereresult:
expectation:
Summary by CodeRabbit