fix(sparse): support brace patterns in includes #25
Conversation
📝 WalkthroughWalkthroughReplace in-file Git env construction with exported Changes
Sequence Diagram(s)(Skipped — changes are algorithmic and localized; no new multi-component sequential flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
commit: |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for brace pattern expansion in sparse-checkout include patterns, specifically for file extension patterns like **/*.{md,mdx,txt}. Before this change, such patterns would be passed directly to git sparse-checkout, which doesn't support brace expansion natively. The implementation expands these patterns into individual patterns (e.g., **/*.md, **/*.mdx, **/*.txt) before passing them to git.
Changes:
- Added
expandBracePattern()function to expand simple brace patterns in file extension globs - Modified
normalizeSparsePatterns()to apply brace expansion to all patterns - Added comprehensive tests for brace expansion in sparse-checkout scenarios
- Added integration test to verify default patterns work with real repositories
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| src/git/fetch-source.ts | Implements brace pattern expansion for file extensions in sparse-checkout patterns |
| tests/sparse-brace-expansion.test.js | Adds unit tests for brace pattern expansion with mocked git commands |
| tests/integration-real-repos.test.js | Adds integration test verifying default pattern works with actual repository |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🤖 Fix all issues with AI agents
In `@tests/sparse-brace-expansion.test.js`:
- Around line 165-267: The test "sync expands default brace pattern when no
include specified" duplicates setup/teardown and repeats the same git shim PATH
mutation that causes CI flakes; extract a shared helper (e.g.,
createTestContext) to perform tmp dir creation, mkdir for binDir and cachePath,
writeGitShim(binDir, logPath) and writeFile(logPath, ""), and another helper
(e.g., withModifiedPath) to set/unset
process.env.PATH/Path/PATHEXT/DOCS_CACHE_GIT_DIR around running runSync; replace
the duplicated inline setup/cleanup in this test (and the earlier test that
mirrors lines 62-109) to call createTestContext and wrap the run in
withModifiedPath so the git shim is always present and env is restored in
finally.
- Around line 61-163: The resolve-remote.ts code calls execFileAsync("git", ...)
without the modified PATH, so the git shim in tests isn't used; import or
otherwise share the buildGitEnv function used by fetch-source.ts and pass its
result as the env option to execFileAsync calls in resolveRemoteCommit (and any
other execFileAsync invocations in resolve-remote.ts) so the spawned git process
uses the test binDir PATH; alternatively extract buildGitEnv to a small shared
helper module and use that from both fetch-source.ts and resolve-remote.ts.
🧹 Nitpick comments (1)
tests/sparse-brace-expansion.test.js (1)
142-149: Assertion assumes fixed argument order afterset.The slice
args.slice(patternIndex + 2)assumes--no-conealways immediately followsset. If the argument order changes (e.g., patterns before flags, or additional flags), this assertion could break silently.Consider a more robust approach:
// Filter to just the pattern-like arguments const patterns = args.slice(patternIndex + 1).filter(arg => !arg.startsWith("--"));
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/git/fetch-source.ts`:
- Around line 262-297: In expandBracePattern (and its inner expand function) the
MAX_BRACE_EXPANSIONS check happens after pushing to results, allowing an
off-by-one; before calling results.push(value) ensure you check if
results.length >= MAX_BRACE_EXPANSIONS and throw the same Error (using
MAX_BRACE_EXPANSIONS and pattern) so the limit is enforced strictly; apply this
pre-push check in both places where results.push(value) is used inside expand to
prevent exceeding the cap.
In `@src/git/git-env.ts`:
- Around line 24-25: Replace the invalid environment variable key
GIT_CONFIG_NOGLOBAL with the correct GIT_CONFIG_GLOBAL and set its value to
"/dev/null" in the git environment object (the entries containing
GIT_CONFIG_NOSYSTEM and GIT_CONFIG_NOGLOBAL); update the symbol
GIT_CONFIG_NOGLOBAL to GIT_CONFIG_GLOBAL and assign "/dev/null" so Git global
config is effectively disabled.
🧹 Nitpick comments (1)
src/git/git-env.ts (1)
6-22: Redundant re-assignments after spreadingprocess.env.Lines 10-22 explicitly re-assign variables that are already included via
...process.envon line 7. These assignments have no effect unless the intent is to document which variables are preserved. If documentation is the goal, consider adding a comment instead.♻️ Simplified version (if explicit re-assignment isn't needed)
return { ...process.env, ...(pathValue ? { PATH: pathValue, Path: pathValue } : {}), ...(pathExtValue ? { PATHEXT: pathExtValue } : {}), - HOME: process.env.HOME, - USER: process.env.USER, - USERPROFILE: process.env.USERPROFILE, - TMPDIR: process.env.TMPDIR, - TMP: process.env.TMP, - TEMP: process.env.TEMP, - SYSTEMROOT: process.env.SYSTEMROOT, - WINDIR: process.env.WINDIR, - SSH_AUTH_SOCK: process.env.SSH_AUTH_SOCK, - SSH_AGENT_PID: process.env.SSH_AGENT_PID, - HTTP_PROXY: process.env.HTTP_PROXY, - HTTPS_PROXY: process.env.HTTPS_PROXY, - NO_PROXY: process.env.NO_PROXY, GIT_TERMINAL_PROMPT: "0", GIT_CONFIG_NOSYSTEM: "1", - GIT_CONFIG_NOGLOBAL: "1", + GIT_CONFIG_GLOBAL: "", ...(process.platform === "win32" ? {} : { GIT_ASKPASS: "/bin/false" }), };
Windows child_process.execFile() doesn't always respect PATH when resolving bare command names like 'git'. This caused tests to invoke the real git.exe instead of the git.cmd shim. Solution: - Add resolveGitCommand() helper that checks DOCS_CACHE_GIT_COMMAND env var - Export and use resolveGitCommand() in fetch-source.ts and resolve-remote.ts - Set DOCS_CACHE_GIT_COMMAND to absolute path in test's withModifiedPath() - Ensures Windows CI uses git.cmd shim instead of system git Fixes sparse-brace-expansion.test.js on Windows CI.
Add comprehensive review guidelines for implementing PR feedback: - Cross-platform testing requirements - Code quality expectations - Testing best practices (DOCS_CACHE_GIT_COMMAND override) - Documentation requirements These guidelines capture lessons learned from PR #25 feedback implementation to help maintain code quality and consistency.
1 participant
Summary
Expand brace syntax patterns like
**/*.{md,mdx,txt}for git sparse-checkout compatibility.Changes
expandBracePattern()to convert brace patterns to individual extensionsnormalizeSparsePatterns()to call expansion during normalizationSummary by CodeRabbit