fix(daemon): split glob segments on / only, never on backslash - #7788
Merged
Conversation
The daemon's module-relative glob expander walked `std::path::Component`s to find its segment boundaries. On Windows `Path::components()` splits on `\` as well as `/`, so a pattern's `\` - which upstream treats as wildmatch's escape character - was consumed as a path separator instead. `mod/a\*` was read as the segments `a` and `*`, and the daemon descended into the directory `a` and served every entry it held, where upstream matches the single segment `a\*` and serves only the file literally named `a*`. Upstream's `glob_match()` finds each boundary with `strchr(arg, '/')` (util1.c:749) and has no `\` arm on any platform, while `wildmatch()` is byte-oriented with one explicit `'/'` check (lib/wildmatch.c:101-102) and makes `\` escape the next pattern byte both outside a class (lib/wildmatch.c:86) and inside one (lib/wildmatch.c:154). Split the wire tail on `/` alone to match, so a `\` always reaches the matcher as the escape byte. `wildmatch()` stays the single owner of pattern interpretation; the expander now only decides where segments end. No Windows special case is added because upstream has none. The change is a provable no-op on Unix, where `Path::components()` already split on `/` alone - the three existing escape tests pass unchanged. Also refreshes four upstream line citations against 3.5.0 and drops the now-stale note claiming the Windows escape arm was an accepted gap.
`[`, `]` and `x` are all legal in a Windows filename, so the fixture for `resolve_sender_sources_glob_class_with_escaped_bracket` is representable there. The test was `#[cfg(unix)]` only because the old `std::path::Component` walk split `[\]]*` at the `\` on Windows, so the pattern never reached `wildmatch()` intact. With segments split on `/` alone the gate is no longer needed, and dropping it turns the Windows arm of the escape rule from a reasoned claim into an assertion the Windows CI cell actually executes. The two neighbouring escape tests stay Unix-only for a real reason: their fixtures are named `a*` and `a\b.txt`, and `*` is a reserved character on Windows while `\` is a separator, so neither name can exist there.
oferchen
enabled auto-merge (squash)
September 9, 2026 12:06
oferchen
added a commit
that referenced
this pull request
Sep 9, 2026
`the_exclude_self_modifier_hides_the_merge_files_basename` failed on every Windows feature-flag cell: left: ["C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\.tmpooXxku\\rules", "bait"] right: ["rules", "bait"] The defect is in the fixture, not the splitter. A daemon filter path has exactly one separator: upstream finds the merge file's basename with `strrchr(name, '/')` (exclude.c:1557-1567), the same `/`-only rule the glob expander uses (util1.c:749). The helper handed it `Path::display()`, which emits the HOST separator, so on Windows the whole backslash-spelled path was the basename and the `e` modifier's exclude matched nothing. `merge_file` now rewrites `MAIN_SEPARATOR` to `/` - a no-op on Unix, and only ever the host separator, never a byte that could belong to a real name. Windows accepts `/` in a path, so the merge file still opens. Adding a backslash arm to the splitter instead would re-introduce exactly the conflation removed by "the daemon glob expander splits on `/` only" (#7788), so the production code is untouched. A sibling cell makes that separator choice load-bearing rather than cosmetic: a merge file literally named `a\b` must yield `a\b` as its basename, not `b`. It is Unix-gated because that is the only place the rule can be exercised - Windows rejects a backslash in a filename, and the daemon refuses to run there at all. Splitting on `['/', '\\']` fails that one cell and nothing else (1 of 1934).
oferchen
added a commit
that referenced
this pull request
Sep 10, 2026
* fix(daemon): honour `merge` and `!` in a module filter rule A daemon `filter = merge FILE` was built as ONE rule: an exclude of the literal pattern `merge <path>`. That pattern matches no file, so every entry the operator's merge file named was served, and a merge file the daemon could not read was ignored instead of refusing the module. `push_token_rules` now reads the named file and splices its rules into the list at the merge rule's own position, mirroring `parse_filter_str`'s FILTRULE_MERGE_FILE arm (exclude.c:1553-1590). A merge file's records carry XFLG_FATAL_ERRORS alone, not the module directive's XFLG_ABS_IF_SLASH | XFLG_DIR2WILD3, so `RuleXflags` now keys the three things add_rule gates on those bits: the embedded-slash anchoring, the `/***` directory suffix, and the add-time side drop. A `H bait` record inside a merge file therefore hides `bait`, where the same rule written as the directive is dropped. The clear forms did NOT share a cause. `filter = clear` was already correct; `filter = !` was not implemented at all - a bare `!` fell through to the bare-pattern arm and became an exclude of the literal name `!`, and `!,` was refused outright. Both spellings are now the list clear, and the tokenizer opens a token on a whole-word `!` so `- bait - ctl !` clears what precedes it. The opener is deliberately narrower than upstream's rule character: `- !bait` stays one rule whose pattern is `!bait`. Measured against a real rsync 3.5.0 daemon over loopback TCP with an upstream 3.5.0 client: all 25 cells of the merge and clear matrix now agree with upstream, including a module holding a file literally named `!` as the bait. * test(daemon): spell the merge-file fixture path the way a daemon does `the_exclude_self_modifier_hides_the_merge_files_basename` failed on every Windows feature-flag cell: left: ["C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\.tmpooXxku\\rules", "bait"] right: ["rules", "bait"] The defect is in the fixture, not the splitter. A daemon filter path has exactly one separator: upstream finds the merge file's basename with `strrchr(name, '/')` (exclude.c:1557-1567), the same `/`-only rule the glob expander uses (util1.c:749). The helper handed it `Path::display()`, which emits the HOST separator, so on Windows the whole backslash-spelled path was the basename and the `e` modifier's exclude matched nothing. `merge_file` now rewrites `MAIN_SEPARATOR` to `/` - a no-op on Unix, and only ever the host separator, never a byte that could belong to a real name. Windows accepts `/` in a path, so the merge file still opens. Adding a backslash arm to the splitter instead would re-introduce exactly the conflation removed by "the daemon glob expander splits on `/` only" (#7788), so the production code is untouched. A sibling cell makes that separator choice load-bearing rather than cosmetic: a merge file literally named `a\b` must yield `a\b` as its basename, not `b`. It is Unix-gated because that is the only place the rule can be exercised - Windows rejects a backslash in a filename, and the daemon refuses to run there at all. Splitting on `['/', '\\']` fails that one cell and nothing else (1 of 1934). * fix(daemon): retract the dir-merge no-op, it is live upstream The `:` / `dir-merge` arm claimed a per-directory merge in a daemon `filter` cannot match anything, and cited exclude.c:1573-1582 for it. The citation is real but the conclusion is REFUTED: `add_rule` registers every FILTRULE_PERDIR_MERGE rule into the GLOBAL `mergelist_parents` (exclude.c:349-391) whatever list it went into, so a rule in `daemon_filter_list` is registered too; `push_local_filters` fills that rule's own `u.mergelist` per directory and `check_filter` recurses into it. A daemon per-directory merge is live. Measured against rsync 3.5.0 over loopback TCP, with the listener's pid asserted per cell: a module holding `sub/.rsync-filter` = `- bait.txt` under `filter = : .rsync-filter` HIDES `sub/bait.txt`; the same holds for `dir-merge`, and with the merge file and the bait at the module root. A sibling subtree with no merge file keeps its file - the per-directory signature. The eager-read arm is dropped rather than kept: answering a per-directory rule with a root-only read would look like the feature without being it. `:` and `dir-merge` keep the single-rule parser's existing behaviour, and the cell that asserted the no-op is inverted into a guard against a future eager `:` arm. Expressing this properly needs a DirMerge wire rule and is tracked separately. The same misreading reached the previous commit's own oracle table, which recorded this cell as convergent when its upstream column already showed the file hidden. The comparison is mechanical now, not by eye: 24 of 25 merge/clear cells agree with upstream, and this one is the known divergence.
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.
The daemon's glob expander found segment boundaries by walking
std::path::Path::components(). On Windows that splits on\as well as/,and
\iswildmatch's escape character — so the escape was consumed as aseparator before the matcher ever saw it.
Upstream has no such conflation and no platform arm at all:
glob_match()finds every boundary withstrchr(arg, '/')(util1.c:749).There is no
\case anywhere in the function.wildmatch()is byte-oriented with a single explicit'/'check(
lib/wildmatch.c:101-102);\escapes the next pattern byte outside a class(
:86) and inside one (:154).Segmentation now splits on
/and only/, on every platform, operating on the/-separated wire tail rather than onstd::path.wildmatch()stays thesingle owner of pattern interpretation — the expander only decides where
segments end. That division is now stated in both functions' docs, since it is
exactly what went wrong.
Reachability — stated plainly
oc-rsyncrefuses to start in daemon mode on non-Unix(
crates/cli/src/frontend/server/daemon.rs:210), and this expander is on thedaemon path only. So the Windows misbehaviour is not reachable in a shipped
configuration today; this is a faithfulness and code-health fix that also
removes a platform-conditional behaviour upstream does not have, ahead of any
future daemon-on-Windows work.
What it does change on every platform: the
strip_prefix(module_path)round-trip is gone (the joined path was rebuilt only to be re-split), and empty
and
.segments are skipped explicitly rather than incidentally.The mutation is the evidence
The defect could not be executed on this host, so it was demonstrated by
emulating Windows' rule on a platform that can run:
split('/')→split(['/', '\\']), which iscomponents()'Windows behaviour: 3 kills, exactly the three escape tests. This is the
load-bearing evidence.
Non-vacuous.
components()already split on/alone — which is also why the Unix suitecould never have been the gate for this.
Test gating
resolve_sender_sources_glob_class_with_escaped_bracketwas#[cfg(unix)]witha comment naming this exact conflation as an accepted platform gap.
[,]andxare all legal in a Windows filename, so the gate comes off and theWindows (stable)cell now executes it. A second, Windows-only test pins themod/a\*case, whose fixture (a*) is not representable on NTFS: it assertsboth that the unmatched pattern survives verbatim and that
<mod>/a/xis notserved.
The other two escape tests stay Unix-only for a real reason — their fixtures are
named
a*anda\b.txt.Gates
Pinned 1.88.0:
check_rustfmt_all.pyclean (3430 files) · clippy-p daemon --all-targets -D warningsclean · nextest-p daemon --lib1907/1907 ·citation drift audit exit 0.
cargo check -p daemon --all-targets --target x86_64-pc-windows-gnuis clean, sothe new
#[cfg(windows)]test type-checks. (-msvccannot be checked frommacOS —
zstd-syscompiles C.) Four stale upstream line citations refreshed inthe same commit.