fix(gitignore): escape directory names before interpolating into pattern position - #1809
Open
AmirF194 wants to merge 2 commits into
Open
fix(gitignore): escape directory names before interpolating into pattern position#1809AmirF194 wants to merge 2 commits into
AmirF194 wants to merge 2 commits into
Conversation
…ern position GitignoreParser rescopes patterns from nested .gitignore files by prefixing the containing directory's relative path, but interpolates that path into pattern position without escaping it. A directory whose name contains a gitignore metacharacter (*, ?, [, ], !, #) is then read as glob syntax instead of a literal name, so a directory like a stray "***" venv can turn a scoped pattern into one matching most or all of the project. Fixes oraios#1806
The escaping added in the prior commit inserts literal backslash escape characters before pattern metacharacters. On Windows, os.sep is also backslash, so os.path.join(rel_dir_pattern, line) mixed path-separator backslashes with escape backslashes, and the pre-existing trailing `.replace(os.sep, "/")` normalization step then converted ALL of them indiscriminately -- turning an escaped 'a\[1\]' into 'a/[1/]', which no longer matches the literal directory name. Verified with ntpath (Windows path semantics) on this Linux session: simulating a Windows relpath through the old os.sep/os.path.join/replace chain produces 'a/[1/]/mod.py', which pathspec does not match against 'a[1]/mod.py'; the new '/'-only construction produces 'a\[1\]/mod.py', which matches. Fixed by normalizing rel_dir to '/' immediately and joining every pattern component with a literal '/', removing the now-unneeded (and on Windows actively harmful) blanket separator replace. Also marks the two directory-creation regression tests that use '*'/'?' literally in a directory name as Windows-skipped (those characters are illegal in Windows filenames, so the directories cannot be created there -- a platform limitation of the test, not the fix), and adds a pure-function test for _escape_gitignore_path_component so the '*'/'?' escaping logic still has cross-platform coverage. Found via this PR's own Windows CI run (jobs 91787560091/91787560092/91787560183 on run 30843966565), which failed test_gitignore_dir_name_with_metachars_anchored_pattern with an assertion failure (not the OSError the other two tests hit), before any maintainer had looked at the PR.
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.
Root cause
GitignoreParser._parse_gitignore_contentrescopes patterns from a nested.gitignorebyjoining the gitignore directory's relative path (
rel_dir) onto each pattern line (threejoin sites,
src/serena/util/file_system.py:263,269,272at HEAD29d07d4f).rel_dircomesfrom
os.path.relpathand is a filesystem name, but it lands directly in pattern position,where
pathspec'sGitWildMatchPatternreads*,?,[,],!and#as syntax ratherthan literal characters.
A directory whose name contains one of those characters (the concrete case in the issue: a
stray venv literally named
***, whose own.gitignoreis just*) turns a scoped patterninto a much broader one:
***/**/*reads as "any first segment, then anything at depth >= 2",so it ends up excluding nearly the whole project. Indexing then reports 0 files with exit code
0, and nothing points at the actual cause.
Invariant
A pattern segment built from a filesystem name must match that name literally, never as glob
syntax. The fix escapes each path component of
rel_dir(backslash before\ * ? [ ] ! #,following
pathspec's own escaping convention) at all three join sites before it is foldedinto the pattern string, so directory names are always matched literally regardless of their
content.
Verification
test/serena/util/test_file_system.py, one per join site(anchored,
**/-prefixed non-anchored, and the implicit-**non-anchored case), plus theissue's own
***scenario. Ran in a cleanpython:3.11-slimcontainer against HEAD29d07d4f: all three fail on unmodifiedmainand pass on this branch.uv run poe test test/serena/util/test_file_system.py: 27/27 pass (24 existing + 3 new).uv run poe lintanduv run poe type-check: clean.codespellon the changed files: clean.test/serena/util/test_exception.py's three pre-existing GUI-dependent failures, which areunrelated to this change and reproduce identically on unmodified
mainin this container.Fixes #1806
Checklist
CONTRIBUTING.mdregarding the scope of PRs.CHANGELOG.md, which concisely describes the change.Update (second commit)
The initial Windows CI run (before any review) failed 3 new tests, not the 3 unrelated
pre-existing flakes documented above:
test_gitignore_dir_name_with_metachars_anchored_patternfailed with a real
AssertionError(not the platform-limitationOSErrorthe other twometachar tests hit for
*/?, which are illegal in Windows filenames). Root cause: theescaping added above inserts literal backslash escape characters, and on Windows
os.sepisalso backslash, so
os.path.join(rel_dir_pattern, line)mixed path-separator backslashes withescape backslashes; the pre-existing trailing
.replace(os.sep, "/")then converted all ofthem indiscriminately, turning an escaped
a\\[1\\]intoa/[1/], which no longer matchesthe literal directory name.
Fixed by building every pattern with a literal
/from the start (neveros.sep/os.path.join), and removing the now-unneeded (and on Windows actively harmful) blanketseparator replace. Verified with
ntpath(Windows path semantics) on this Linux session:simulating the old chain on a Windows-style relpath produces
a/[1/]/mod.py(does not matcha[1]/mod.pyviapathspec); the new construction producesa\\[1\\]/mod.py(matches).The two tests that create a directory literally named
***/q?are markedskipif(sys.platform == "win32")(those characters can't exist in a Windows filename, a testlimitation, not a fix limitation); added a pure-function test for
_escape_gitignore_path_componentso that escaping logic keeps cross-platform coverage withouttouching the filesystem.