Repository navigation
fix(path): show one separator in a displayed Windows path - #12024
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDisplayed paths now use host-specific separators. Windows output uses backslashes, while Unix output preserves existing behavior. Relative-path formatting, display assertions, and Windows end-to-end output have dedicated coverage. ChangesPath display normalization
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to This localized change standardizes displayed Windows path separators without altering stored paths or non-Windows behavior. No actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR normalizes separators in human-facing Windows path output while preserving path identity where strings are matched.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (4): Last reviewed commit: "fix(path): show one separator in a displ..." | Re-trigger Greptile |
953be03 to
758eb7f
Compare
`Path::join` appends a multi-segment literal verbatim, so
`root.join(".git/hooks")` resolves correctly through `components()` while
`Display` shows the `/` it was given -- and roots handed back by libraries
arrive `/`-separated already. One printed path could switch form three
times: `C:/Users/me\proj\.git/hooks\pre-commit`.
Settled in `file::display_path`, the helper that formats a path for a
message. Deliberately *not* in `PathExt::display_user`: that one also feeds
strings mise matches rather than shows. `ToolRequest::Path::version()`
builds `path:<display_user>` and lockfile entries are compared on that
string, and `system::edits` compares a user-supplied filter against one --
rewriting there would change identity, not presentation.
Four `mise generate` subcommands were reporting through `xx::file::display_path`
rather than mise's own, so their "Wrote to"/"Generated" lines skipped every
step the rest of mise applies -- no `~`, no extended-length prefix removal,
and now no separator settling. They import `crate::file::display_path` now.
That is what the e2e case in this PR was catching.
`display_rel_path` bypassed the helper too, hardcoding `./` and printing the
remainder raw, so it produced `./mise-tasks\build`. Its prefix now takes the
host separator and its remainder goes through `display_path`.
Byte-identical on unix.
758eb7f to
6b14ce2
Compare
A single path mise prints on Windows can carry both separators at once. Measured on
2026.8.2 windows-x64:
The last one switches form three times inside one path.
Two causes, and why fixing the construction sites does not finish the job
Path::joinappends what it is given verbatim, soroot.join(".git/hooks"),.join(".github/workflows"),.join(".devcontainer/devcontainer.json")and the config-filename candidates (
".config/mise/config.toml"and friends) resolve correctlythrough
components()whileDisplayshows the/./-separated. TheC:/Users/...above comes from the git library, notfrom anything mise assembled — there is no construction site to fix.
The change
Settled in
PathExt::display_user, which #12014 established as the one place mise turns a pathinto text for a person. That covers both causes at once, including (2).
/is a separator on Windows and never part of a name, so both spellings address the same fileand rewriting is safe for display. Nothing changes off Windows, where
\is an ordinaryfilename character — a file really can be named
weird\namethere, and turning that into aseparator would rename it in the message. The existing unix test pins that boundary.
display_rel_pathbypasseddisplay_userentirely, hardcoding./and printing the remainderraw, so it produced
./mise-tasks\build. Its prefix now takesMAIN_SEPARATORand its remaindergoes through
display_user. On unix that is byte-identical, sinceMAIN_SEPARATORis/anddisplay_userrewrites nothing. Its only caller is the+path/-pathstatus line inhook_env.Deliberately unchanged
no construction site in mise to begin with.
mise ls --jsonvalues, shim contents. Those go out throughto_string_lossy, notdisplay_path. The case that mattered there was already fixed at thesource: see the note on
replace_path, where tilde expansion was producingC:\Users\me\.local/share/miseand that path reachedMISE_DATA_DIR, shims andls --json.This PR is the display half of the same symptom, and says so rather than widening.
\is an escape in a glob andrewriting one would mislead: across all 356
display_path/display_rel_pathcall sites, nonetakes a pattern. Patterns are carried as
Stringand printed with{}; the one site intask_source_checkerthat looked like a candidate prints a resolved source file.Verification
Measured before the change (the three commands above) and again afterwards against the exact
composition the code performs, with a small cargo probe rather than by reading:
That last one corrected an assertion I had written from reasoning:
/is not a separator inside\\?\, soa/breads as one component there andduncedeclines to simplify a name Windowscould not hold. The separators still settle; the prefix stays.
canonicalizenever emits thatshape, but the test now records the real answer.
Tests:
src/path.rs: a path that switches form three times settles; already-uniform, relative and UNCinputs; composition with the extended-length prefix from fix(trust): stop printing extended-length path prefixes #12014; and on unix, that
weird\nameand
/a/b\care left exactly as they are.src/file.rs:display_rel_pathuses the host separator on both halves.e2e-win/display_path_separators.Tests.ps1:mise generate git-pre-commit --writeandmise config lsreport no/, while still naming the path — so neither can pass by printingnothing. Matched as a literal, since
-BeLikewould read?and[as wildcards.cargo fmtwas run locally. The build, clippy and the test suite were not — those are left toCI, so behaviour after the fix rests on the measurements above and on the tests rather than on a
running binary.
Found while auditing Windows behaviour, the same pass that produced #12007, #12008, #12012, #12013,
#12014, #12016 and #12023. No existing issue or discussion covers it.
Summary by CodeRabbit
/and\formatting.