--exclude: use .gitignore glob syntax - #154
Merged
Merged
Conversation
`--exclude node_modules` matched nothing, silently. Patterns were fnmatch'd against the whole path and relative ones were resolved against the cwd, so a bare name could only ever match one literal directory -- and usually not even that. Every NAS user's first instinct (@eadir, .snapshots, node_modules) was a no-op, and the tool said nothing (#147). Adopt the syntax users already have in their fingers, the one git, ripgrep and fd use: - no '/' -> matches the name at any depth (@eadir, *.iso) - leading '/' -> absolute, anchored (/srv/media/cache*) - interior '/' -> matches at any depth (Steam/temp) - trailing '/' -> directories only (cache/) - '*' stops at '/', '**' crosses, '?' and [a-z] as usual Negation ('!') is deliberately unsupported: last-match-wins ordering is where the complexity and the bugs are, and nobody has asked for it. This is a behaviour change, taken deliberately without syntax versioning so there is only ever one meaning for a pattern. Patterns stored in an existing hashfile replay under the new rules. Two things blunt the edge: a pattern that matches nothing is now reported instead of being silent, and a malformed one is an error before the scan starts rather than a warning the run ignores. New src/glob.{c,h}. Patterns compile to PCRE2 fragments joined into one combined GRegex, so matching costs one regex run per path however many patterns there are; exact absolute paths skip it via a hash lookup. GRegex is PCRE2 and GLib is already linked, so this adds no dependency. The set is compiled once in filescan_init() -- main thread, before any walker exists -- and is read-only after, so the walkers need no lock. The per-pattern regexes survive only to attribute a hit for the -v message, which runs on paths already being skipped, never on the hot negative path. The hashfile and its -wal/-shm sidecars now register via add_exclude_path(), matched literally, so a '*' or '[' in the hashfile's own path cannot become a wildcard and drop unrelated files. Also fixes a silent failure found while testing: a rejected --exclude printed an error and carried on, scanning a wider tree than asked for and still exiting 0. 9 unit tests over the matching rules, 10 integration tests over the observable behaviour. scripts/verify.sh passes (126 tests, valgrind smoke). Co-Authored-By: Claude <noreply@anthropic.com>
Review pass over the --exclude change. One finding was a real regression, the
rest are structural.
Turn on PCRE2's JIT (G_REGEX_OPTIMIZE) at both g_regex_new() sites. Without it
the new matcher was ~12x slower per path than the fnmatch() code it replaces --
measured in a harness over 4096 realistic paths with 3 patterns: 138 ns/path
old, 3694 ns as written, 304 ns with the flag. That is per directory entry on
every walker, so on a million-file tree it is seconds of CPU. The flag is not
deprecated in any GLib we target (it predates 2.14) and compiles clean under
the project's warning set.
Structural cuts, all behaviour-preserving:
- One combined regex, not two. The dir-only alternation earned nothing:
attribute() already applies dir_only and is the only thing that can name
the matching pattern, so directories were paying a second regex run on the
hot negative path for a filter that ran anyway.
- Drop `bool compiled` (written in three places, read nowhere) and
`glob_pat.literal` (always a second copy of ->pattern; the hash table now
keys on ->pattern, and ->re == NULL is the single discriminator).
- Escape via g_regex_escape_string() instead of a hand-kept metacharacter
list that could drift from PCRE2's.
- append_class() rolls back with g_string_truncate() instead of building a
throwaway GString per character class.
- g_clear_pointer(), memchr(), g_str_has_suffix() where the codebase already
uses them; fold the trailing-'/' test into is_plain_path().
- glob_set_add_literal() and add_exclude_path() return void -- they had no
failure path, and every caller ignored the int.
- Delete glob_set_empty(): no production caller.
- Drop the unreachable `err ? err : "..."` and `which ? which : "?"`
fallbacks; every failure path sets them.
- Drop <bsd/sys/queue.h> from file_scan.c, dead since the SLIST went.
Two correctness-adjacent fixes the review surfaced:
- gp->matches++ was a plain shared increment from the walker threads, which
contradicted the "read-only after compile, so walkers need no lock" claim
in glob.h, CLAUDE.md and the commit message. Only its zero/non-zero was
ever read, so it becomes a relaxed _Atomic bool written once on first hit:
no race, and no contended write per excluded path.
- The replay path ignored add_exclude_pattern()'s return, so a stored pattern
that no longer parses would scan a wider tree and still exit 0 -- on the
unattended systemd path, where nobody would see it. It now fails like the
command-line path does.
Also move the "matched nothing" warning out of filescan_free() into
scan_files(), gated on the walk having succeeded: as teardown it also fired
after a failed scan, where every pattern trivially matched nothing and the
noise buried the real error.
Tests: gs_hit() now aborts on a pattern that fails to compile instead of
returning false, which had been letting every negative assertion pass
vacuously. Seven integration cases share an assert_kept() helper. Added the
pre-1.6.0 upgrade note to the NAS guide, where the people with patterns
already stored in a hashfile actually look.
scripts/verify.sh passes (126 tests, valgrind smoke).
Co-Authored-By: Claude <noreply@anthropic.com>
The valgrind CI job failed on every --exclude test with "conditional jump or
move depends on uninitialised value(s)" in unnamed frames -- PCRE2's
JIT-generated code, which matches a word at a time and so reads past the
string's terminator. The reads are inside their allocations and harmless, but
the branches are on undefined bytes, and both buffers were genuinely
under-initialised:
- process_dir()'s `child` was malloc'd once per directory, and each entry
only ever writes up to its own terminator, so the tail stayed undefined for
the life of the buffer. calloc it: once per directory, not per entry.
- scan_file()'s `char path[PATH_MAX]` is a 4 KiB stack buffer that realpath()
fills only as far as the terminator. Zero it; once per scan root.
Fixing the buffers rather than suppressing the JIT frames: a suppression on
`???` addresses would have to match unnamed frames, which is both fragile and
exactly the kind of blanket that hides a real error later.
verify.sh's smoke never exercised this because it does not pass --exclude, so
no regex is ever compiled -- only `make integration-valgrind` reaches it. Now
clean: 126 tests under memcheck, "valgrind: no findings".
Co-Authored-By: Claude <noreply@anthropic.com>
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
1 participant
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.
Closes #147.
--exclude node_modulesmatched nothing, silently. Patterns werefnmatch'd against the whole path and relative ones were resolved against the cwd, so a bare name could only ever match one literal directory — and usually not even that. Every NAS user's first instinct (@eaDir,.snapshots,node_modules) was a no-op, and the tool said nothing.The syntax
The one git, ripgrep and fd already use:
@eaDir,*.iso/→ matches the name at any depth/srv/media/cache*/→ absolute, anchoredSteam/temp/→ matches at any depthcache//→ directories only*?[a-z]***stops at/,**crossesNegation (
!) is deliberately unsupported — last-match-wins ordering is where the complexity and the bugs are, and nobody has asked for it.Excluding a directory prunes the walk there, so naming a directory also drops everything inside it.
This is a breaking change
Taken deliberately, without syntax versioning, so a pattern only ever has one meaning. Patterns stored in an existing hashfile replay under the new rules — which matters most for scheduled systemd jobs.
Two things blunt the edge:
The widening direction (a stored relative pattern that now matches more) can't be caught by a count, but
-valready printsExcluding: <path> (matches <pattern>)per file, which shows it directly. Documented as Changed in 1.6.0 in the man page, with an upgrade note in the NAS quick-start where affected users actually look, and inCLAUDE.mdfor contributors.Implementation
New
src/glob.{c,h}. Patterns compile to PCRE2 fragments joined into one combinedGRegex, so matching costs one regex run per path however many patterns there are. Exact absolute paths skip it via a hash lookup.GRegexis PCRE2 and GLib was already linked, so no new dependency.The set is compiled once in
filescan_init()— main thread, before any walker exists — and is read-only afterwards apart from one relaxed_Atomic boolper pattern, so the walkers need no lock. The per-pattern regexes survive only to name the matching pattern and apply the directory-only rule, which runs on paths already being excluded, never on the hot negative path.The hashfile and its
-wal/-shmsidecars register viaadd_exclude_path()and match literally, so a*or[in the hashfile's own path cannot become a wildcard.Performance
is_excluded()runs once per directory entry on every walker, so this was measured rather than assumed. With PCRE2's JIT enabled (G_REGEX_OPTIMIZE), 4096 realistic paths against 3 patterns:strcmp+ 3×fnmatch~2.2× the absolute cost of the old code, but O(1) in pattern count rather than O(n) — which is the trade the design is for. The JIT flag is not optional here; without it this would have been a 12× regression on a hot path. (It is also well under the
statx/btrfs-btree cost that dominates the walk.)Testing
9 unit tests over the matching rules, 10 integration tests over observable behaviour — pruning, the
S_ISDIRplumbing that the unit tests can't reach, the warning, and the malformed-pattern exit code.scripts/verify.shpasses: build with warnings-as-failure, 126 tests, valgrind scan+dedupe+replay smoke.A
/simplifypass is the second commit: it found the missing JIT flag, a plain (non-atomic) shared counter written from the walker threads, and the replay path ignoring pattern errors. See that commit message for the full list.🤖 Generated with Claude Code