diff --git a/lore-revision/src/filter.rs b/lore-revision/src/filter.rs index 15ce85f4..495cdba0 100644 --- a/lore-revision/src/filter.rs +++ b/lore-revision/src/filter.rs @@ -215,12 +215,26 @@ pub fn load_view(view_path: impl AsRef) -> Result { }) } +/// Reads a filter file, one rule per line. +/// +/// 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(); 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 +251,33 @@ pub fn load_filter(path: impl AsRef) -> Result { + if negated { + has_include = true; + } else { + has_exclude = true; + } + } + Err(error) => { + // 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/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..eb6e7dff 100644 --- a/lore-revision/tests/filter.rs +++ b/lore-revision/tests/filter.rs @@ -1063,4 +1063,62 @@ 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_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`. 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 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 _; + + // 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 + )); + assert!(filter.excludes( + &RelativePath::new_from_initial_path("Unreal/Intermediate").expect("Path create"), + true + )); + } }