fix(glob): retain valid gitignore rules after parse errors - #36478
Conversation
bartlomieju
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
## 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`
Summary
.gitignorepatterns when another line cannot be parsedDetails
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 -- --checkcargo test -p deno_configcargo test -p specs_tests --test specs publish::dry_run_invalid_gitignorecargo clippy -p deno_config --all-targets -- -D warnings