From 38a42905aec6e229f45736f7db89248d0a521280 Mon Sep 17 00:00:00 2001 From: kik4444 <7779637+kik4444@users.noreply.github.com> Date: Fri, 1 Mar 2024 15:23:03 +0200 Subject: [PATCH] Fix `touch` to allow changing timestamps on directories, remake from #11760 (#12005) Based off of #11760 to be mergable without conflicts. # Description Fix for #11757. The main issue in #11757 is I tried to copy the timestamp from one directory to another only to realize that did not work whereas the coreutils `^touch` had no problems. I thought `--reference` just did not work, but apparently the whole `touch` command could not work on directories because `OpenOptions::new().write(true).create(true).open(&item)` tries to create `touch`'s target in advance and then modify its timestamps. But if the target is a directory that already exists then this would fail even though the crate used for working with timestamps, `filetime`, already works on directories. # User-Facing Changes I don't believe this should change any existing valid behaviors. It just changes a non-working behavior. # Tests + Formatting ~~I only could not run `cargo test` because I get compilation errors on the latest main branch~~ All tests pass with `cargo test --features=sqlite` # After Submitting --- crates/nu-command/src/filesystem/touch.rs | 168 ++++------ crates/nu-command/tests/commands/touch.rs | 384 ++++++++++++++++++++-- 2 files changed, 420 insertions(+), 132 deletions(-) diff --git a/crates/nu-command/src/filesystem/touch.rs b/crates/nu-command/src/filesystem/touch.rs index 095f3e556d..1f0ec4fa3f 100644 --- a/crates/nu-command/src/filesystem/touch.rs +++ b/crates/nu-command/src/filesystem/touch.rs @@ -1,7 +1,6 @@ -use std::fs::OpenOptions; use std::path::Path; +use std::{fs::OpenOptions, time::SystemTime}; -use chrono::{DateTime, Local}; use filetime::FileTime; use nu_engine::CallExt; @@ -64,7 +63,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 use_reference: bool = call.has_flag(engine_state, stack, "reference")?; + 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 = call.rest(engine_state, stack, 0)?; @@ -75,88 +74,73 @@ impl Command for Touch { }); } - let mut date: Option> = None; - let mut ref_date_atime: Option> = None; + let mut mtime = SystemTime::now(); + let mut atime = mtime; - // Change both times if none is specified + // Change both times if neither is specified if !change_mtime && !change_atime { change_mtime = true; change_atime = true; } - if change_mtime || change_atime { - date = Some(Local::now()); - } - - if use_reference { - let reference: Option> = - call.get_flag(engine_state, stack, "reference")?; - match reference { - Some(reference) => { - let reference_path = Path::new(&reference.item); - if !reference_path.exists() { - return Err(ShellError::TypeMismatch { - err_message: "path provided is invalid".to_string(), - span: reference.span, - }); - } - - date = Some( - reference_path - .metadata() - .expect("should be a valid path") // Should never fail as the path exists - .modified() - .expect("should have metadata") // This should always be valid as it is available on all nushell's supported platforms (Linux, Windows, MacOS) - .into(), - ); - - ref_date_atime = Some( - reference_path - .metadata() - .expect("should be a valid path") // Should never fail as the path exists - .accessed() - .expect("should have metadata") // This should always be valid as it is available on all nushell's supported platforms (Linux, Windows, MacOS) - .into(), - ); - } - None => { - return Err(ShellError::MissingParameter { - param_name: "reference".to_string(), - span: call.head, - }); - } + if let Some(reference) = reference { + let reference_path = Path::new(&reference.item); + if !reference_path.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, + })?; + mtime = metadata + .modified() + .map_err(|err| ShellError::IOErrorSpanned { + msg: format!("Failed to read modified time: {err}"), + span: reference.span, + })?; + atime = metadata + .accessed() + .map_err(|err| ShellError::IOErrorSpanned { + msg: format!("Failed to read access time: {err}"), + span: reference.span, + })?; } for (index, item) in files.into_iter().enumerate() { - if no_create { - let path = Path::new(&item); - if !path.exists() { - continue; - } + let path = Path::new(&item); + + // If --no-create is passed and the file/dir does not exist there's nothing to do + if no_create && !path.exists() { + continue; } - if let Err(err) = OpenOptions::new() - .write(true) - .create(true) - .truncate(false) - .open(&item) - { - return Err(ShellError::CreateNotPossible { - msg: format!("Failed to create file: {err}"), - span: call - .positional_nth(index) - .expect("already checked positional") - .span, - }); - }; + // Create a file at the given path unless the path is a directory + if !path.is_dir() { + if let Err(err) = OpenOptions::new() + .write(true) + .create(true) + .truncate(false) + .open(path) + { + return Err(ShellError::CreateNotPossible { + msg: format!("Failed to create file: {err}"), + span: call + .positional_nth(index) + .expect("already checked positional") + .span, + }); + }; + } if change_mtime { - // Should not panic as we return an error above if we can't parse the date - if let Err(err) = filetime::set_file_mtime( - &item, - FileTime::from_system_time(date.expect("should be a valid date").into()), - ) { + if let Err(err) = filetime::set_file_mtime(&item, FileTime::from_system_time(mtime)) + { return Err(ShellError::ChangeModifiedTimeNotPossible { msg: format!("Failed to change the modified time: {err}"), span: call @@ -168,38 +152,16 @@ impl Command for Touch { } if change_atime { - // Reference file/directory may have different access and modified times - if use_reference { - // Should not panic as we return an error above if we can't parse the date - if let Err(err) = filetime::set_file_atime( - &item, - FileTime::from_system_time( - ref_date_atime.expect("should be a valid date").into(), - ), - ) { - return Err(ShellError::ChangeAccessTimeNotPossible { - msg: format!("Failed to change the access time: {err}"), - span: call - .positional_nth(index) - .expect("already checked positional") - .span, - }); - }; - } else { - // Should not panic as we return an error above if we can't parse the date - if let Err(err) = filetime::set_file_atime( - &item, - FileTime::from_system_time(date.expect("should be a valid date").into()), - ) { - return Err(ShellError::ChangeAccessTimeNotPossible { - msg: format!("Failed to change the access time: {err}"), - span: call - .positional_nth(index) - .expect("already checked positional") - .span, - }); - }; - } + if let Err(err) = filetime::set_file_atime(&item, FileTime::from_system_time(atime)) + { + return Err(ShellError::ChangeAccessTimeNotPossible { + msg: format!("Failed to change the access time: {err}"), + span: call + .positional_nth(index) + .expect("already checked positional") + .span, + }); + }; } } diff --git a/crates/nu-command/tests/commands/touch.rs b/crates/nu-command/tests/commands/touch.rs index 3b0d55f0fc..1b7c9df783 100644 --- a/crates/nu-command/tests/commands/touch.rs +++ b/crates/nu-command/tests/commands/touch.rs @@ -3,6 +3,9 @@ use nu_test_support::fs::Stub; use nu_test_support::nu; use nu_test_support::playground::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); + #[test] fn creates_a_file_when_it_doesnt_exist() { Playground::setup("create_test_1", |dirs, _sandbox| { @@ -36,21 +39,29 @@ fn creates_two_files() { fn change_modified_time_of_file_to_today() { Playground::setup("change_time_test_9", |dirs, sandbox| { sandbox.with_files(vec![Stub::EmptyFile("file.txt")]); + let path = dirs.test().join("file.txt"); + + // Set file.txt's times to the past before the test to make sure `touch` actually changes the mtime to today + filetime::set_file_times(&path, TIME_ONE, TIME_ONE).unwrap(); nu!( cwd: dirs.test(), "touch -m file.txt" ); - let path = dirs.test().join("file.txt"); + let metadata = path.metadata().unwrap(); // Check only the date since the time may not match exactly - let date = Local::now().date_naive(); - let actual_date_time: DateTime = - DateTime::from(path.metadata().unwrap().modified().unwrap()); - let actual_date = actual_date_time.date_naive(); + let today = Local::now().date_naive(); + let mtime_day = DateTime::::from(metadata.modified().unwrap()).date_naive(); - assert_eq!(date, actual_date); + assert_eq!(today, mtime_day); + + // Check that atime remains unchanged + assert_eq!( + TIME_ONE, + filetime::FileTime::from_system_time(metadata.accessed().unwrap()) + ); }) } @@ -58,21 +69,29 @@ fn change_modified_time_of_file_to_today() { fn change_access_time_of_file_to_today() { Playground::setup("change_time_test_18", |dirs, sandbox| { sandbox.with_files(vec![Stub::EmptyFile("file.txt")]); + let path = dirs.test().join("file.txt"); + + // Set file.txt's times to the past before the test to make sure `touch` actually changes the atime to today + filetime::set_file_times(&path, TIME_ONE, TIME_ONE).unwrap(); nu!( cwd: dirs.test(), "touch -a file.txt" ); - let path = dirs.test().join("file.txt"); + let metadata = path.metadata().unwrap(); // Check only the date since the time may not match exactly - let date = Local::now().date_naive(); - let actual_date_time: DateTime = - DateTime::from(path.metadata().unwrap().accessed().unwrap()); - let actual_date = actual_date_time.date_naive(); + let today = Local::now().date_naive(); + let atime_day = DateTime::::from(metadata.accessed().unwrap()).date_naive(); - assert_eq!(date, actual_date); + assert_eq!(today, atime_day); + + // Check that mtime remains unchanged + assert_eq!( + TIME_ONE, + filetime::FileTime::from_system_time(metadata.modified().unwrap()) + ); }) } @@ -80,30 +99,31 @@ fn change_access_time_of_file_to_today() { fn change_modified_and_access_time_of_file_to_today() { Playground::setup("change_time_test_27", |dirs, sandbox| { sandbox.with_files(vec![Stub::EmptyFile("file.txt")]); + let path = dirs.test().join("file.txt"); + + filetime::set_file_times(&path, TIME_ONE, TIME_ONE).unwrap(); nu!( cwd: dirs.test(), "touch -a -m file.txt" ); - let metadata = dirs.test().join("file.txt").metadata().unwrap(); + let metadata = path.metadata().unwrap(); // Check only the date since the time may not match exactly - let date = Local::now().date_naive(); - let adate_time: DateTime = DateTime::from(metadata.accessed().unwrap()); - let adate = adate_time.date_naive(); - let mdate_time: DateTime = DateTime::from(metadata.modified().unwrap()); - let mdate = mdate_time.date_naive(); + let today = Local::now().date_naive(); + let mtime_day = DateTime::::from(metadata.modified().unwrap()).date_naive(); + let atime_day = DateTime::::from(metadata.accessed().unwrap()).date_naive(); - assert_eq!(date, adate); - assert_eq!(date, mdate); + assert_eq!(today, mtime_day); + assert_eq!(today, atime_day); }) } #[test] fn not_create_file_if_it_not_exists() { Playground::setup("change_time_test_28", |dirs, _sandbox| { - nu!( + let outcome = nu!( cwd: dirs.test(), "touch -c file.txt" ); @@ -112,17 +132,39 @@ fn not_create_file_if_it_not_exists() { assert!(!path.exists()); - nu!( - cwd: dirs.test(), - "touch -c file.txt" - ); - - let path = dirs.test().join("file.txt"); - - assert!(!path.exists()); + // If --no-create is improperly handled `touch` may error when trying to change the times of a nonexistent file + assert!(outcome.status.success()) }) } +#[test] +fn change_file_times_if_exists_with_no_create() { + Playground::setup( + "change_file_times_if_exists_with_no_create", + |dirs, sandbox| { + sandbox.with_files(vec![Stub::EmptyFile("file.txt")]); + let path = dirs.test().join("file.txt"); + + filetime::set_file_times(&path, TIME_ONE, TIME_ONE).unwrap(); + + nu!( + cwd: dirs.test(), + "touch -c file.txt" + ); + + let metadata = path.metadata().unwrap(); + + // Check only the date since the time may not match exactly + let today = Local::now().date_naive(); + let mtime_day = DateTime::::from(metadata.modified().unwrap()).date_naive(); + let atime_day = DateTime::::from(metadata.accessed().unwrap()).date_naive(); + + assert_eq!(today, mtime_day); + assert_eq!(today, atime_day); + }, + ) +} + #[test] fn creates_file_three_dots() { Playground::setup("create_test_1", |dirs, _sandbox| { @@ -161,3 +203,287 @@ fn creates_file_four_dots_quotation_marks() { assert!(path.exists()); }) } + +#[test] +fn change_file_times_to_reference_file() { + Playground::setup("change_dir_times_to_reference_dir", |dirs, sandbox| { + sandbox.with_files(vec![ + Stub::EmptyFile("reference_file"), + Stub::EmptyFile("target_file"), + ]); + + let reference = dirs.test().join("reference_file"); + let target = dirs.test().join("target_file"); + + // Change the times for reference + filetime::set_file_times( + &reference, + filetime::FileTime::from_unix_time(1337, 0), + TIME_ONE, + ) + .unwrap(); + + // target should have today's date since it was just created, but reference should be different + assert_ne!( + reference.metadata().unwrap().accessed().unwrap(), + target.metadata().unwrap().accessed().unwrap() + ); + assert_ne!( + reference.metadata().unwrap().modified().unwrap(), + target.metadata().unwrap().modified().unwrap() + ); + + nu!( + cwd: dirs.test(), + "touch -r reference_file target_file" + ); + + assert_eq!( + reference.metadata().unwrap().accessed().unwrap(), + target.metadata().unwrap().accessed().unwrap() + ); + assert_eq!( + reference.metadata().unwrap().modified().unwrap(), + target.metadata().unwrap().modified().unwrap() + ); + }) +} + +#[test] +fn change_file_mtime_to_reference() { + Playground::setup("change_file_mtime_to_reference", |dirs, sandbox| { + sandbox.with_files(vec![ + Stub::EmptyFile("reference_file"), + Stub::EmptyFile("target_file"), + ]); + + let reference = dirs.test().join("reference_file"); + let target = dirs.test().join("target_file"); + + // Change the times for reference + filetime::set_file_times( + &reference, + TIME_ONE, + filetime::FileTime::from_unix_time(1337, 0), + ) + .unwrap(); + + // target should have today's date since it was just created, but reference should be different + assert_ne!( + reference.metadata().unwrap().accessed().unwrap(), + target.metadata().unwrap().accessed().unwrap() + ); + assert_ne!( + reference.metadata().unwrap().modified().unwrap(), + target.metadata().unwrap().modified().unwrap() + ); + + // Save target's current atime to make sure it is preserved + let target_original_atime = target.metadata().unwrap().accessed().unwrap(); + + nu!( + cwd: dirs.test(), + "touch -mr reference_file target_file" + ); + + assert_eq!( + reference.metadata().unwrap().modified().unwrap(), + target.metadata().unwrap().modified().unwrap() + ); + assert_ne!( + reference.metadata().unwrap().accessed().unwrap(), + target.metadata().unwrap().accessed().unwrap() + ); + assert_eq!( + target_original_atime, + target.metadata().unwrap().accessed().unwrap() + ); + }) +} + +#[test] +fn change_modified_time_of_dir_to_today() { + Playground::setup("change_dir_mtime", |dirs, sandbox| { + sandbox.mkdir("test_dir"); + let path = dirs.test().join("test_dir"); + + filetime::set_file_mtime(&path, TIME_ONE).unwrap(); + + nu!( + cwd: dirs.test(), + "touch -m test_dir" + ); + + // Check only the date since the time may not match exactly + let today = Local::now().date_naive(); + let mtime_day = + DateTime::::from(path.metadata().unwrap().modified().unwrap()).date_naive(); + + assert_eq!(today, mtime_day); + }) +} + +#[test] +fn change_access_time_of_dir_to_today() { + Playground::setup("change_dir_atime", |dirs, sandbox| { + sandbox.mkdir("test_dir"); + let path = dirs.test().join("test_dir"); + + filetime::set_file_atime(&path, TIME_ONE).unwrap(); + + nu!( + cwd: dirs.test(), + "touch -a test_dir" + ); + + // Check only the date since the time may not match exactly + let today = Local::now().date_naive(); + let atime_day = + DateTime::::from(path.metadata().unwrap().accessed().unwrap()).date_naive(); + + assert_eq!(today, atime_day); + }) +} + +#[test] +fn change_modified_and_access_time_of_dir_to_today() { + Playground::setup("change_dir_times", |dirs, sandbox| { + sandbox.mkdir("test_dir"); + let path = dirs.test().join("test_dir"); + + filetime::set_file_times(&path, TIME_ONE, TIME_ONE).unwrap(); + + nu!( + cwd: dirs.test(), + "touch -a -m test_dir" + ); + + let metadata = path.metadata().unwrap(); + + // Check only the date since the time may not match exactly + let today = Local::now().date_naive(); + let mtime_day = DateTime::::from(metadata.modified().unwrap()).date_naive(); + let atime_day = DateTime::::from(metadata.accessed().unwrap()).date_naive(); + + assert_eq!(today, mtime_day); + assert_eq!(today, atime_day); + }) +} + +#[test] +fn change_dir_three_dots_times() { + Playground::setup("change_dir_three_dots_times", |dirs, sandbox| { + sandbox.mkdir("test_dir..."); + let path = dirs.test().join("test_dir..."); + + filetime::set_file_times(&path, TIME_ONE, TIME_ONE).unwrap(); + + nu!( + cwd: dirs.test(), + "touch test_dir..." + ); + + let metadata = path.metadata().unwrap(); + + // Check only the date since the time may not match exactly + let today = Local::now().date_naive(); + let mtime_day = DateTime::::from(metadata.modified().unwrap()).date_naive(); + let atime_day = DateTime::::from(metadata.accessed().unwrap()).date_naive(); + + assert_eq!(today, mtime_day); + assert_eq!(today, atime_day); + }) +} + +#[test] +fn change_dir_times_to_reference_dir() { + Playground::setup("change_dir_times_to_reference_dir", |dirs, sandbox| { + sandbox.mkdir("reference_dir"); + sandbox.mkdir("target_dir"); + + let reference = dirs.test().join("reference_dir"); + let target = dirs.test().join("target_dir"); + + // Change the times for reference + filetime::set_file_times( + &reference, + filetime::FileTime::from_unix_time(1337, 0), + TIME_ONE, + ) + .unwrap(); + + // target should have today's date since it was just created, but reference should be different + assert_ne!( + reference.metadata().unwrap().accessed().unwrap(), + target.metadata().unwrap().accessed().unwrap() + ); + assert_ne!( + reference.metadata().unwrap().modified().unwrap(), + target.metadata().unwrap().modified().unwrap() + ); + + nu!( + cwd: dirs.test(), + "touch -r reference_dir target_dir" + ); + + assert_eq!( + reference.metadata().unwrap().accessed().unwrap(), + target.metadata().unwrap().accessed().unwrap() + ); + assert_eq!( + reference.metadata().unwrap().modified().unwrap(), + target.metadata().unwrap().modified().unwrap() + ); + }) +} + +#[test] +fn change_dir_atime_to_reference() { + Playground::setup("change_dir_atime_to_reference", |dirs, sandbox| { + sandbox.mkdir("reference_dir"); + sandbox.mkdir("target_dir"); + + let reference = dirs.test().join("reference_dir"); + let target = dirs.test().join("target_dir"); + + // Change the times for reference + filetime::set_file_times( + &reference, + filetime::FileTime::from_unix_time(1337, 0), + TIME_ONE, + ) + .unwrap(); + + // target should have today's date since it was just created, but reference should be different + assert_ne!( + reference.metadata().unwrap().accessed().unwrap(), + target.metadata().unwrap().accessed().unwrap() + ); + assert_ne!( + reference.metadata().unwrap().modified().unwrap(), + target.metadata().unwrap().modified().unwrap() + ); + + // Save target's current mtime to make sure it is preserved + let target_original_mtime = target.metadata().unwrap().modified().unwrap(); + + nu!( + cwd: dirs.test(), + "touch -ar reference_dir target_dir" + ); + + assert_eq!( + reference.metadata().unwrap().accessed().unwrap(), + target.metadata().unwrap().accessed().unwrap() + ); + assert_ne!( + reference.metadata().unwrap().modified().unwrap(), + target.metadata().unwrap().modified().unwrap() + ); + assert_eq!( + target_original_mtime, + target.metadata().unwrap().modified().unwrap() + ); + }) +}