ignore: correct handling of nested rules overriding wild card unignore#5210
ignore: correct handling of nested rules overriding wild card unignore#5210ethomson merged 1 commit intolibgit2:masterfrom
Conversation
pks-t
left a comment
There was a problem hiding this comment.
Thanks for your PR! The idea seems sensible, and seeing that no tests break is always reassuring with gitignores. I think that we could avoid the newly introduced flag, as I don't see why it shouldn't be set at some point in time. I'm happy to be convinced otherwise if you had specific reasons for it.
src/ignore.c
Outdated
| if (ignore_lookup_in_rules(out, file, &path)) { | ||
| goto cleanup; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this makes sense to me. There cannot be an ignore rule that is invalidated by its parent rules, but the reverse can be true as demonstrated by your tests. The downside is that we need to always go all the way to the top for files that were ignored in the top-level directory, where we could previously immediately abort in the first iteration.
Naturally, correctness outweighs performance though.
There was a problem hiding this comment.
We could try to work out the starting ignore by taking a minimum of current depth or length of ignores. What do you think?
893b8b0 to
bf2d6c1
Compare
problem: filesystem_iterator loads .gitignore files in top-down order. subsequently, ignore module evaluates them in the order they are loaded. this creates a problem if we have unignored a rule (using a wild card) in a sub dir and ignored it again in a level further below (see the test included in this patch). solution: process ignores in reverse order. closes libgit2#4963
|
Seems correct to me. Thanks for the work here @BuddySpike! The ignore functionality is deeply tricky so I'm glad you were able to fix it up for us. |
problem:
filesystem_iterator loads .gitignore files in top-down order.
subsequently, ignore module evaluates them in the order they are loaded.
this creates a problem if we have unignored a rule (using a wild card)
in a sub dir and ignored it again in a level further below (see the test
included in this patch).
solution:
process ignores in reverse order.
closes #4963