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

[mahirhir]

Fixes #182.

What goes wrong

filter::load_filter reads the ignore file line by line and propagates the rule error with ?:

if negated {
    filter.add_inclusion(glob)?;
} else {
    filter.add_exclusion(glob)?;
}

So one rule the filter cannot express aborts the read and discards every rule in the file, including the ones before the offending line — and it does so with nothing naming the line at fault. repository::load_filter then turns that Err into None:

if let Ok(filter) = filter::load(&ignore_path, &view_path) { Some(...) } else { None }

with no log at any level, so the repository ends up with no filter at all and the only signal reaching the user is that everything they meant to ignore shows up as changed. In #182 that was **/DerivedDataCache/ going dead because a later line said !**/Plugins/*/Binaries/.

The change

The refusal itself is untouched — add_inclusion still rejects an unanchored multi-component re-inclusion, and unanchored_multi_component_inclusion_is_refused still passes. What changes is that the failure now says which line caused it instead of vanishing:

.loreignore:2: unsupported rule `!**/Plugins/*/Binaries/`: filter inclusions
cannot start with ** as that will force traversal of the entire revision tree

• filter::load_filter raises a hard error naming the path, the 1-based line number, and the rule as written. Per review (@mjansson): Lore hard-errors on invalid config rather than skipping it, and a filter that kept some rules and dropped others is not a smaller version of what the author wrote but a different filter, with no way for the reader to tell which one they got.
• The line info is in the error message, not in FilterError::internal_with_context. Display for Traced forwards to the inner error and never renders the trace, so a context string would have recorded the line somewhere no reader of the error looks. There is a comment at the call site saying so.
• repository::load_filter logs the failure instead of swallowing it, for the errors that can still come out of filter::load.

This gives the reporter's items 1 and 2. Item 3 — documenting which gitignore subset .loreignore supports — is not in this PR; happy to follow up if you want it in docs/.

Still open, deliberately outside this diff

repository::load_filter returns Option<Arc<Filter>> and repository.rs:2122 does .unwrap_or_default(), so the hard error is raised, logged with the line, and then turned into an empty filter one level up. Propagating it is Option -> Result through that call site; say the word and it goes in here, or in a separate PR.

Verification

unsupported_rule_fails_the_load_and_names_the_line in lore-revision/tests/filter.rs writes the reporter's file plus a third rule after the bad line, asserts the load fails, and asserts the message names both the line and the rule as written. a_file_of_supported_rules_still_loads beside it is the positive control, so a failure is attributable to the unsupported rule rather than to every load.

cargo test -p lore-revision --test filter
test result: ok. 12 passed; 0 failed

Negative control — tests kept, lore-revision/src/filter.rs reverted to warn-and-skip:

test tests::unsupported_rule_fails_the_load_and_names_the_line ... FAILED
test tests::a_file_of_supported_rules_still_loads ... ok
test result: FAILED. 11 passed; 1 failed

[mjansson]

General approach in Lore is that we don't skip over invalid configs and data but hard error - silently (which is basically what you get in an integration toolchain, a warning will just be printed in some log somewhere) skipping over invalid entries mean the filter no longer represents what the intent was, and the resulting partial filter will do potentially completely different inclusions/exclusions than the intention was. Arguably as bad as no filter.

I would prefer a hard error but extended with the line info as suggested.

[mahirhir]

Done — 9e54260 makes it a hard error and names the line.

.loreignore:2: unsupported rule `!**/Plugins/*/Binaries/`: filter inclusions
cannot start with ** as that will force traversal of the entire revision tree

Your reasoning holds where mine did not: a filter that kept some of its rules and dropped others is not a smaller version of what the author wrote, it is a different filter, and nobody can tell which one they got by looking at the output. I was weighing it against "no filter at all" and treating a warning as the thing that closes the gap, which is the part you named — in an integration toolchain that warning is a line in a log nobody opens.

One implementation note, since it decides whether the line number is worth anything. I first put it in FilterError::internal_with_context, which is what that constructor is for, and then checked what it prints: Display for Traced forwards to the inner error and never renders the trace, so the file and line would have been recorded in a field no reader of the error ever sees. It is in the message instead. There is a comment at the call site saying so, since the next person will reach for internal_with_context too.

Tests: the regression test now asserts the load fails and that the message names both the line and the rule as written. I added a positive control beside it — a file of supported rules still loads — so a failure is attributable to the unsupported rule rather than to every load. cargo test -p lore-revision --test filter is 12 passed. Negative control, tests kept and src reverted to warn-and-skip: only the new test goes red, the positive control stays green.

unanchored_multi_component_inclusion_is_refused is untouched and still passes; the refusal itself never changed, only what happens to the rest of the file.

One thing I did not do, because it is wider than this PR. repository::load_filter returns Option<Arc<Filter>> and the caller at repository.rs:2122 does .unwrap_or_default(). So the hard error is raised, logged with the line, and then converted to an empty filter — which lands the caller in the same "silently filtering nothing" state you are objecting to, one level up. Making that propagate is Option -> Result through that call site and I did not want to widen the diff into it without asking. Say the word and I will do it here, or open it separately against #182.

Item 3 from the report — documenting which gitignore subset .loreignore supports — is still open. With the error now naming the rule and the reason, a docs/ section listing the refusals seems worth having; happy to write it if you want it.