fix(linter): use require_git(true) to properly respect nested git repo boundaries#20951
fix(linter): use require_git(true) to properly respect nested git repo boundaries#20951fabb wants to merge 4 commits into
Conversation
github-actions
Bot
added
A-linter
76affdf to
c72d032
Compare
c72d032 to
089f596
Compare
…o boundaries Switches `require_git(false)` to `require_git(true)` in the oxlint and oxfmt walkers. **The problem:** `require_git(false)` (introduced in oxc-project#17375) makes the `ignore` crate read `.gitignore` files from directories *above* the git root, which differs from standard git behaviour. This causes a concrete bug in nested git repositories: `parent_repo/.gitignore` incorrectly filters files inside `parent_repo/child_repo/`, which has its own `.git`. **Why `require_git(true)` fixes it:** The `ignore` crate already implements correct git-boundary logic via a `saw_git` flag in `matched_ignore`. When iterating parent directories from innermost to outermost, `saw_git` becomes `true` at the innermost `.git` boundary, preventing gitignore rules from any outer repository from applying. `require_git(true)` activates this standard git behaviour. **Alternative discarded workarounds:** `parents(false)` was a workaround idea for the nested-repo bug. That would cause a regression where `oxlint src/` would not pick up `.gitignore` rules from the repo root above `src/`. **Trade-off:** `.gitignore` rules no longer apply when there is no `.git` ancestor anywhere in the directory tree. This reverts the behaviour added in oxc-project#17375. Reverts oxc-project#17375
089f596 to
db743f7
Compare
There was a problem hiding this comment.
Regarding these changes (require_git: true+parents: true), I don't have any objections to making them at least for Oxfmt, given that this reflects the default behavior for ignore crate.
It won't match the behavior of Prettier CLI exactly, but since it doesn't match perfectly even now, I don't think it's a problem if it improves.
However, I am concerned about whether the #20952 that sets require_git dynamically is actually necessary.
There was a problem hiding this comment.
If we set require_git(true), I think we should change parents(true) as well.
There was a problem hiding this comment.
true, i don’t know why i forgot it here.
There was a problem hiding this comment.
fixed. parents(false) was there to match prettier (which only searches cwd), but since we already diverge from prettier with require_git(true) anyway, parents(true) is the consistent choice — require_git stops upward traversal at git boundaries so outer repos can't bleed in
require_git(true) stops upward traversal at git boundaries, making parents(true) safe and consistent with oxlint's walk settings.
|
massive apologies for the delay on this, but thank you for submitting this PR! I think i've fixed this in a slightly more ideal way, in #22033, that maintains the previous behaviour, while allowiing .gitignores in non-git initialized directories. The fix should be released on Monday (May 4th) (or possibly tuesday due to the holiday) - please try it and let me know! Thanks! |
3 participants
Alternative approach: #20952 avoids reverting #17375 by detecting at runtime whether the paths are inside a git repo and setting
require_gitdynamically.Summary
Switches
require_git(false)→require_git(true)in the oxlint and oxfmt walkers.The problem:
require_git(false)(introduced in #17375) makes theignorecrate read.gitignorefiles from directories above the git root, which differs from standard git behaviour. This causes a concrete bug in nested git repositories:parent_repo/.gitignoreincorrectly filters files insideparent_repo/child_repo/, which has its own.git.Why
require_git(true)fixes it: Theignorecrate already implements correct git-boundary logic via asaw_gitflag inmatched_ignore. When iterating parent directories from innermost to outermost,saw_gitbecomestrueat the innermost.gitboundary, preventing gitignore rules from any outer repository from applying.require_git(true)activates this standard git behaviour.Alternative discarded workarounds:
parents(false)in #17813 was a workaround idea for the nested-repo bug. That would cause a regression whereoxlint src/would not pick up.gitignorerules from the repo root abovesrc/.Trade-off:
.gitignorerules no longer apply when there is no.gitancestor anywhere in the directory tree. This reverts the behaviour added in #17375.Fixes #17805
Fixes #17510
Reverts #17375
Supersedes #17813