Conversation
In tailwindlabs#15941, the intention was to prevent .gitignore files outside of a git repository from taking effect while ensuring global ignore files configured in git (like core.excludesFile) continue to work. However, builder.git_global(false) was explicitly set in the oxide scanner walker, which caused git's global ignore configuration (core.excludesFile / ~/.config/git/ignore) to be completely ignored. This change enables git_global(true) so that global ignore rules configured in git are honored as expected during source detection. Fixes tailwindlabs#20509
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughThe excludes-file parser now accepts quoted Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to The global-ignore change is mergeable with awareness of the test's shared configuration mutation. Isolating that fixture remains a bounded follow-up; no existing test failure or production defect was established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Global Git ignore settings now affect which files are scanned. The change remains within existing filesystem permissions and source-selection behavior, with no demonstrated privilege escalation. The remaining uncertainty is who controls these settings in shared or automated build environments. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/oxide/tests/scanner.rs (1)
2961-2961: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSynchronize the process-wide
GIT_CONFIG_GLOBALmutation.
GIT_CONFIG_GLOBALis process-wide, and global Git-ignore handling reads it duringScanner::new. The configured pattern matches onlyignored-by-global.html. The inspected concurrent scanner fixtures do not contain that basename, and their Git commands are onlygit initcalls with ignored output. The race therefore does not currently change an asserted result or cause a command failure. It can affect a future scan that contains this basename. Isolate this test in a separate process or protect the mutation with a shared lock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 7f764ba3-4601-4e02-b611-e93f5ae31d2d
📒 Files selected for processing (2)
crates/oxide/src/scanner/mod.rscrates/oxide/tests/scanner.rs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
When scanning for source candidates, Oxide previously disabled git global ignore files via
builder.git_global(false).As noted in #15941, the intention was to prevent
.gitignorefiles in parent directories outside of a git repository from taking effect, while keeping global ignore files configured in git working. However, callinggit_global(false)on the walker had the unintended side effect of ignoringcore.excludesFile(and$XDG_CONFIG_HOME/git/ignore) entirely.This change enables
builder.git_global(true)so that ignore patterns configured in Git's global configuration are respected during source scanning.Fixes #20509
Test plan
respects_git_core_excludes_fileincrates/oxide/tests/scanner.rsverifying that files matched by Git'score.excludesFileare skipped.[ci-all]