Support commit SHA in --remote-branch option#212
Conversation
|
|
📝 WalkthroughWalkthroughThe pull request modifies the Changes
Possibly related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #212 +/- ##
==========================================
+ Coverage 92.07% 92.16% +0.08%
==========================================
Files 44 44
Lines 2058 2080 +22
Branches 450 458 +8
==========================================
+ Hits 1895 1917 +22
Misses 163 163 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
tests/core/file/gitCommand.test.ts (1)
Line range hint
46-78: Add test cases for branch and commit SHA supportThe current tests only cover the case where branch is undefined. We need additional test cases to verify the new functionality.
Add these test cases:
test('should execute git commands sequence when branch is specified', async () => { const mockExecAsync = vi.fn().mockResolvedValue({ stdout: '', stderr: '' }); const url = 'https://github.com/user/repo.git'; const directory = '/tmp/repo'; const branch = 'feature-branch'; await execGitShallowClone(url, directory, branch, { execAsync: mockExecAsync }); expect(mockExecAsync).toHaveBeenNthCalledWith(1, `git -C ${directory} init`); expect(mockExecAsync).toHaveBeenNthCalledWith(2, `git -C ${directory} remote add origin ${url}`); expect(mockExecAsync).toHaveBeenNthCalledWith(3, `git -C ${directory} fetch --depth 1 origin ${branch}`); expect(mockExecAsync).toHaveBeenNthCalledWith(4, `git -C ${directory} checkout FETCH_HEAD`); }); test('should support commit SHA', async () => { const mockExecAsync = vi.fn().mockResolvedValue({ stdout: '', stderr: '' }); const url = 'https://github.com/user/repo.git'; const directory = '/tmp/repo'; const commitSha = 'abc123def456'; await execGitShallowClone(url, directory, commitSha, { execAsync: mockExecAsync }); expect(mockExecAsync).toHaveBeenNthCalledWith(3, `git -C ${directory} fetch --depth 1 origin ${commitSha}`); }); test('should handle errors in git commands sequence', async () => { const mockExecAsync = vi.fn() .mockResolvedValueOnce({ stdout: '', stderr: '' }) // init succeeds .mockRejectedValueOnce(new Error('Remote add failed')); // remote add fails const url = 'https://github.com/user/repo.git'; const directory = '/tmp/repo'; const branch = 'feature-branch'; await expect(execGitShallowClone(url, directory, branch, { execAsync: mockExecAsync })) .rejects.toThrow('Remote add failed'); });
🧹 Nitpick comments (1)
src/core/file/gitCommand.ts (1)
29-38: Enhance error handling and add loggingThe implementation lacks proper error handling and logging which could make debugging issues difficult, especially for commit SHA support.
Add try-catch blocks and logging:
if(branch){ + try { await deps.execAsync(`git -C ${directory} init`); await deps.execAsync(`git -C ${directory} remote add origin ${url}`); await deps.execAsync(`git -C ${directory} fetch --depth 1 origin ${branch}`); await deps.execAsync(`git -C ${directory} checkout FETCH_HEAD`); await deps.execAsync(`rm -rf ${directory}/.git 2>/dev/null || rmdir /s /q ${directory}\\.git`) + } catch (error) { + logger.error(`Failed to clone repository at branch/commit: ${branch}`, error); + throw error; + } }else{ + try { await deps.execAsync(`git clone --depth 1 ${url} ${directory}`); + } catch (error) { + logger.error('Failed to shallow clone repository', error); + throw error; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/file/gitCommand.ts(1 hunks)tests/core/file/gitCommand.test.ts(2 hunks)
🔇 Additional comments (1)
src/core/file/gitCommand.ts (1)
29-38: Verify the implementation supports commit SHA as intended
The current implementation uses git fetch which should work with commit SHAs, but we should verify this behavior.
Let's verify the git commands work with commit SHAs:
![standard-input[bot]](https://avatars.githubusercontent.com/in/1011034?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/core/file/gitCommand.ts (1)
31-36:⚠️ Potential issueAdd input validation for branch/commit SHA parameter
The branch parameter is directly interpolated into shell commands without validation, making it vulnerable to shell injection. This is especially important now that it accepts commit SHAs.
Add validation before using the branch parameter:
if (branch) { + // Validate branch parameter to prevent shell injection + if (!/^[a-zA-Z0-9._-]+$/.test(branch)) { + throw new Error('Invalid branch name or commit SHA'); + } await deps.execAsync(`git -C ${directory} init`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/file/gitCommand.ts(2 hunks)tests/core/file/gitCommand.test.ts(2 hunks)
🔇 Additional comments (2)
src/core/file/gitCommand.ts (1)
2-3: LGTM: Appropriate imports added for file system operations
The new imports are correctly added to support the .git directory cleanup operations.
tests/core/file/gitCommand.test.ts (1)
47-51: LGTM: Test updated correctly for undefined branch case
The test correctly verifies the behavior when no branch is specified.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cli/actions/remoteAction.ts (1)
76-76: Enhance error messaging for commit SHA scenariosWhile the parameter rename is correct, the error handling could be more specific for commit SHA failures.
export const cloneRepository = async ( url: string, directory: string, remoteBranch?: string, deps = { execGitShallowClone: execGitShallowClone, }, ): Promise<void> => { - logger.log(`Clone repository: ${url} to temporary directory. ${pc.dim(`path: ${directory}`)}`); + const isCommitSha = remoteBranch && /^[0-9a-f]{40}$/i.test(remoteBranch); + logger.log( + `Clone repository: ${url}${remoteBranch ? ` (${isCommitSha ? 'commit' : 'branch'}: ${remoteBranch})` : ''} to temporary directory. ${pc.dim( + `path: ${directory}`, + )}`, + ); logger.log(''); try { await deps.execGitShallowClone(url, directory, remoteBranch); } catch (error) { - throw new RepomixError(`Failed to clone repository: ${(error as Error).message}`); + throw new RepomixError( + `Failed to ${remoteBranch ? `fetch ${isCommitSha ? 'commit' : 'branch'}` : 'clone repository'}: ${ + (error as Error).message + }`, + ); } };Also applies to: 85-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/cli/actions/remoteAction.ts(2 hunks)src/core/file/gitCommand.ts(2 hunks)tests/core/file/gitCommand.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/core/file/gitCommand.test.ts
🔇 Additional comments (2)
src/core/file/gitCommand.ts (2)
31-39: 🛠️ Refactor suggestion
Add error handling for git operations
The git commands could fail for various reasons (network issues, invalid commit SHA, etc.) but there's no specific error handling.
if (remoteBranch) {
+ try {
await deps.execAsync(`git -C ${directory} init`);
await deps.execAsync(`git -C ${directory} remote add origin ${url}`);
await deps.execAsync(`git -C ${directory} fetch --depth 1 origin ${remoteBranch}`);
await deps.execAsync(`git -C ${directory} checkout FETCH_HEAD`);
await fs.rm(path.join(directory, '.git'), { recursive: true, force: true });
+ } catch (error) {
+ // Cleanup partial clone on failure
+ await fs.rm(directory, { recursive: true, force: true });
+ throw new Error(`Git operation failed: ${(error as Error).message}`);
+ }
} else {
+ try {
await deps.execAsync(`git clone --depth 1 ${url} ${directory}`);
+ } catch (error) {
+ throw new Error(`Git clone failed: ${(error as Error).message}`);
+ }
}Likely invalid or redundant comment.
31-36:
Modify implementation to properly support commit SHAs
The current implementation using fetch --depth 1 may fail for commit SHAs as they require the full history to be fetched. For proper commit SHA support:
if (remoteBranch) {
+ // Validate input to prevent shell injection
+ if (!/^[a-zA-Z0-9._-]+$/.test(remoteBranch)) {
+ throw new Error('Invalid branch name or commit SHA');
+ }
+
+ // Determine if remoteBranch is a commit SHA (40 chars hex)
+ const isCommitSha = /^[0-9a-f]{40}$/i.test(remoteBranch);
+
await deps.execAsync(`git -C ${directory} init`);
await deps.execAsync(`git -C ${directory} remote add origin ${url}`);
- await deps.execAsync(`git -C ${directory} fetch --depth 1 origin ${remoteBranch}`);
+ // Fetch without --depth for commit SHAs to ensure the commit is accessible
+ await deps.execAsync(`git -C ${directory} fetch ${isCommitSha ? '' : '--depth 1'} origin ${remoteBranch}`);
await deps.execAsync(`git -C ${directory} checkout FETCH_HEAD`);
await fs.rm(path.join(directory, '.git'), { recursive: true, force: true });Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/core/file/gitCommand.ts (1)
26-26:⚠️ Potential issueAdd input validation for remoteBranch parameter
The remoteBranch parameter needs validation before being used in shell commands to prevent injection attacks.
export const execGitShallowClone = async ( url: string, directory: string, remoteBranch?: string, deps = { execAsync, }, ) => { + if (remoteBranch && !/^[a-zA-Z0-9._-]+$/.test(remoteBranch)) { + throw new Error('Invalid branch name or commit SHA'); + }
🧹 Nitpick comments (1)
src/core/file/gitCommand.ts (1)
31-39: Validate the URL parameterThe URL parameter should be validated to ensure it's a valid git URL.
export const execGitShallowClone = async ( url: string, directory: string, remoteBranch?: string, deps = { execAsync, }, ) => { + // Basic git URL validation + const gitUrlPattern = /^(https?:\/\/|git@|git:\/\/).+\.git$/; + if (!gitUrlPattern.test(url)) { + throw new Error('Invalid git URL format'); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/cli/actions/remoteAction.ts(2 hunks)src/core/file/gitCommand.ts(2 hunks)tests/core/file/gitCommand.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/core/file/gitCommand.test.ts
- src/cli/actions/remoteAction.ts
🔇 Additional comments (3)
src/core/file/gitCommand.ts (3)
2-3: LGTM: Appropriate imports added for file system operations
The new imports are correctly used for handling file system operations in a cross-platform manner.
31-36: 🛠️ Refactor suggestion
Improve git fetch strategy for commit SHAs
The current implementation uses --depth 1 which might not work reliably with commit SHAs if they're not in the latest commits.
if (remoteBranch) {
await deps.execAsync(`git -C ${directory} init`);
await deps.execAsync(`git -C ${directory} remote add origin ${url}`);
- await deps.execAsync(`git -C ${directory} fetch --depth 1 origin ${remoteBranch}`);
+ // Fetch without depth limit if it looks like a SHA
+ const fetchCmd = /^[a-f0-9]{40}$/.test(remoteBranch)
+ ? `git -C ${directory} fetch origin ${remoteBranch}`
+ : `git -C ${directory} fetch --depth 1 origin ${remoteBranch}`;
+ await deps.execAsync(fetchCmd);
await deps.execAsync(`git -C ${directory} checkout FETCH_HEAD`);
await fs.rm(path.join(directory, '.git'), { recursive: true, force: true });Likely invalid or redundant comment.
31-39: 🛠️ Refactor suggestion
Add error handling and cleanup
The implementation should handle errors and clean up partial clones on failure.
+ try {
if (remoteBranch) {
await deps.execAsync(`git -C ${directory} init`);
await deps.execAsync(`git -C ${directory} remote add origin ${url}`);
await deps.execAsync(`git -C ${directory} fetch --depth 1 origin ${remoteBranch}`);
await deps.execAsync(`git -C ${directory} checkout FETCH_HEAD`);
await fs.rm(path.join(directory, '.git'), { recursive: true, force: true });
} else {
await deps.execAsync(`git clone --depth 1 ${url} ${directory}`);
}
+ } catch (error) {
+ // Clean up partial clone on failure
+ await fs.rm(directory, { recursive: true, force: true }).catch(() => {});
+ throw new Error(`Git clone failed: ${(error as Error).message}`);
+ }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/core/file/gitCommand.ts (2)
26-26:⚠️ Potential issueAdd type validation for remoteBranch parameter
While the rename to
remoteBranchis good, the parameter still needs validation to prevent shell injection.Add validation before using the parameter:
+type ValidRemoteBranch = string & { readonly __brand: unique symbol }; + +function validateRemoteBranch(input: string): ValidRemoteBranch { + if (!/^[a-zA-Z0-9._/-]+$|^[0-9a-f]{4,40}$/i.test(input)) { + throw new Error('Invalid branch name or commit SHA'); + } + return input as ValidRemoteBranch; +} export const execGitShallowClone = async ( url: string, directory: string, - remoteBranch?: string, + remoteBranch?: ValidRemoteBranch,
31-48: 🛠️ Refactor suggestionAdd error handling for git operations
The git commands could fail for various reasons (network issues, invalid SHA, etc.) but lack specific error handling.
if (remoteBranch) { + try { await deps.execAsync(`git -C ${directory} init`); await deps.execAsync(`git -C ${directory} remote add origin ${url}`); // Short SHA detection - matches 4-40 character hex string const maybeShortSha = remoteBranch?.match(/^[0-9a-f]{4,39}$/i); if (maybeShortSha) { await deps.execAsync(`git -C ${directory} fetch origin`); await deps.execAsync(`git -C ${directory} checkout ${remoteBranch}`); } else { await deps.execAsync(`git -C ${directory} fetch --depth 1 origin ${remoteBranch}`); await deps.execAsync(`git -C ${directory} checkout FETCH_HEAD`); } + } catch (error) { + await fs.rm(directory, { recursive: true, force: true }); + throw new Error(`Git operation failed: ${(error as Error).message}`); + } } else { + try { await deps.execAsync(`git clone --depth 1 ${url} ${directory}`); + } catch (error) { + throw new Error(`Shallow clone failed: ${(error as Error).message}`); + } }
🧹 Nitpick comments (2)
src/core/file/gitCommand.ts (2)
35-36: Improve SHA detection precisionThe current regex allows 4-39 character hex strings, but Git SHAs are typically 7-40 characters.
- const maybeShortSha = remoteBranch?.match(/^[0-9a-f]{4,39}$/i); + const maybeShortSha = remoteBranch?.match(/^[0-9a-f]{7,40}$/i);
50-51: Add error handling for cleanup operationThe cleanup operation should handle errors gracefully to ensure users are informed of any issues.
// Clean up .git directory - await fs.rm(path.join(directory, '.git'), { recursive: true, force: true }); + try { + await fs.rm(path.join(directory, '.git'), { recursive: true, force: true }); + } catch (error) { + logger.warn(`Failed to cleanup .git directory: ${(error as Error).message}`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/file/gitCommand.ts(2 hunks)
🔇 Additional comments (1)
src/core/file/gitCommand.ts (1)
2-3: LGTM: Improved cross-platform compatibility with Node.js built-ins
The addition of fs/promises and path imports enables better cross-platform file operations, addressing previous feedback about platform-specific commands.
|
Hi, @tranquochuy645 ! Your changes look good, but I wanted to add support for short commit hashes, so I've added a commit to handle that. I've verified that all the following cases now work: # branch
npm run cli-run -- --remote yamadashy/repomix --remote-branch main
# full hash
npm run cli-run -- --remote yamadashy/repomix --remote-branch bc4f6fd6eb1a680c6d88404da5eb0844ee795e6e
# short hash
npm run cli-run -- --remote yamadashy/repomix --remote-branch bc4f6fHow does this look to you? |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/core/file/gitCommand.ts (1)
31-36:⚠️ Potential issueAdd input validation for remoteBranch parameter
The remoteBranch parameter is directly interpolated into shell commands without sanitization, which could allow command injection.
Add validation before using the remoteBranch parameter:
if (remoteBranch) { + // Validate remoteBranch parameter to prevent shell injection + if (!/^[a-zA-Z0-9._/-]+$/.test(remoteBranch)) { + throw new Error('Invalid branch name, tag, or commit SHA'); + } await deps.execAsync(`git -C ${directory} init`);
🧹 Nitpick comments (2)
src/core/file/gitCommand.ts (2)
37-56: Enhance error messages for better debuggingThe short SHA detection and handling logic is well-implemented. Consider improving error messages to help users understand and resolve issues more easily.
- throw err; + const errorMessage = err instanceof Error ? err.message : 'Unknown error'; + throw new Error(`Failed to fetch branch/tag '${remoteBranch}': ${errorMessage}`);- throw err; + const errorMessage = err instanceof Error ? err.message : 'Unknown error'; + throw new Error(`Failed to fetch commit '${remoteBranch}': ${errorMessage}. Possible causes: network issues, authentication failure, or invalid reference.`);
61-62: Add error handling for cleanup operationThe cleanup implementation is cross-platform compatible, but should handle potential errors during .git directory removal.
// Clean up .git directory - await fs.rm(path.join(directory, '.git'), { recursive: true, force: true }); + try { + await fs.rm(path.join(directory, '.git'), { recursive: true, force: true }); + } catch (error) { + logger.warn(`Failed to cleanup .git directory: ${(error as Error).message}`); + // Continue execution as this is not a critical failure + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/file/gitCommand.ts(2 hunks)
🔇 Additional comments (1)
src/core/file/gitCommand.ts (1)
1-3: LGTM! Clean imports and consistent parameter naming.
The changes improve cross-platform compatibility with fs/promises and follow Node.js best practices with the node: prefix. The parameter rename to 'remoteBranch' better reflects its purpose.
Also applies to: 26-26
src/core/file/gitCommand.ts
Outdated
| await deps.execAsync(`git -C ${directory} remote add origin ${url}`); | ||
|
|
||
| // Short SHA detection - matches 4-40 character hex string | ||
| const maybeShortSha = remoteBranch?.match(/^[0-9a-f]{4,39}$/i); |
There was a problem hiding this comment.
Hi @yamadashy,
Thanks for the update!
I tried the fix with short commit hashes, and it works great! However, there's one thing still on my mind. The regex [0-9a-f]{4,39} can match a wide range of branch names (e.g., add1).
In cases where a user has a branch name matching this regex, it could trigger unnecessary fetching of all branches in a large repository, potentially wasting hours. What's more, the user doesn't have an option to speed up the process in this scenario. While this may be rare, it's still a possibility.
I believe using git fetch origin (without --depth 1) as a fallback when git fetch --depth 1 origin ${remoteBranch} fails would be a better solution.
There was a problem hiding this comment.
I have pushed some changes implementing the "fallback" approach.
Let me know your thoughts!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/core/file/gitCommand.ts (1)
31-59: Enhance error handling messages and typesThe implementation of the fallback approach for short SHA detection looks good. Consider these improvements:
- Add more descriptive error messages
- Use custom error types for better error handling
+class GitError extends Error { + constructor(message: string, public readonly command?: string) { + super(message); + this.name = 'GitError'; + } +} if (!maybeShortSha) { - throw err; + throw new GitError( + `Failed to fetch branch "${remoteBranch}": ${(err as Error).message}`, + 'git fetch' + ); } if (!isRefNotfoundError) { - throw err; + throw new GitError( + `Git operation failed: ${(err as Error).message}`, + 'git fetch' + ); }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 49-49: src/core/file/gitCommand.ts#L49
Added line #L49 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/file/gitCommand.ts(2 hunks)tests/core/file/gitCommand.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/core/file/gitCommand.test.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/core/file/gitCommand.ts
[warning] 49-49: src/core/file/gitCommand.ts#L49
Added line #L49 was not covered by tests
🔇 Additional comments (4)
src/core/file/gitCommand.ts (4)
2-3: LGTM: Clean imports for fs operations
The added imports from Node.js built-in modules are appropriate for the new file system operations.
26-26: LGTM: Improved parameter naming
The rename from 'branch' to 'remoteBranch' better reflects the parameter's purpose and usage.
61-62: LGTM: Cross-platform cleanup implementation
The cleanup implementation uses modern Node.js APIs and is cross-platform compatible.
47-50: Add test coverage for non-ref-notfound errors
The error handling for non-ref-notfound errors (line 49) lacks test coverage.
Would you like me to generate test cases covering these error scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 49-49: src/core/file/gitCommand.ts#L49
Added line #L49 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/core/file/gitCommand.ts (1)
31-36:⚠️ Potential issueAdd input validation to prevent shell injection
The code directly interpolates user input into shell commands without sanitization, which could allow command injection attacks.
Add validation before using the remoteBranch parameter:
if (remoteBranch) { + // Validate remoteBranch parameter to prevent shell injection + if (!/^[0-9a-zA-Z._/-]+$/.test(remoteBranch)) { + throw new Error('Invalid branch name, tag, or commit SHA'); + } await deps.execAsync(`git -C ${directory} init`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/file/gitCommand.ts(2 hunks)
🔇 Additional comments (3)
src/core/file/gitCommand.ts (3)
2-3: LGTM: Clean imports using Node.js best practices
The new imports follow Node.js best practices by using the node: prefix for built-in modules.
26-26: LGTM: Clear parameter naming
The rename from branch to remoteBranch better reflects the parameter's purpose and aligns with the changes across the codebase.
63-64: LGTM: Cross-platform cleanup implementation
The cleanup implementation uses Node.js fs API instead of shell commands, ensuring cross-platform compatibility.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/core/file/gitCommand.ts (2)
26-26:⚠️ Potential issueAdd input validation for remoteBranch parameter
The
remoteBranchparameter is directly interpolated into shell commands without sanitization, which could allow shell injection attacks.Add validation before using the parameter:
export const execGitShallowClone = async ( url: string, directory: string, remoteBranch?: string, deps = { execAsync, }, ) => { + // Validate remoteBranch parameter to prevent shell injection + if (remoteBranch && !/^[a-zA-Z0-9._/-]+$/.test(remoteBranch)) { + throw new Error('Invalid branch name or commit SHA'); + }
47-61: 🛠️ Refactor suggestionImprove SHA detection logic
The current SHA detection has several issues:
- The regex
/^[0-9a-f]{4,39}$/iis too permissive (Git requires minimum 7 chars)- The variable name
isNotShortSHAis confusing due to double negative- The fallback fetch could be expensive for large repositories
Apply these improvements:
- // Short SHA detection - matches a hexadecimal string of 4 to 39 characters - // If the string matches this regex, it MIGHT be a short SHA - // If the string doesn't match, it is DEFINITELY NOT a short SHA - const isNotShortSHA = !remoteBranch.match(/^[0-9a-f]{4,39}$/i); - - if (isNotShortSHA) { + // Short SHA detection - matches a hexadecimal string of 7 to 40 characters + // Full SHA is exactly 40 chars, short SHA is 7-39 chars + const isPotentialSHA = remoteBranch.match(/^[0-9a-f]{7,39}$|^[0-9a-f]{40}$/i); + + if (!isPotentialSHA) { // Rethrow error as nothing else we can do throw err; }
🧹 Nitpick comments (1)
src/core/file/gitCommand.ts (1)
66-67: Add error handling for cleanup operationThe cleanup operation should handle errors gracefully, as the .git directory removal might fail.
Add error handling:
// Clean up .git directory - await fs.rm(path.join(directory, '.git'), { recursive: true, force: true }); + try { + await fs.rm(path.join(directory, '.git'), { recursive: true, force: true }); + } catch (error) { + logger.warn('Failed to cleanup .git directory:', (error as Error).message); + // Continue as this is not a critical error + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/file/gitCommand.ts(2 hunks)tests/core/file/gitCommand.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/core/file/gitCommand.test.ts
|
Sorry for the delay in reviewing this! I've rebased and force pushed to keep the branch up to date. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/file/gitCommand.ts (1)
34-45: Enhance error handling clarityThe error handling is robust but could be more explicit about the type of error encountered.
Consider adding more specific error messages:
try { await deps.execAsync(`git -C ${directory} fetch --depth 1 origin ${remoteBranch}`); await deps.execAsync(`git -C ${directory} checkout FETCH_HEAD`); } catch (err: unknown) { + if (!(err instanceof Error)) { + throw new Error('Unknown error during git operations'); + } + // git fetch --depth 1 origin <short SHA> always throws "couldn't find remote ref" error const isRefNotfoundError = - err instanceof Error && err.message.includes(`couldn't find remote ref ${remoteBranch}`); + err.message.includes(`couldn't find remote ref ${remoteBranch}`); if (!isRefNotfoundError) { - // Rethrow error as nothing else we can do + // Rethrow with context for better debugging + throw new Error(`Git operation failed: ${err.message}`); - throw err; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/cli/actions/remoteAction.ts(2 hunks)src/core/file/gitCommand.ts(2 hunks)tests/core/file/gitCommand.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cli/actions/remoteAction.ts
- tests/core/file/gitCommand.test.ts
🔇 Additional comments (4)
src/core/file/gitCommand.ts (4)
2-3: LGTM: Proper imports added for filesystem operations
The new imports for fs/promises and path are correctly added to support the cleanup operations.
26-26: LGTM: Parameter name improved for clarity
Renaming from branch to remoteBranch better indicates the parameter's purpose.
66-67: LGTM: Clean implementation of .git directory cleanup
The cleanup uses modern Node.js APIs (fs.rm) with proper path joining and error handling options.
31-33:
Add input validation for URL and directory parameters
The parameters are directly interpolated into shell commands without sanitization, making them vulnerable to command injection.
Add validation before executing shell commands:
+ // Validate URL format
+ try {
+ new URL(url);
+ } catch {
+ throw new Error('Invalid Git URL format');
+ }
+
+ // Validate and resolve directory path
+ const resolvedPath = path.resolve(directory);
+ if (resolvedPath.includes('..')) {
+ throw new Error('Directory path cannot contain path traversal');
+ }
+
if (remoteBranch) {
- await deps.execAsync(`git -C ${directory} init`);
- await deps.execAsync(`git -C ${directory} remote add origin ${url}`);
+ await deps.execAsync(`git -C ${resolvedPath} init`);
+ await deps.execAsync(`git -C ${resolvedPath} remote add origin ${url}`);Likely invalid or redundant comment.
|
@tranquochuy645 Thanks for your improvements. I'll merge this now. |
|
I added an example of using a commit hash with the --remote-branch flag to make it clearer that this flag supports not just branches but also tags and commit hashes. #232 |
2 participants
Related:
Feature Request: Add Branch Option for Remote Repository Packing #195
Add remote branch option #196
Update README.md #199
The older PR claims to support branch names, tags, and commit hashes, but commit hashes are not actually supported.
Checklist
npm run testnpm run lint