diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..5bb5fb8 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,17 @@ +# Changelog + +All notable changes to Orbit are recorded here. The format follows +[Keep a Changelog](https://keepachangelog.com/en/1.1.0/) and this project +adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +## [Unreleased] + +### Fixed + +- `orbit copy` and `orbit plan` now refuse a destination that ends with a + path separator (exit code 3, `destination_has_trailing_separator`). A + trailing separator is a directory signal, so silently normalizing it to a + single-file destination was surprising. Fixes #110. +- Windows error messages render paths with a consistent ordinary spelling. + Reversible `\\?\C:\` and `\\?\UNC\` prefixes are now stripped before + display so similar errors read the same way. Fixes #111. diff --git a/src/error.rs b/src/error.rs index a998788..a288e51 100644 --- a/src/error.rs +++ b/src/error.rs @@ -15,6 +15,8 @@ pub enum OrbitError { DestinationExists(PathBuf), #[error("destination parent does not exist or is not a directory: {0}")] DestinationParentMissing(PathBuf), + #[error("destination has a trailing path separator, expected a single file path: {0}")] + DestinationHasTrailingSeparator(PathBuf), #[error("staged copy did not match the source: {0}")] VerificationFailed(PathBuf), #[error("source changed while it was being copied: {0}")] @@ -45,6 +47,7 @@ impl OrbitError { Self::CopyRequiresRegularFile(_) => "copy_requires_regular_file", Self::DestinationExists(_) => "destination_exists", Self::DestinationParentMissing(_) => "destination_parent_missing", + Self::DestinationHasTrailingSeparator(_) => "destination_has_trailing_separator", Self::VerificationFailed(_) => "verification_failed", Self::SourceChanged(_) => "source_changed", Self::StagingChanged(_) => "staging_changed", @@ -61,6 +64,7 @@ impl OrbitError { | Self::CopyRequiresRegularFile(_) | Self::DestinationExists(_) | Self::DestinationParentMissing(_) + | Self::DestinationHasTrailingSeparator(_) | Self::SourceChanged(_) => 3, Self::VerificationFailed(_) | Self::StagingChanged(_) | Self::StagingCleanup { .. } => { 1 diff --git a/src/paths.rs b/src/paths.rs index ae2a49d..9a3f76c 100644 --- a/src/paths.rs +++ b/src/paths.rs @@ -20,6 +20,11 @@ pub fn resolve_endpoints( destination: &Path, cwd: &Path, ) -> Result { + if ends_with_separator(destination) { + return Err(OrbitError::DestinationHasTrailingSeparator( + destination.to_path_buf(), + )); + } let source_input = absolutize(source, cwd)?; let source_metadata = match fs::symlink_metadata(&source_input) { Ok(metadata) => metadata, @@ -49,6 +54,22 @@ pub fn resolve_endpoints( }) } +#[cfg(windows)] +fn ends_with_separator(path: &Path) -> bool { + // The raw user input must be inspected here because GetFullPathNameW drops + // trailing separators before resolve_leaf can see them. + path.as_os_str() + .encode_wide() + .last() + .is_some_and(|unit| unit == u16::from(b'\\') || unit == u16::from(b'/')) +} + +#[cfg(not(windows))] +fn ends_with_separator(path: &Path) -> bool { + use std::os::unix::ffi::OsStrExt; + matches!(path.as_os_str().as_bytes().last(), Some(&b'/')) +} + fn absolutize(path: &Path, cwd: &Path) -> Result { let path = if path.is_absolute() { path.to_path_buf() @@ -287,6 +308,33 @@ mod tests { )); } + #[test] + fn rejects_destination_with_trailing_separator() { + let temp = tempdir().unwrap(); + let source = temp.path().join("source.txt"); + fs::write(&source, b"source").unwrap(); + // `\\` is a path separator on Windows but a literal filename character + // on Unix, so only check the separators the host actually uses. + let suffixes: &[&str] = if cfg!(windows) { + &["new_dir/", "new_dir\\"] + } else { + &["new_dir/"] + }; + for suffix in suffixes { + let mut raw = temp.path().as_os_str().to_os_string(); + raw.push(std::path::MAIN_SEPARATOR_STR); + raw.push(suffix); + let destination = std::path::PathBuf::from(raw); + assert!( + matches!( + resolve_endpoints(&source, &destination, temp.path()), + Err(OrbitError::DestinationHasTrailingSeparator(_)) + ), + "destination {destination:?} slipped through trailing-separator check" + ); + } + } + #[test] fn rejects_destination_inside_source_directory() { let temp = tempdir().unwrap(); diff --git a/src/report.rs b/src/report.rs index c89da0b..0147b20 100644 --- a/src/report.rs +++ b/src/report.rs @@ -186,6 +186,10 @@ fn error_message(error: &OrbitError) -> String { "destination parent does not exist or is not a directory: {}", display_path(path) ), + OrbitError::DestinationHasTrailingSeparator(path) => format!( + "destination has a trailing path separator, expected a single file path: {}", + display_path(path) + ), OrbitError::VerificationFailed(path) => format!( "staged copy did not match the source: {}", display_path(path) @@ -304,6 +308,7 @@ fn write_json_line(writer: &mut impl Write, value: &impl serde::Serialize) -> io writeln!(writer) } +#[cfg(not(windows))] fn display_path(path: &Path) -> String { match path.to_str() { Some(value) => value.to_owned(), @@ -311,6 +316,33 @@ fn display_path(path: &Path) -> String { } } +#[cfg(windows)] +fn display_path(path: &Path) -> String { + let Some(value) = path.to_str() else { + return format!("[{}:{}]", native_path_encoding(), native_path_hex(path)); + }; + let Some(rest) = value.strip_prefix(r"\\?\") else { + return value.to_owned(); + }; + if let Some(unc) = rest.strip_prefix("UNC\\") { + return format!(r"\\{unc}"); + } + let bytes = rest.as_bytes(); + let drive_letter = bytes + .first() + .copied() + .is_some_and(|c| c.is_ascii_alphabetic()); + let drive_colon = bytes.get(1).copied() == Some(b':'); + let drive_root = bytes + .get(2) + .copied() + .is_none_or(|c| c == b'\\' || c == b'/'); + if drive_letter && drive_colon && drive_root { + return rest.to_owned(); + } + value.to_owned() +} + #[cfg(unix)] fn native_path_encoding() -> &'static str { "unix_bytes_hex" @@ -510,6 +542,45 @@ mod tests { ); } + #[cfg(windows)] + #[test] + fn display_path_strips_reversible_verbatim_prefix() { + use super::display_path; + use std::path::PathBuf; + + assert_eq!( + display_path(&PathBuf::from(r"\\?\C:\tmp\vibe\noexist")), + r"C:\tmp\vibe\noexist" + ); + assert_eq!(display_path(&PathBuf::from(r"\\?\C:\")), r"C:\"); + assert_eq!( + display_path(&PathBuf::from(r"\\?\UNC\server\share\file.txt")), + r"\\server\share\file.txt" + ); + // Non-reversible verbatim forms stay as the operating system reported them. + let volume = r"\\?\Volume{00000000-0000-0000-0000-000000000000}\file.txt"; + assert_eq!(display_path(&PathBuf::from(volume)), volume); + assert_eq!( + display_path(&PathBuf::from(r"C:\tmp\vibe\missing.txt")), + r"C:\tmp\vibe\missing.txt" + ); + } + + #[test] + fn trailing_separator_error_message_names_the_destination() { + use super::error_message; + use std::path::PathBuf; + + let message = error_message(&OrbitError::DestinationHasTrailingSeparator(PathBuf::from( + "out/", + ))); + assert!( + message.contains("trailing path separator"), + "message was: {message}" + ); + assert!(message.contains("out/"), "message was: {message}"); + } + #[test] fn every_error_variant_has_a_stable_code_and_exit_code() { let errors = [ diff --git a/tests/copy_integration.rs b/tests/copy_integration.rs index 1fdb3e8..65a6440 100644 --- a/tests/copy_integration.rs +++ b/tests/copy_integration.rs @@ -128,6 +128,28 @@ fn missing_destination_parent_is_refused_without_creating_directories() { assert!(partials(temp.path()).is_empty()); } +#[test] +fn trailing_separator_destination_is_refused_without_creating_a_file() { + let temp = tempfile::tempdir().unwrap(); + fs::write(temp.path().join("source.bin"), b"payload").unwrap(); + + #[cfg(windows)] + let destinations = ["new_dir/", "new_dir\\"]; + #[cfg(not(windows))] + let destinations = ["new_dir/"]; + + for destination in destinations { + orbit(temp.path()) + .args(["copy", "source.bin", destination]) + .assert() + .code(3) + .stderr(predicate::str::contains("trailing path separator")); + + assert!(!temp.path().join("new_dir").exists()); + assert!(partials(temp.path()).is_empty()); + } +} + #[test] fn json_success_is_one_machine_readable_result() { let temp = tempfile::tempdir().unwrap(); @@ -185,6 +207,29 @@ fn quiet_refusal_remains_visible_on_stderr() { .stderr(predicate::str::contains("destination already exists")); } +#[cfg(windows)] +#[test] +fn error_messages_use_ordinary_path_spelling_on_windows() { + let temp = tempfile::tempdir().unwrap(); + fs::write(temp.path().join("source.bin"), b"payload").unwrap(); + + let output = orbit(temp.path()) + .args(["copy", "source.bin", "missing/destination.bin"]) + .output() + .unwrap(); + + assert_eq!(output.status.code(), Some(3)); + let stderr = String::from_utf8(output.stderr).unwrap(); + assert!( + !stderr.contains(r"\\?\"), + "error message leaked verbatim prefix: {stderr}" + ); + assert!( + stderr.contains("destination parent"), + "unexpected message: {stderr}" + ); +} + #[test] fn symbolic_link_source_is_refused_without_following_it() { let temp = tempfile::tempdir().unwrap(); diff --git a/tests/plan_integration.rs b/tests/plan_integration.rs index 19c6c31..cd7fdf1 100644 --- a/tests/plan_integration.rs +++ b/tests/plan_integration.rs @@ -426,13 +426,41 @@ fn quiet_conflict_reports_exact_blocking_paths_and_reasons_on_stderr() { String::from_utf8(output.stderr).unwrap(), format!( "error: CONFLICT {} — destination file has different contents\nerror: CONFLICT {} — destination file has different contents\n", - resolved_destination.join("a.txt").to_str().unwrap(), - resolved_destination.join("b.txt").to_str().unwrap(), + display_path(&resolved_destination.join("a.txt")), + display_path(&resolved_destination.join("b.txt")), ) ); assert_tree_unchanged(temp.path(), &before); } +/// Mirrors `report::display_path` on Windows: reversible `\\?\` prefixes are +/// stripped so human error output matches an ordinary path spelling. +fn display_path(path: &Path) -> String { + let value = path.to_str().expect("test paths are UTF-8"); + #[cfg(windows)] + { + if let Some(rest) = value.strip_prefix(r"\\?\") { + if let Some(unc) = rest.strip_prefix("UNC\\") { + return format!(r"\\{unc}"); + } + let bytes = rest.as_bytes(); + let drive_letter = bytes + .first() + .copied() + .is_some_and(|c| c.is_ascii_alphabetic()); + let drive_colon = bytes.get(1).copied() == Some(b':'); + let drive_root = bytes + .get(2) + .copied() + .is_none_or(|c| c == b'\\' || c == b'/'); + if drive_letter && drive_colon && drive_root { + return rest.to_owned(); + } + } + } + value.to_owned() +} + #[cfg(unix)] #[test] fn quiet_unsupported_socket_reports_source_and_can_be_explicitly_ignored() {