Stop the excluded directory rule from firing on a re-included ancestor - #144
KaizenShogun wants to merge 3 commits into
Conversation
The directory bucket now only accepts matches on a strict ancestor, and the rule only fires once the ancestor is confirmed excluded by the whole spec, asked outermost first. Fixes part B of cpburnz#137.
98e0378 to
c842c94
Compare
| assert pattern.regex is not None, pattern | ||
| for dir_match in pattern.regex.finditer(file): | ||
| if dir_match.groupdict().get(_DIR_MARK) is None: | ||
| continue | ||
| elif dir_match.end(_DIR_MARK) < len(file): | ||
| is_ancestor = True | ||
| else: | ||
| is_self = True |
There was a problem hiding this comment.
This can be simplified. for dir_match in pattern.regex.finditer(file) will only yield a single match that is exactly the same as match.match above.
-
if dir_match.groupdict().get(_DIR_MARK) is None:can never eval to true. -
elif dir_match.end(_DIR_MARK) < len(file):and theelse:can be pulled out of the loop, and the loop eliminated.
There was a problem hiding this comment.
Partly: the is None check was dead and is gone. The loop itself is needed for **/, which compiles to the unanchored (?P<ps_d>/): on a/b/, finditer matches at the ancestor and at the directory itself, while match only returns the first. Replacing the loop with match.match fails test_02_dir_reinclusion_whitelist. I added a comment saying so.
Ancestors are asked outermost first, so when one is asked every ancestor above it is already known not to be excluded. Asking again through match_file() made each level re-ask all the levels above it: with ['d0/', '!/d0/', 'd0/', '!d*/', 'd0/d1/', '!d*/**'] a path 20 levels deep took 1,048,576 match calls; it now takes 21. Also drop the unreachable None check in the simple backend's finditer loop, and note why the loop is needed: '**/' compiles to the unanchored '(?P<ps_d>/)', which matches both an ancestor and the directory itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
2 participants
Fixes part B of #137 — the excluded-directory rule fires on an ancestor that the same spec re-includes.
dir_includeis resolved among patterns that produce a_DIR_MARKmatch, while whether an ancestor ends up excluded is decided by every pattern that matches it.!**/node_modules/**compiles without a dir mark, so it never displaces.*from the directory bucket even though it re-includes the very ancestor.*excluded.Two changes per backend: the directory bucket only accepts matches on a strict ancestor, and the rule only fires once
_ancestor_excluded()confirms that ancestor is excluded by the whole spec — asked outermost first, the order git stops descending in.One thing worth knowing before you read the diff. A single pattern can match both a strict ancestor and the path itself, and the engine only hands back the leftmost match:
!*/againstsub/d/returnssub/, so classifying on that one match makes!*/an ancestor exclusion and never the directory it re-includes — which breakstest_02_dir_reinclusion_whitelist. So the simple backend asks for every separator. In re2/hyperscan the same split is{base}/?$rather than a third expression per pattern, which costs ~9x.Measured on
f0fb3f4,GitIgnoreSpec, all three backends, git 2.55.0 as the oracle:tests/Sweep and per-check columns are simple / re2 / hyperscan. The sweep is 96 two-pattern specs over 13 queries, each verdict taken from a real repository —
check-ignore --stdinfor files, a canary probe for directories, sincecheck-ignore d/answers itself. Bench and corpus: https://github.com/KaizenShogun/gitignore-conformance — happy to run any variant you'd rather have through it.The new test's verdicts were taken two ways that agree on every row:
check-ignore -vand whatgit add -Aactually stages.@youdie006 reproduced the bug independently on git 2.43.0 (#137 comment) with the same verdicts I get on 2.55.0, so none of this is version skew.
— Midas