Skip to content

fix(glob): retain valid gitignore rules after parse errors - #36478

Merged
nathanwhit merged 2 commits into
denoland:mainfrom
nathanwhit:fix/gitignore-parse-errors
Aug 17, 2026
Merged

fix(glob): retain valid gitignore rules after parse errors#36478
nathanwhit merged 2 commits into
denoland:mainfrom
nathanwhit:fix/gitignore-parse-errors

Conversation

@nathanwhit

@nathanwhit nathanwhit commented Aug 7, 2026

Copy link
Copy Markdown
Member

Summary

  • retain valid .gitignore patterns when another line cannot be parsed
  • identify the malformed file and line in a warning
  • cover root, nested, and publish dry-run behavior

Details

Gitignore loading previously returned no matcher after the first malformed pattern. That discarded valid patterns from the same file, including patterns parsed before the malformed line.

The loader now skips only the malformed line, reports its location, and continues building the matcher from the remaining patterns. Parent-directory matchers continue to compose with nested files as before.

Validation

  • cargo fmt -p deno_config -- --check
  • cargo test -p deno_config
  • cargo test -p specs_tests --test specs publish::dry_run_invalid_gitignore
  • cargo clippy -p deno_config --all-targets -- -D warnings

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The .ok()? inside the and_then closure discarding the whole file β€” including patterns parsed before the bad line β€” was clearly the bug, and skipping just the offending line is the minimal correct fix. Line numbering is right, the result is memoized per directory so the warning fires once, and the "Warning: " string prefix matches what the rest of libs/config does rather than the colors::yellow convention used where colors are available. The spec test mirrors publish/dry_run_gitignored closely and is a real regression test β€” pre-fix both before.txt and after.txt would show up in the published file list.

Please fix lint before merging β€” lint debug is failing on linux, macOS and Windows with Found 1 not formatted file. It's just the two new assert!(...) calls in libs/config/glob/gitignore.rs needing to wrap; a ./tools/format.js run should do it. Approving on the assumption that's all it is, so it can go in as soon as CI is green.

One thing worth a changelog line: this is user-visible for deno publish. Previously a malformed .gitignore meant nothing was ignored and extra files shipped; now the valid patterns take effect, so files that shipped in a prior release can be excluded β€” and for a module in the package's graph that becomes a hard error[excluded-module] rather than a quietly smaller tarball. Correct behavior, but someone whose publish worked last release can start failing on a .gitignore line they've had for years. The warning does name the file and line, which helps.

Two nits, neither blocking: the message reads "invalid .gitignore pattern at /path/.gitignore:2", so ".gitignore" shows up twice β€” "skipping invalid pattern at {}:{}" would be tighter. And the include_paths loop just below still uses let _ignore = builder.add_line(...), so identical failures are now handled two different ways a few lines apart; a one-line comment noting those patterns are internally generated (so a failure there is a bug, not user error) would save the next reader the detour.

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear improvement. builder.add_line(None, line).ok()? discarded the entire .gitignore on the first unparseable pattern β€” and silently, so a single stray [ would quietly un-ignore everything in that directory. For deno publish that means files the user believed were ignored get uploaded, which is the failure mode worth caring about.

Skipping just the offending line and warning is the right behavior, and the warning carries the path, 1-based line number, and the underlying error, so it's actionable.

The two unit tests are well chosen β€” one covers patterns on both sides of the bad line, the other covers that a bad child .gitignore doesn't take the parent's rules down with it, which is the case the recursion made easy to get wrong. The publish spec test is the right end-to-end check since that's where the consequence is visible. LGTM.

@nathanwhit
nathanwhit enabled auto-merge (squash) August 17, 2026 19:57
@nathanwhit
nathanwhit merged commit ea07bc6 into denoland:main Aug 17, 2026
269 of 271 checks passed
@nathanwhit
nathanwhit deleted the fix/gitignore-parse-errors branch August 17, 2026 19:58
bartlomieju pushed a commit that referenced this pull request Aug 27, 2026
## Summary

- retain valid `.gitignore` patterns when another line cannot be parsed
- identify the malformed file and line in a warning
- cover root, nested, and publish dry-run behavior

## Details

Gitignore loading previously returned no matcher after the first
malformed pattern. That discarded valid patterns from the same file,
including patterns parsed before the malformed line.

The loader now skips only the malformed line, reports its location, and
continues building the matcher from the remaining patterns.
Parent-directory matchers continue to compose with nested files as
before.

## Validation

- `cargo fmt -p deno_config -- --check`
- `cargo test -p deno_config`
- `cargo test -p specs_tests --test specs
publish::dry_run_invalid_gitignore`
- `cargo clippy -p deno_config --all-targets -- -D warnings`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants