From a935e0720f8688dd7241a532a750fb06a3322f20 Mon Sep 17 00:00:00 2001 From: Doru Date: Sun, 3 Nov 2024 01:56:05 -0300 Subject: [PATCH] no deref in touch (#14214) # Description Adds --no-deref flag to `touch`. Nice and backwards compatible, and I get to touch symlinks. I still don't get to set their dates directly, but maybe that'll come with utouch. Some sadness in the implementation, since `set_symlink_file_times` doesn't take Option values and we call it twice with the old "read" values from reference (or now, if missing). This shouldn't be a big concern since `touch` already did two calls if you set both mtime and atime. Also, `--no-deref` applies both to the reference file, and to the target file. No splitting them up, because that's silly. Can always bikeshed. I nicked `--no-deref` from the uutils flag, and made the short flag `-d` because it obviously can't be `-h`. I thought of `-S` like in `glob`, for the "negative/filter out" uppercase short letters. Ultimately I don't think it matters much. Should fix #14212 since it's not really tied to uutils, besides the comment about setting a `datetime` value directly. # User-Facing Changes New flag. # Tests + Formatting Maybe. # After Submitting --- crates/nu-command/src/filesystem/touch.rs | 91 +++++++++++++--- crates/nu-command/tests/commands/touch.rs | 127 +++++++++++++++++++++- 2 files changed, 203 insertions(+), 15 deletions(-) diff --git a/crates/nu-command/src/filesystem/touch.rs b/crates/nu-command/src/filesystem/touch.rs index 9085554bb4..b4cd808be5 100644 --- a/crates/nu-command/src/filesystem/touch.rs +++ b/crates/nu-command/src/filesystem/touch.rs @@ -48,6 +48,11 @@ impl Command for Touch { "do not create the file if it does not exist", Some('c'), ) + .switch( + "no-deref", + "do not follow symlinks", + Some('s') + ) .category(Category::FileSystem) } @@ -64,6 +69,7 @@ impl Command for Touch { ) -> Result { let mut change_mtime: bool = call.has_flag(engine_state, stack, "modified")?; let mut change_atime: bool = call.has_flag(engine_state, stack, "access")?; + let no_follow_symlinks: bool = call.has_flag(engine_state, stack, "no-deref")?; let reference: Option> = call.get_flag(engine_state, stack, "reference")?; let no_create: bool = call.has_flag(engine_state, stack, "no-create")?; let files: Vec> = get_rest_for_glob_pattern(engine_state, stack, call, 0)?; @@ -88,19 +94,29 @@ impl Command for Touch { if let Some(reference) = reference { let reference_path = nu_path::expand_path_with(reference.item, &cwd, true); - if !reference_path.exists() { + let exists = if no_follow_symlinks { + // There's no symlink_exists function, so we settle for + // getting direct metadata and if it's OK, it exists + reference_path.symlink_metadata().is_ok() + } else { + reference_path.exists() + }; + if !exists { return Err(ShellError::FileNotFoundCustom { msg: "Reference path not found".into(), span: reference.span, }); } - let metadata = reference_path - .metadata() - .map_err(|err| ShellError::IOErrorSpanned { - msg: format!("Failed to read metadata: {err}"), - span: reference.span, - })?; + let metadata = if no_follow_symlinks { + reference_path.symlink_metadata() + } else { + reference_path.metadata() + }; + let metadata = metadata.map_err(|err| ShellError::IOErrorSpanned { + msg: format!("Failed to read metadata: {err}"), + span: reference.span, + })?; mtime = metadata .modified() .map_err(|err| ShellError::IOErrorSpanned { @@ -117,14 +133,27 @@ impl Command for Touch { for glob in files { let path = expand_path_with(glob.item.as_ref(), &cwd, glob.item.is_expand()); + let exists = if no_follow_symlinks { + path.symlink_metadata().is_ok() + } else { + path.exists() + }; // If --no-create is passed and the file/dir does not exist there's nothing to do - if no_create && !path.exists() { + if no_create && !exists { continue; } - // Create a file at the given path unless the path is a directory - if !path.is_dir() { + // If --no-deref was passed in, the behavior of touch is to error on missing + if no_follow_symlinks && !exists { + return Err(ShellError::FileNotFound { + file: path.to_string_lossy().into_owned(), + span: glob.span, + }); + } + + // Create a file at the given path unless the path is a directory (or a symlink with -d) + if !path.is_dir() && (!no_follow_symlinks || !path.is_symlink()) { if let Err(err) = OpenOptions::new() .write(true) .create(true) @@ -138,9 +167,31 @@ impl Command for Touch { }; } + // We have to inefficiently access the target metadata to not reset it + // in set_symlink_file_times, because the filetime doesn't expose individual methods for it + let get_target_metadata = || { + path.symlink_metadata() + .map_err(|err| ShellError::IOErrorSpanned { + msg: format!("Failed to read metadata: {err}"), + span: glob.span, + }) + }; + if change_mtime { - if let Err(err) = filetime::set_file_mtime(&path, FileTime::from_system_time(mtime)) - { + let result = if no_follow_symlinks { + filetime::set_symlink_file_times( + &path, + if change_atime { + FileTime::from_system_time(atime) + } else { + FileTime::from_system_time(get_target_metadata()?.accessed()?) + }, + FileTime::from_system_time(mtime), + ) + } else { + filetime::set_file_mtime(&path, FileTime::from_system_time(mtime)) + }; + if let Err(err) = result { return Err(ShellError::ChangeModifiedTimeNotPossible { msg: format!("Failed to change the modified time: {err}"), span: glob.span, @@ -149,8 +200,20 @@ impl Command for Touch { } if change_atime { - if let Err(err) = filetime::set_file_atime(&path, FileTime::from_system_time(atime)) - { + let result = if no_follow_symlinks { + filetime::set_symlink_file_times( + &path, + FileTime::from_system_time(atime), + if change_mtime { + FileTime::from_system_time(mtime) + } else { + FileTime::from_system_time(get_target_metadata()?.modified()?) + }, + ) + } else { + filetime::set_file_atime(&path, FileTime::from_system_time(atime)) + }; + if let Err(err) = result { return Err(ShellError::ChangeAccessTimeNotPossible { msg: format!("Failed to change the access time: {err}"), span: glob.span, diff --git a/crates/nu-command/tests/commands/touch.rs b/crates/nu-command/tests/commands/touch.rs index 10ae299a0f..b7ab06f511 100644 --- a/crates/nu-command/tests/commands/touch.rs +++ b/crates/nu-command/tests/commands/touch.rs @@ -1,7 +1,7 @@ use chrono::{DateTime, Local}; use nu_test_support::fs::{files_exist_at, Stub}; use nu_test_support::nu; -use nu_test_support::playground::Playground; +use nu_test_support::playground::{Dirs, Playground}; // Use 1 instead of 0 because 0 has a special meaning in Windows const TIME_ONE: filetime::FileTime = filetime::FileTime::from_unix_time(1, 0); @@ -527,3 +527,128 @@ fn reference_respects_cwd() { assert!(path.exists()); }) } + +fn setup_symlink_fs(dirs: &Dirs, sandbox: &mut Playground<'_>) { + sandbox.mkdir("d"); + sandbox.with_files(&[Stub::EmptyFile("f"), Stub::EmptyFile("d/f")]); + sandbox.symlink("f", "fs"); + sandbox.symlink("d", "ds"); + sandbox.symlink("d/f", "fds"); + + // sandbox.symlink does not handle symlinks to missing files well. It panics + // But they are useful, and they should be tested. + #[cfg(unix)] + { + std::os::unix::fs::symlink(dirs.test().join("m"), dirs.test().join("fms")).unwrap(); + } + + #[cfg(windows)] + { + std::os::windows::fs::symlink_file(dirs.test().join("m"), dirs.test().join("fms")).unwrap(); + } + + // Change the file times to a known "old" value for comparison + filetime::set_symlink_file_times(dirs.test().join("f"), TIME_ONE, TIME_ONE).unwrap(); + filetime::set_symlink_file_times(dirs.test().join("d"), TIME_ONE, TIME_ONE).unwrap(); + filetime::set_symlink_file_times(dirs.test().join("d/f"), TIME_ONE, TIME_ONE).unwrap(); + filetime::set_symlink_file_times(dirs.test().join("ds"), TIME_ONE, TIME_ONE).unwrap(); + filetime::set_symlink_file_times(dirs.test().join("fs"), TIME_ONE, TIME_ONE).unwrap(); + filetime::set_symlink_file_times(dirs.test().join("fds"), TIME_ONE, TIME_ONE).unwrap(); + filetime::set_symlink_file_times(dirs.test().join("fms"), TIME_ONE, TIME_ONE).unwrap(); +} + +fn get_times(path: &nu_path::AbsolutePath) -> (filetime::FileTime, filetime::FileTime) { + let metadata = path.symlink_metadata().unwrap(); + + ( + filetime::FileTime::from_system_time(metadata.accessed().unwrap()), + filetime::FileTime::from_system_time(metadata.modified().unwrap()), + ) +} + +#[test] +fn follow_symlinks() { + Playground::setup("touch_follows_symlinks", |dirs, sandbox| { + setup_symlink_fs(&dirs, sandbox); + + let missing = dirs.test().join("m"); + assert!(!missing.exists()); + + nu!( + cwd: dirs.test(), + " + touch fds + touch ds + touch fs + touch fms + " + ); + + // We created the missing symlink target + assert!(missing.exists()); + + // The timestamps for files and directories were changed from TIME_ONE + let file_times = get_times(&dirs.test().join("f")); + let dir_times = get_times(&dirs.test().join("d")); + let dir_file_times = get_times(&dirs.test().join("d/f")); + + assert_ne!(file_times, (TIME_ONE, TIME_ONE)); + assert_ne!(dir_times, (TIME_ONE, TIME_ONE)); + assert_ne!(dir_file_times, (TIME_ONE, TIME_ONE)); + + // For symlinks, they remain (mostly) the same + // We can't test accessed times, since to reach the target file, the symlink must be accessed! + let file_symlink_times = get_times(&dirs.test().join("fs")); + let dir_symlink_times = get_times(&dirs.test().join("ds")); + let dir_file_symlink_times = get_times(&dirs.test().join("fds")); + let file_missing_symlink_times = get_times(&dirs.test().join("fms")); + + assert_eq!(file_symlink_times.1, TIME_ONE); + assert_eq!(dir_symlink_times.1, TIME_ONE); + assert_eq!(dir_file_symlink_times.1, TIME_ONE); + assert_eq!(file_missing_symlink_times.1, TIME_ONE); + }) +} + +#[test] +fn no_follow_symlinks() { + Playground::setup("touch_touches_symlinks", |dirs, sandbox| { + setup_symlink_fs(&dirs, sandbox); + + let missing = dirs.test().join("m"); + assert!(!missing.exists()); + + nu!( + cwd: dirs.test(), + " + touch fds -s + touch ds -s + touch fs -s + touch fms -s + " + ); + + // We did not create the missing symlink target + assert!(!missing.exists()); + + // The timestamps for files and directories remain the same + let file_times = get_times(&dirs.test().join("f")); + let dir_times = get_times(&dirs.test().join("d")); + let dir_file_times = get_times(&dirs.test().join("d/f")); + + assert_eq!(file_times, (TIME_ONE, TIME_ONE)); + assert_eq!(dir_times, (TIME_ONE, TIME_ONE)); + assert_eq!(dir_file_times, (TIME_ONE, TIME_ONE)); + + // For symlinks, everything changed. (except their targets, and paths, and personality) + let file_symlink_times = get_times(&dirs.test().join("fs")); + let dir_symlink_times = get_times(&dirs.test().join("ds")); + let dir_file_symlink_times = get_times(&dirs.test().join("fds")); + let file_missing_symlink_times = get_times(&dirs.test().join("fms")); + + assert_ne!(file_symlink_times, (TIME_ONE, TIME_ONE)); + assert_ne!(dir_symlink_times, (TIME_ONE, TIME_ONE)); + assert_ne!(dir_file_symlink_times, (TIME_ONE, TIME_ONE)); + assert_ne!(file_missing_symlink_times, (TIME_ONE, TIME_ONE)); + }) +}