feat(core): respect git info exclude files#10121
Conversation
🦋 Changeset detectedLatest commit: ec1b930 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Git’s local exclude ( 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_service/src/workspace/server.rs (1)
163-178: Tiny tidy-up: collapse.ok()+let Someintolet Ok.Reads a touch nicer and saves one hop.
♻️ Suggested simplification
fn resolve_git_common_dir(fs: &dyn FsWithResolverProxy, git_dir: &Utf8Path) -> Utf8PathBuf { - let Some(content) = fs - .read_file_from_path(git_dir.join("commondir").as_ref()) - .ok() - else { + let Ok(content) = fs.read_file_from_path(git_dir.join("commondir").as_ref()) else { return git_dir.to_path_buf(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/workspace/server.rs` around lines 163 - 178, The pattern in resolve_git_common_dir uses fs.read_file_from_path(...).ok() followed by let Some(content) which can be simplified to use let Ok(content) = fs.read_file_from_path(...) directly; update the early-return branch to match on Ok and keep the existing return of git_dir.to_path_buf() on Err, preserving the subsequent trimming/map/unwrap_or_else logic that calls resolve_git_path so the behavior of resolve_git_common_dir and interaction with FsWithResolverProxy and resolve_git_path remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_service/src/workspace/server.rs`:
- Around line 163-178: The pattern in resolve_git_common_dir uses
fs.read_file_from_path(...).ok() followed by let Some(content) which can be
simplified to use let Ok(content) = fs.read_file_from_path(...) directly; update
the early-return branch to match on Ok and keep the existing return of
git_dir.to_path_buf() on Err, preserving the subsequent
trimming/map/unwrap_or_else logic that calls resolve_git_path so the behavior of
resolve_git_common_dir and interaction with FsWithResolverProxy and
resolve_git_path remains identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25f651b8-6be5-466e-972f-4506d257e7fe
⛔ Files ignored due to path filters (2)
crates/biome_cli/tests/snapshots/main_cases_vcs_ignored_files/ignore_git_common_info_exclude.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_cases_vcs_ignored_files/ignore_git_info_exclude.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
.changeset/respect-git-info-exclude.mdcrates/biome_cli/tests/cases/vcs_ignored_files.rscrates/biome_service/src/workspace/server.rs
jongwan56
force-pushed
the
fix/git-info-exclude
branch
from
31311f2 to
fa839fc
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_cli/tests/cases/vcs_ignored_files.rs (1)
140-179: Worktree indirection scenario looks good.The
.gitfile →gitdir→commondirchain is wired correctly (actual.git/worktrees/current+../..resolving toactual.git), exercising the new common-dir resolution path well.One small optional thought: a third test where both
.gitignoreand.git/info/excludeare absent/empty would lock in the "emitNoIgnoreFileFoundonly when all sources are empty" branch from the server-side change. Not a blocker —should_fail_when_ignore_file_is_absentalready partially covers it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_cli/tests/cases/vcs_ignored_files.rs` around lines 140 - 179, Add an additional test that verifies the NoIgnoreFileFound emission only occurs when all ignore sources are absent/empty: create a test analogous to ignore_git_common_info_exclude (e.g., ignore_no_sources_emits_no_ignore) that sets up a repo with vcs enabled and git clientKind but with both .gitignore and actual.git/info/exclude missing or empty and without other ignore sources, run run_cli_with_dyn_fs with the same Args::from(["format", "--write", ...]) pattern, and assert the CLI result and snapshot expect the NoIgnoreFileFound behavior; this complements should_fail_when_ignore_file_is_absent and ensures the server-side "all sources empty" branch is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_cli/tests/cases/vcs_ignored_files.rs`:
- Around line 140-179: Add an additional test that verifies the
NoIgnoreFileFound emission only occurs when all ignore sources are absent/empty:
create a test analogous to ignore_git_common_info_exclude (e.g.,
ignore_no_sources_emits_no_ignore) that sets up a repo with vcs enabled and git
clientKind but with both .gitignore and actual.git/info/exclude missing or empty
and without other ignore sources, run run_cli_with_dyn_fs with the same
Args::from(["format", "--write", ...]) pattern, and assert the CLI result and
snapshot expect the NoIgnoreFileFound behavior; this complements
should_fail_when_ignore_file_is_absent and ensures the server-side "all sources
empty" branch is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 09eebe88-1c53-4d23-9ccb-079cebb6f941
⛔ Files ignored due to path filters (2)
crates/biome_cli/tests/snapshots/main_cases_vcs_ignored_files/ignore_git_common_info_exclude.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_cases_vcs_ignored_files/ignore_git_info_exclude.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
.changeset/respect-git-info-exclude.mdcrates/biome_cli/tests/cases/vcs_ignored_files.rscrates/biome_service/src/workspace/server.rs
✅ Files skipped from review due to trivial changes (1)
- .changeset/respect-git-info-exclude.md
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_service/src/workspace/server.rs
|
Would you mind elaborating a bit more on how |
|
This PR would close #4822 |
|
Thanks, that makes sense. I added comments around the Git directory resolution. Git treats The worktree case matters because a linked worktree's I also updated the changeset and PR description to reference #4822. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_service/src/workspace/server.rs`:
- Around line 1356-1386: The pull request changes when the NoIgnoreFileFound
diagnostic is emitted by adding read_git_info_exclude(...) into the ignore-file
detection path, so update the repo changelog: add a
.changeset/respect-git-info-exclude.md entry that notes the behavioral change
(NoIgnoreFileFound now only fires if .gitignore, .ignore, and .git/info/exclude
are all missing/empty), reference the affected logic by name
(read_git_info_exclude, NoIgnoreFileFound, and store_root_ignore_patterns) and
briefly explain the user-visible impact for consumers who relied on the previous
diagnostic behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a0acc62d-9748-4223-bde4-b12ef8d3f171
📒 Files selected for processing (2)
.changeset/respect-git-info-exclude.mdcrates/biome_service/src/workspace/server.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/respect-git-info-exclude.md
There was a problem hiding this comment.
Thank for this, users will appreciate it.
However, this PR isn't a bug fix, but a new feature, because the VCS integration will do more than before, by default.
We need:
- to update the changeset to be a
minorand reword it entirely. Changeset are for our end-users, so we need to remove all technical words - update the documentation of the CLI arguments in the source code, specially the
use_ignore_filefield - send a PR to
biomejs/websiterepository and update the section that talks about VCS integration - send the PR towards the
nextbranch (this one and the one of the website)
We're close to our next minor, so let us know if you need help, or you can do everything yourself
| let gitdir = line.strip_prefix("gitdir:")?.trim(); | ||
| Some(resolve_git_path(directory, gitdir)) | ||
| }), | ||
| Err(_) => Some(dot_git), |
There was a problem hiding this comment.
This is incorrect. It should return None. We shouldn't pretend that .git actually exists
There was a problem hiding this comment.
Updated. resolve_git_dir now returns None when .git is neither a readable gitdir: file nor an actual directory, so Biome no longer assumes a missing .git path exists.
| { | ||
| ignore_file_contents.push(content); | ||
| } | ||
|
|
There was a problem hiding this comment.
I would prefer if read_git_info_exclude wouldn't pollute ignore_file_contents, and instead it would read and parse the content, and would return the lines we need. It seems duplicate work, but it would make things well separated.
There was a problem hiding this comment.
Updated. read_git_info_exclude_patterns now reads and parses .git/info/exclude separately and returns the patterns, so the root ignore file contents stay separate.
jongwan56
force-pushed
the
fix/git-info-exclude
branch
from
58d8c5b to
ec1b930
Compare
jongwan56
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/biome_cli/tests/cases/vcs_ignored_files.rs (1)
156-159: Optional: a one-liner explaining the worktree fixture would be lovely.The fixture here is doing some clever sleight of hand —
.gitis a file pointing at a per-worktree gitdir,commondirthen bounces back up to the sharedactual.git, whereinfo/excludeactually lives. Given the same question came up in PR review, a short comment in the test would save the next reader a Git-internals deep dive.📝 Suggested comment
+ // Simulate a linked worktree: `.git` is a file pointing at the worktree's + // gitdir, whose `commondir` resolves back to the shared repository + // directory where `info/exclude` lives. fs.create_file(".gitignore", ""); fs.create_file(".git", "gitdir: actual.git/worktrees/current\n"); fs.create_file("actual.git/worktrees/current/commondir", "../..\n"); fs.create_file("actual.git/info/exclude", "file2.js\n");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_cli/tests/cases/vcs_ignored_files.rs` around lines 156 - 159, Add a one-line comment above the test fixture that creates ".gitignore", ".git", "actual.git/worktrees/current/commondir", and "actual.git/info/exclude" explaining that ".git" is a worktree pointer file pointing to a per-worktree gitdir (actual.git/worktrees/current), that "commondir" points back to the shared repository (../..) so the real excludes live in "actual.git/info/exclude", and therefore the test uses this layout to simulate Git worktree behavior for ignore resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/respect-git-info-exclude.md:
- Line 2: Update the changeset entry that currently reads `"@biomejs/biome":
minor` to use the patch change type instead (i.e. `"@biomejs/biome": patch`)
because this PR is a bug fix targeting main (resolves `#4822`); locate the
changeset file containing the string `"@biomejs/biome": minor` and modify only
the change type token from minor to patch.
- Line 5: Update the changeset description to start with the required prefix
"Fixed [`#4822`](issue link):" so it follows the bug-fix convention; edit the
.changeset/respect-git-info-exclude.md content and prepend "Fixed [`#4822`](...):
" (replace ... with the actual issue URL) before the existing sentence about
applying Git's local exclude file.
---
Nitpick comments:
In `@crates/biome_cli/tests/cases/vcs_ignored_files.rs`:
- Around line 156-159: Add a one-line comment above the test fixture that
creates ".gitignore", ".git", "actual.git/worktrees/current/commondir", and
"actual.git/info/exclude" explaining that ".git" is a worktree pointer file
pointing to a per-worktree gitdir (actual.git/worktrees/current), that
"commondir" points back to the shared repository (../..) so the real excludes
live in "actual.git/info/exclude", and therefore the test uses this layout to
simulate Git worktree behavior for ignore resolution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 903a2678-e0c9-4e0e-bcb1-840db323ecd1
⛔ Files ignored due to path filters (2)
crates/biome_cli/tests/snapshots/main_cases_vcs_ignored_files/ignore_git_common_info_exclude.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_cases_vcs_ignored_files/ignore_git_info_exclude.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
.changeset/respect-git-info-exclude.mdcrates/biome_cli/tests/cases/vcs_ignored_files.rscrates/biome_configuration/src/vcs.rs
✅ Files skipped from review due to trivial changes (1)
- crates/biome_configuration/src/vcs.rs
| "@biomejs/biome": minor | ||
| --- | ||
|
|
||
| Biome now applies Git's local exclude file when VCS ignore files are enabled. Files listed in `.git/info/exclude` are skipped the same way as files listed in `.gitignore`, including in linked worktrees. |
There was a problem hiding this comment.
Lead with the Fixed [#4822](...) prefix.
Since issue #4822 exists for this bug, the changeset description should start with the standard Fixed [#NUMBER](issue link): ... form so it lines up with the rest of the changelog.
🩹 Suggested fix
-Biome now applies Git's local exclude file when VCS ignore files are enabled. Files listed in `.git/info/exclude` are skipped the same way as files listed in `.gitignore`, including in linked worktrees.
+Fixed [`#4822`](https://github.com/biomejs/biome/issues/4822): Biome now applies Git's local exclude file when VCS ignore files are enabled. Files listed in `.git/info/exclude` are skipped the same way as files listed in `.gitignore`, including in linked worktrees.As per coding guidelines: "For bug fixes, start with 'Fixed [#NUMBER](issue link): ...'."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Biome now applies Git's local exclude file when VCS ignore files are enabled. Files listed in `.git/info/exclude` are skipped the same way as files listed in `.gitignore`, including in linked worktrees. | |
| Fixed [`#4822`](https://github.com/biomejs/biome/issues/4822): Biome now applies Git's local exclude file when VCS ignore files are enabled. Files listed in `.git/info/exclude` are skipped the same way as files listed in `.gitignore`, including in linked worktrees. |
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: There seems to be a noun/verb agreement error. Did you mean “excludes” or “excluded”?
Context: ...inor --- Biome now applies Git's local exclude file when VCS ignore files are enabled....
(SINGULAR_NOUN_VERB_AGREEMENT)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/respect-git-info-exclude.md at line 5, Update the changeset
description to start with the required prefix "Fixed [`#4822`](issue link):" so it
follows the bug-fix convention; edit the .changeset/respect-git-info-exclude.md
content and prepend "Fixed [`#4822`](...): " (replace ... with the actual issue
URL) before the existing sentence about applying Git's local exclude file.
3 participants
Fixes #4822
Summary
Makes
vcs.useIgnoreFileinclude repository-local Git exclude patterns from.git/info/exclude. Linked worktrees are handled by resolvinggitdirandcommondir.Test Plan
just fjust lcargo test -p biome_cli --test main vcs_ignored_files -- --nocaptureDocs
No documentation changes.