Address CodeRabbit review: Windows tests follow the SystemRoot fallback

The tests built the expected explorer.exe / reg.exe path from SystemRoot with unwrap(),
so they panicked when it was unset and ignored the C:\Windows fallback for an unset or
relative value. The rule is now a small pure function windows_root(value) in each crate;
the tests use it and check the absolute, unset, relative, rooted-without-drive and empty
cases without touching the process environment.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
David MannandClaude Opus 5.5 committed 2026-10-26 14:22:38 +01:00
1 parent e47b2d8514
commit 6c2e90c8bb
3 files changed
+45 -8

No files matched your search

+15 -1
View File
@@ -1532,11 +1532,25 @@ fn reveal_in_finder_on_windows_passes_the_clicked_tabs_saved_path() {
let r = s.execute("file.revealInFinder", json!({"document": 0, "dryRun": true})).unwrap();
// The Windows Explorer by full path: a bare name would first find an explorer.exe in the app's folder.
let explorer = std::path::Path::new(&std::env::var_os("SystemRoot").unwrap()).join("explorer.exe");
let explorer = crate::layer_menu_cmds::windows_root(std::env::var_os("SystemRoot")).join("explorer.exe");
assert_eq!(r["program"], json!(explorer.to_string_lossy()));
assert_eq!(r["args"], json!(["/select,", r"C:\项目 图片\海报 🖼.png"]));
}
/// Windows tools run from the Windows folder `SystemRoot` names, and from `C:\Windows` when it is
/// unset or relative (which would resolve against the working directory).
#[cfg(windows)]
#[test]
fn the_windows_folder_falls_back_when_system_root_is_unset_or_relative() {
use crate::layer_menu_cmds::windows_root;
use std::path::PathBuf;
assert_eq!(windows_root(Some(r"D:\WinNT".into())), PathBuf::from(r"D:\WinNT"));
assert_eq!(windows_root(None), PathBuf::from(r"C:\Windows"));
assert_eq!(windows_root(Some(r"Windows".into())), PathBuf::from(r"C:\Windows"));
assert_eq!(windows_root(Some(r"\Windows".into())), PathBuf::from(r"C:\Windows"));
assert_eq!(windows_root(Some("".into())), PathBuf::from(r"C:\Windows"));
}
/// "Include All Subfolders" (AUTO-219-5, #3753): Batch and Image Processor take the images in
/// subfolders too, each result keeping its subfolder under the output folder (so same-named files
/// don't collide), and skip the output folder when it sits inside the input folder.
+8 -3
View File
@@ -420,9 +420,14 @@ pub fn reveal_command(path: &str) -> Result<(String, Vec<String>)> {
/// an `explorer.exe` lying next to a portable PhotoCraft would start instead (#9666).
#[cfg(windows)]
fn system_program(program: &str) -> String {
let root = std::env::var_os("SystemRoot").map(std::path::PathBuf::from).filter(|p| p.is_absolute());
let root = root.unwrap_or_else(|| std::path::PathBuf::from(r"C:\Windows"));
root.join(format!("{program}.exe")).to_string_lossy().into_owned()
windows_root(std::env::var_os("SystemRoot")).join(format!("{program}.exe")).to_string_lossy().into_owned()
}
/// The Windows folder for the `SystemRoot` value `system_root`: `C:\Windows` when it is unset or
/// not an absolute path (a relative one would resolve against the working directory).
#[cfg(windows)]
pub(crate) fn windows_root(system_root: Option<std::ffi::OsString>) -> std::path::PathBuf {
system_root.map(std::path::PathBuf::from).filter(|p| p.is_absolute()).unwrap_or_else(|| std::path::PathBuf::from(r"C:\Windows"))
}
#[cfg(not(windows))]
+22 -4
View File
@@ -128,13 +128,18 @@ fn os_locale() -> Option<String> {
fn locale_query() -> std::process::Command {
use std::os::windows::process::CommandExt;
const CREATE_NO_WINDOW: u32 = 0x0800_0000;
let root = std::env::var_os("SystemRoot").map(std::path::PathBuf::from).filter(|p| p.is_absolute());
let root = root.unwrap_or_else(|| std::path::PathBuf::from(r"C:\Windows"));
let mut cmd = std::process::Command::new(root.join("System32").join("reg.exe"));
let mut cmd = std::process::Command::new(windows_root(std::env::var_os("SystemRoot")).join("System32").join("reg.exe"));
cmd.args(["query", "HKCU\\Control Panel\\International", "/v", "LocaleName"]).creation_flags(CREATE_NO_WINDOW);
cmd
}
/// The Windows folder for the `SystemRoot` value `system_root`: `C:\Windows` when it is unset or
/// not an absolute path (a relative one would resolve against the working directory).
#[cfg(target_os = "windows")]
fn windows_root(system_root: Option<std::ffi::OsString>) -> std::path::PathBuf {
system_root.map(std::path::PathBuf::from).filter(|p| p.is_absolute()).unwrap_or_else(|| std::path::PathBuf::from(r"C:\Windows"))
}
#[cfg(not(any(target_os = "macos", target_os = "windows")))]
fn os_locale() -> Option<String> {
None
@@ -574,11 +579,24 @@ mod tests {
#[test]
fn locale_query_runs_the_system_reg() {
let cmd = locale_query();
let system = std::path::Path::new(&std::env::var_os("SystemRoot").unwrap()).join("System32").join("reg.exe");
let system = windows_root(std::env::var_os("SystemRoot")).join("System32").join("reg.exe");
assert_eq!(std::path::Path::new(cmd.get_program()), system);
assert!(os_locale().is_some_and(|l| !l.is_empty()));
}
/// `SystemRoot` unset or relative (which would resolve against the working directory) falls
/// back to `C:\Windows`.
#[cfg(target_os = "windows")]
#[test]
fn the_windows_folder_falls_back_when_system_root_is_unset_or_relative() {
use std::path::PathBuf;
assert_eq!(windows_root(Some(r"D:\WinNT".into())), PathBuf::from(r"D:\WinNT"));
assert_eq!(windows_root(None), PathBuf::from(r"C:\Windows"));
assert_eq!(windows_root(Some(r"Windows".into())), PathBuf::from(r"C:\Windows"));
assert_eq!(windows_root(Some(r"\Windows".into())), PathBuf::from(r"C:\Windows"));
assert_eq!(windows_root(Some("".into())), PathBuf::from(r"C:\Windows"));
}
#[test]
fn locale_orders() {
assert_eq!(script_order(Some("ja_JP.UTF-8"))[0], Japanese);