fix(filter): an unsupported rule in .loreignore is a hard error naming the line

lore-revision/src/filter.rs

@@ -215,12 +215,26 @@ pub fn load_view(view_path: impl AsRef<Path>) -> Result<Filter, FilterError> {
     })
 }
 
+/// 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<Path>) -> Result<FilterInstance, FilterError> {
+    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<Path>) -> Result<FilterInstance, FilterError
                 glob = &glob[1..];
             }
 
-            if negated {
-                filter.add_inclusion(glob)?;
-                has_include = true;
+            let added = if negated {
+                filter.add_inclusion(glob)
             } else {
-                filter.add_exclusion(glob)?;
-                has_exclude = true;
+                filter.add_exclusion(glob)
+            };
+
+            match added {
+                Ok(()) => {
+                    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(),
+                    )));
+                }
             }
         }
 

lore-revision/src/repository.rs

@@ -2531,10 +2531,17 @@ pub fn load_filter(root_path: &Path) -> Option<Arc<filter::Filter>> {
 
     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
+        }
     }
 }
 

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
+        ));
+    }
 }