Fix GitIgnoreSpec re-including files under an excluded directory (#129) - #132
Open
youdie006 wants to merge 1 commit into
Open
Fix GitIgnoreSpec re-including files under an excluded directory (#129)#132youdie006 wants to merge 1 commit into
youdie006 wants to merge 1 commit into
Conversation
GitIgnoreSpec resolves patterns with a flat last-match, so a file-level
negation could re-include a file whose parent directory is excluded, which git
forbids ("It is not possible to re-include a file if a parent directory of that
file is excluded"). For example ["build", "!keep.log"] wrongly treated
build/keep.log as not-ignored, while real git check-ignore ignores it.
Wrap the gitignore backend (_AncestorDirBackend): when a file is not already
ignored by its own resolution, walk the file's ancestor directory prefixes and,
for each, ask whether that directory is excluded, resolved as a directory
(ancestor + "/") using git's plain last-match order via util.check_match_file.
If any ancestor directory is excluded, the file is ignored regardless of a later
file-level negation. Resolving the ancestor as a directory preserves
directory-level re-inclusions, so "build/*" + "!build/keep.log" still
re-includes (build/* excludes only the contents, not the build directory
itself), as do the !libfoo/!libfoo/** and !*/ idioms.
A small _wrap_backend extension point is added on PathSpec (a no-op by default,
overridden by GitIgnoreSpec) so the wrapper applies uniformly, including through
the internal test backend factory.
Fixes cpburnz#129.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #129. Thanks to @eeshsaxena for the clear report and the directory-vs-contents distinction -- it made this straightforward to reproduce.
Problem
GitIgnoreSpecresolves patterns with a flat last-match, so a file-level negation can re-include a file whose parent directory is excluded, which git forbids ("It is not possible to re-include a file if a parent directory of that file is excluded"):Proposed approach (open to a different design)
I want to be upfront that this is a core match-resolution change, so please treat the approach as a proposal -- I am happy to restructure it however you prefer.
The fix wraps the gitignore backend (
_AncestorDirBackend). When a file is not already ignored by its own resolution, it walks the file's ancestor directory prefixes and, for each, asks whether that directory is excluded -- resolved as a directory (ancestor + "/") using git's plain last-match order via the existingutil.check_match_file. If any ancestor directory is excluded, the file is ignored regardless of a later file-level negation.Resolving the ancestor as a directory (rather than reusing the leaf-file resolution) is what keeps directory-level re-inclusions working, so this correctly distinguishes:
build+!keep.logbuild/keep.logbuild/*+!build/keep.logbuild/keep.logbuild/*only excludes the contents (it does not match thebuilddirectory itself), so re-inclusion is preserved -- that case, plus the*+!libfoo+!libfoo/**whitelist idiom (test_08_issue_81), the!*/"scan all directories" idiom (test_07_issue_74), and!*.yaml/(test_02_issue_41), all still behave as before.Notes / tradeoffs I would value your opinion on:
check_match_file) for the ancestor directory resolution rather than the compiled re2/hyperscan path, so it is backend-agnostic but does not use the fast combined regex for those extra directory lookups._test_backend_factoryhook used by the tests), I added a small_wrap_backendextension point onPathSpec(a no-op by default, overridden byGitIgnoreSpec). If you would rather fold the check in elsewhere -- e.g. inside each backend, or only in_make_backend-- I am glad to change it.Tests
Added
test_10_issue_129_{a,b,c}totests/test_06_gitignore.py, running across all backends viaparameterize_from_lines. Verified red-green: with the fix reverted thebuild/keep.loganda/keep.logignore cases fail across every backend; with the fix they pass, and thebuild/*re-inclusion case passes both ways. Expected results confirmed againstgit check-ignore(2.54.0). Full suite (200 passed) and the strict docs build stay green.This change was prepared with AI assistance and reviewed by me before submission.