From dd23f8b655bbd0070fb1ec8e13c9668f451dc905 Mon Sep 17 00:00:00 2001 From: Mahiro Hirakawa Date: Thu, 3 Sep 2026 05:30:18 +0900 Subject: [PATCH 1/2] fix(filter): one unsupported rule no longer discards the whole ignore file load_filter propagated the error from add_inclusion/add_exclusion with ?, so a single rule the filter cannot express aborted the read and dropped every rule in the file, including the ones before it. repository::load_filter then turned that Err into None with no diagnostic, leaving the repository with no filter at all. The only signal reaching the user was that everything they meant to ignore showed up as changed. Report the offending line and skip it, keeping the rest of the file, and log the load failure in repository::load_filter instead of swallowing it. The refusal itself is unchanged: an unanchored multi-component re-inclusion is still rejected, it just no longer takes the file down with it. Fixes #182 Signed-off-by: Mahiro Hirakawa --- lore-revision/src/filter.rs | 41 ++++++++++++++++++++++++++++----- lore-revision/src/repository.rs | 15 ++++++++---- lore-revision/tests/filter.rs | 38 ++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 10 deletions(-) diff --git a/lore-revision/src/filter.rs b/lore-revision/src/filter.rs index 15ce85f4..2a4c4518 100644 --- a/lore-revision/src/filter.rs +++ b/lore-revision/src/filter.rs @@ -215,12 +215,25 @@ pub fn load_view(view_path: impl AsRef) -> Result { }) } +/// Reads a filter file, one rule per line. +/// +/// A rule this filter cannot express is reported and skipped, and the rest of +/// the file still applies. Aborting instead would discard every rule in the +/// file, including the ones before the offending line, and a filter that +/// silently excludes nothing is far more damaging than one missing rule: the +/// only signal the author gets is that everything they meant to ignore turns up +/// as changed. pub fn load_filter(path: impl AsRef) -> Result { + let path = path.as_ref(); let mut filter = FilterInstance::default(); if let Ok(file) = File::open(path) { let mut has_include = false; let mut has_exclude = false; - for line in BufReader::new(file).lines().map_while(Result::ok) { + for (index, line) in BufReader::new(file) + .lines() + .map_while(Result::ok) + .enumerate() + { let mut glob = line.trim(); if glob.is_empty() || glob.starts_with('#') { continue; @@ -237,12 +250,28 @@ pub fn load_filter(path: impl AsRef) -> Result { + if negated { + has_include = true; + } else { + has_exclude = true; + } + } + Err(error) => { + lore_warn!( + "{}:{}: ignoring unsupported rule `{}`: {error}. The remaining rules in this file still apply.", + path.display(), + index + 1, + line.trim(), + ); + } } } diff --git a/lore-revision/src/repository.rs b/lore-revision/src/repository.rs index c2bd15fb..8648ac49 100644 --- a/lore-revision/src/repository.rs +++ b/lore-revision/src/repository.rs @@ -2531,10 +2531,17 @@ pub fn load_filter(root_path: &Path) -> Option> { let view_path = root_path.join(format.dot_dir()).join(VIEW_FILTER); - if let Ok(filter) = filter::load(&ignore_path, &view_path) { - Some(Arc::new(filter)) - } else { - None + match filter::load(&ignore_path, &view_path) { + Ok(filter) => Some(Arc::new(filter)), + Err(error) => { + // Returning `None` here means nothing is filtered at all, which looks + // to the user like every ignored file suddenly changed. Say why. + lore_warn!( + "Failed to load the filter from {}: {error}. Nothing will be ignored until this is fixed.", + ignore_path.display(), + ); + None + } } } diff --git a/lore-revision/tests/filter.rs b/lore-revision/tests/filter.rs index 9ead9074..af1d22c2 100644 --- a/lore-revision/tests/filter.rs +++ b/lore-revision/tests/filter.rs @@ -1063,4 +1063,42 @@ mod tests { true )); } + + /// A rule the filter cannot express must not take the rest of the file with + /// it. Regression for the `.loreignore` that stopped ignoring anything at + /// all because one line held an unanchored multi-component re-inclusion. + #[test] + fn unsupported_rule_does_not_discard_the_file() { + use std::io::Write as _; + + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join(".loreignore"); + let mut file = std::fs::File::create(&path).expect("create"); + // Line 2 is valid .gitignore but unsupported by lore: an unanchored + // multi-component re-inclusion, refused by `add_inclusion`. + file.write_all(b"**/DerivedDataCache/\n!**/Plugins/*/Binaries/\n**/Intermediate/\n") + .expect("write"); + drop(file); + + let filter = lore_revision::filter::load_filter(&path).expect("load"); + + // The rule before the unsupported line still applies. + assert!(filter.excludes( + &RelativePath::new_from_initial_path("Unreal/DerivedDataCache").expect("Path create"), + true + )); + // So does the rule after it. + assert!(filter.excludes( + &RelativePath::new_from_initial_path("Unreal/Intermediate").expect("Path create"), + true + )); + // The unsupported rule itself is skipped, not honoured. + assert!( + filter.excludes( + &RelativePath::new_from_initial_path("Unreal/DerivedDataCache/VT") + .expect("Path create"), + true + ) + ); + } } From 3688e650feff0de7155fee77302a4c30e8e1c08f Mon Sep 17 00:00:00 2001 From: Mahiro Hirakawa Date: Thu, 3 Sep 2026 21:19:10 +0900 Subject: [PATCH 2/2] fix(filter): make an unsupported rule a hard error naming the line Per review: Lore hard-errors on invalid config rather than skipping it, and a partial filter no longer represents the author's intent, so warn-and-skip is replaced with a hard error carrying the file, the 1-based line number and the rule as written. The line info goes in the error message, not in internal_with_context: Display for Traced forwards to the inner error and never prints the trace, so a context string would name the line somewhere the reader never looks. The regression test now asserts the load fails and that the message names both the line and the rule. A positive control alongside it asserts a file of supported rules still loads, so the failure is attributable to the unsupported rule rather than to every load. Verification: cargo test -p lore-revision --test filter -> 12 passed. Negative control, tests kept and src reverted to warn-and-skip: the new test fails and the positive control stays green. Signed-off-by: Mahiro Hirakawa --- lore-revision/src/filter.rs | 24 +++++++++++------- lore-revision/tests/filter.rs | 46 +++++++++++++++++++++++++---------- 2 files changed, 48 insertions(+), 22 deletions(-) diff --git a/lore-revision/src/filter.rs b/lore-revision/src/filter.rs index 2a4c4518..495cdba0 100644 --- a/lore-revision/src/filter.rs +++ b/lore-revision/src/filter.rs @@ -217,12 +217,13 @@ pub fn load_view(view_path: impl AsRef) -> Result { /// Reads a filter file, one rule per line. /// -/// A rule this filter cannot express is reported and skipped, and the rest of -/// the file still applies. Aborting instead would discard every rule in the -/// file, including the ones before the offending line, and a filter that -/// silently excludes nothing is far more damaging than one missing rule: the -/// only signal the author gets is that everything they meant to ignore turns up -/// as changed. +/// A rule this filter cannot express is a hard error, and the error names the +/// file, the 1-based line number and the rule as written. Skipping the line +/// instead would leave a filter that no longer represents the author's intent +/// and whose inclusions and exclusions may differ arbitrarily from what was +/// meant, which is at least as bad as having no filter; and the only signal a +/// warning gives is a line in a log the author of an integration toolchain +/// never reads. pub fn load_filter(path: impl AsRef) -> Result { let path = path.as_ref(); let mut filter = FilterInstance::default(); @@ -265,12 +266,17 @@ pub fn load_filter(path: impl AsRef) -> Result { - lore_warn!( - "{}:{}: ignoring unsupported rule `{}`: {error}. The remaining rules in this file still apply.", + // The line info goes in the message rather than in + // `internal_with_context`: `Display for Traced` forwards to + // the inner error and never prints the trace, so a context + // string would name the line somewhere the reader of the + // error never looks. + return Err(FilterError::internal(format!( + "{}:{}: unsupported rule `{}`: {error}", path.display(), index + 1, line.trim(), - ); + ))); } } } diff --git a/lore-revision/tests/filter.rs b/lore-revision/tests/filter.rs index af1d22c2..eb6e7dff 100644 --- a/lore-revision/tests/filter.rs +++ b/lore-revision/tests/filter.rs @@ -1068,37 +1068,57 @@ mod tests { /// it. Regression for the `.loreignore` that stopped ignoring anything at /// all because one line held an unanchored multi-component re-inclusion. #[test] - fn unsupported_rule_does_not_discard_the_file() { + fn unsupported_rule_fails_the_load_and_names_the_line() { use std::io::Write as _; let dir = tempfile::tempdir().expect("temp dir"); let path = dir.path().join(".loreignore"); let mut file = std::fs::File::create(&path).expect("create"); // Line 2 is valid .gitignore but unsupported by lore: an unanchored - // multi-component re-inclusion, refused by `add_inclusion`. + // multi-component re-inclusion, refused by `add_inclusion`. Line 3 is + // valid and sits after it, so a load that reports success would have to + // have carried on past the refusal. file.write_all(b"**/DerivedDataCache/\n!**/Plugins/*/Binaries/\n**/Intermediate/\n") .expect("write"); drop(file); - let filter = lore_revision::filter::load_filter(&path).expect("load"); + let error = lore_revision::filter::load_filter(&path) + .expect_err("an unsupported rule must fail the load, not be skipped"); + + // The message has to be enough to fix the file without bisecting it: + // which file, which line, and the rule as the author wrote it. + let message = error.to_string(); + assert!( + message.contains(&format!("{}:2:", path.display())), + "error must name the file and the 1-based line: {message}" + ); + assert!( + message.contains("!**/Plugins/*/Binaries/"), + "error must quote the rule as written: {message}" + ); + } + + #[test] + fn a_file_of_supported_rules_still_loads() { + use std::io::Write as _; - // The rule before the unsupported line still applies. + // Positive control for the test above: the failure has to come from the + // unsupported rule, not from every load of a file. + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join(".loreignore"); + let mut file = std::fs::File::create(&path).expect("create"); + file.write_all(b"**/DerivedDataCache/\n**/Intermediate/\n") + .expect("write"); + drop(file); + + let filter = lore_revision::filter::load_filter(&path).expect("load"); assert!(filter.excludes( &RelativePath::new_from_initial_path("Unreal/DerivedDataCache").expect("Path create"), true )); - // So does the rule after it. assert!(filter.excludes( &RelativePath::new_from_initial_path("Unreal/Intermediate").expect("Path create"), true )); - // The unsupported rule itself is skipped, not honoured. - assert!( - filter.excludes( - &RelativePath::new_from_initial_path("Unreal/DerivedDataCache/VT") - .expect("Path create"), - true - ) - ); } }