Merge pull request #8521 from sylvestre/mv-special

mv: improve the verbose mode to make tests/mv/mv-special-1.sh pass

Author: cakebaker

src/uu/mv/src/mv.rs

@@ -603,12 +603,12 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, options: &Options)
         return Err(MvError::NotADirectory(target_dir.quote().to_string()).into());
     }
 
-    let multi_progress = options.progress_bar.then(MultiProgress::new);
+    let display_manager = options.progress_bar.then(MultiProgress::new);
 
-    let count_progress = if let Some(ref multi_progress) = multi_progress {
+    let count_progress = if let Some(ref display_manager) = display_manager {
         if files.len() > 1 {
             Some(
-                multi_progress.add(
+                display_manager.add(
                     ProgressBar::new(files.len().try_into().unwrap()).with_style(
                         ProgressStyle::with_template(&format!(
                             "{} {{msg}} {{wide_bar}} {{pos}}/{{len}}",
@@ -669,7 +669,7 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, options: &Options)
             sourcepath,
             &targetpath,
             options,
-            multi_progress.as_ref(),
+            display_manager.as_ref(),
             hardlink_params.0,
             hardlink_params.1,
         ) {
@@ -678,7 +678,7 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, options: &Options)
                 let e = e.map_err_context(|| {
                     translate!("mv-error-cannot-move", "source" => sourcepath.quote(), "target" => targetpath.quote())
                 });
-                match multi_progress {
+                match display_manager {
                     Some(ref pb) => pb.suspend(|| show!(e)),
                     None => show!(e),
                 }
@@ -697,7 +697,7 @@ fn rename(
     from: &Path,
     to: &Path,
     opts: &Options,
-    multi_progress: Option<&MultiProgress>,
+    display_manager: Option<&MultiProgress>,
     #[cfg(unix)] hardlink_tracker: Option<&mut HardlinkTracker>,
     #[cfg(unix)] hardlink_scanner: Option<&HardlinkGroupScanner>,
     #[cfg(not(unix))] _hardlink_tracker: Option<()>,
@@ -745,7 +745,7 @@ fn rename(
         backup_path = backup_control::get_backup_path(opts.backup, to, &opts.suffix);
         if let Some(ref backup_path) = backup_path {
             // For backup renames, we don't need to track hardlinks as we're just moving the existing file
-            rename_with_fallback(to, backup_path, multi_progress, None, None)?;
+            rename_with_fallback(to, backup_path, display_manager, false, None, None)?;
         }
     }
 
@@ -763,11 +763,18 @@ fn rename(
 
     #[cfg(unix)]
     {
-        rename_with_fallback(from, to, multi_progress, hardlink_tracker, hardlink_scanner)?;
+        rename_with_fallback(
+            from,
+            to,
+            display_manager,
+            opts.verbose,
+            hardlink_tracker,
+            hardlink_scanner,
+        )?;
     }
     #[cfg(not(unix))]
     {
-        rename_with_fallback(from, to, multi_progress, None, None)?;
+        rename_with_fallback(from, to, display_manager, opts.verbose, None, None)?;
     }
 
     #[cfg(feature = "selinux")]
@@ -784,7 +791,7 @@ fn rename(
             None => translate!("mv-verbose-renamed", "from" => from.quote(), "to" => to.quote()),
         };
 
-        match multi_progress {
+        match display_manager {
             Some(pb) => pb.suspend(|| {
                 println!("{message}");
             }),
@@ -809,7 +816,8 @@ fn is_fifo(_filetype: fs::FileType) -> bool {
 fn rename_with_fallback(
     from: &Path,
     to: &Path,
-    multi_progress: Option<&MultiProgress>,
+    display_manager: Option<&MultiProgress>,
+    verbose: bool,
     #[cfg(unix)] hardlink_tracker: Option<&mut HardlinkTracker>,
     #[cfg(unix)] hardlink_scanner: Option<&HardlinkGroupScanner>,
     #[cfg(not(unix))] _hardlink_tracker: Option<()>,
@@ -842,13 +850,20 @@ fn rename_with_fallback(
                     hardlink_tracker,
                     hardlink_scanner,
                     |tracker, scanner| {
-                        rename_dir_fallback(from, to, multi_progress, Some(tracker), Some(scanner))
+                        rename_dir_fallback(
+                            from,
+                            to,
+                            display_manager,
+                            verbose,
+                            Some(tracker),
+                            Some(scanner),
+                        )
                     },
                 )
             }
             #[cfg(not(unix))]
             {
-                rename_dir_fallback(from, to, multi_progress)
+                rename_dir_fallback(from, to, display_manager, verbose)
             }
         } else if is_fifo(file_type) {
             rename_fifo_fallback(from, to)
@@ -921,7 +936,8 @@ fn rename_symlink_fallback(from: &Path, to: &Path) -> io::Result<()> {
 fn rename_dir_fallback(
     from: &Path,
     to: &Path,
-    multi_progress: Option<&MultiProgress>,
+    display_manager: Option<&MultiProgress>,
+    verbose: bool,
     #[cfg(unix)] hardlink_tracker: Option<&mut HardlinkTracker>,
     #[cfg(unix)] hardlink_scanner: Option<&HardlinkGroupScanner>,
 ) -> io::Result<()> {
@@ -939,12 +955,12 @@ fn rename_dir_fallback(
     //    (Move will probably fail due to permission error later?)
     let total_size = dir_get_size(from).ok();
 
-    let progress_bar = match (multi_progress, total_size) {
-        (Some(multi_progress), Some(total_size)) => {
+    let progress_bar = match (display_manager, total_size) {
+        (Some(display_manager), Some(total_size)) => {
             let template = "{msg}: [{elapsed_precise}] {wide_bar} {bytes:>7}/{total_bytes:7}";
             let style = ProgressStyle::with_template(template).unwrap();
             let bar = ProgressBar::new(total_size).with_style(style);
-            Some(multi_progress.add(bar))
+            Some(display_manager.add(bar))
         }
         (_, _) => None,
     };
@@ -960,7 +976,9 @@ fn rename_dir_fallback(
         hardlink_tracker,
         #[cfg(unix)]
         hardlink_scanner,
+        verbose,
         progress_bar.as_ref(),
+        display_manager,
     );
 
     #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
@@ -980,7 +998,9 @@ fn copy_dir_contents(
     to: &Path,
     #[cfg(unix)] hardlink_tracker: Option<&mut HardlinkTracker>,
     #[cfg(unix)] hardlink_scanner: Option<&HardlinkGroupScanner>,
+    verbose: bool,
     progress_bar: Option<&ProgressBar>,
+    display_manager: Option<&MultiProgress>,
 ) -> io::Result<()> {
     // Create the destination directory
     fs::create_dir_all(to)?;
@@ -989,12 +1009,20 @@ fn copy_dir_contents(
     #[cfg(unix)]
     {
         if let (Some(tracker), Some(scanner)) = (hardlink_tracker, hardlink_scanner) {
-            copy_dir_contents_recursive(from, to, tracker, scanner, progress_bar)?;
+            copy_dir_contents_recursive(
+                from,
+                to,
+                tracker,
+                scanner,
+                verbose,
+                progress_bar,
+                display_manager,
+            )?;
         }
     }
     #[cfg(not(unix))]
     {
-        copy_dir_contents_recursive(from, to, progress_bar)?;
+        copy_dir_contents_recursive(from, to, None, None, verbose, progress_bar, display_manager)?;
     }
 
     Ok(())
@@ -1005,7 +1033,11 @@ fn copy_dir_contents_recursive(
     to_dir: &Path,
     #[cfg(unix)] hardlink_tracker: &mut HardlinkTracker,
     #[cfg(unix)] hardlink_scanner: &HardlinkGroupScanner,
+    #[cfg(not(unix))] _hardlink_tracker: Option<()>,
+    #[cfg(not(unix))] _hardlink_scanner: Option<()>,
+    verbose: bool,
     progress_bar: Option<&ProgressBar>,
+    display_manager: Option<&MultiProgress>,
 ) -> io::Result<()> {
     let entries = fs::read_dir(from_dir)?;
 
@@ -1022,14 +1054,32 @@ fn copy_dir_contents_recursive(
         if from_path.is_dir() {
             // Recursively copy subdirectory
             fs::create_dir_all(&to_path)?;
+
+            // Print verbose message for directory
+            if verbose {
+                let message = translate!("mv-verbose-renamed", "from" => from_path.quote(), "to" => to_path.quote());
+                match display_manager {
+                    Some(pb) => pb.suspend(|| {
+                        println!("{message}");
+                    }),
+                    None => println!("{message}"),
+                }
+            }
+
             copy_dir_contents_recursive(
                 &from_path,
                 &to_path,
                 #[cfg(unix)]
                 hardlink_tracker,
                 #[cfg(unix)]
                 hardlink_scanner,
+                #[cfg(not(unix))]
+                _hardlink_tracker,
+                #[cfg(not(unix))]
+                _hardlink_scanner,
+                verbose,
                 progress_bar,
+                display_manager,
             )?;
         } else {
             // Copy file with or without hardlink support based on platform
@@ -1046,6 +1096,17 @@ fn copy_dir_contents_recursive(
             {
                 fs::copy(&from_path, &to_path)?;
             }
+
+            // Print verbose message for file
+            if verbose {
+                let message = translate!("mv-verbose-renamed", "from" => from_path.quote(), "to" => to_path.quote());
+                match display_manager {
+                    Some(pb) => pb.suspend(|| {
+                        println!("{message}");
+                    }),
+                    None => println!("{message}"),
+                }
+            }
         }
 
         if let Some(pb) = progress_bar {

tests/by-util/test_mv.rs

@@ -2660,3 +2660,58 @@ fn test_mv_error_usage_display_too_few() {
         .stderr_contains("Usage: mv [OPTION]... [-T] SOURCE DEST")
         .stderr_contains("For more information, try '--help'.");
 }
+
+#[test]
+#[cfg(target_os = "linux")]
+fn test_mv_verbose_directory_recursive() {
+    use tempfile::TempDir;
+
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    at.mkdir("mv-dir");
+    at.mkdir("mv-dir/a");
+    at.mkdir("mv-dir/a/b");
+    at.mkdir("mv-dir/a/b/c");
+    at.mkdir("mv-dir/d");
+    at.mkdir("mv-dir/d/e");
+    at.mkdir("mv-dir/d/e/f");
+    at.touch("mv-dir/a/b/c/file1");
+    at.touch("mv-dir/d/e/f/file2");
+
+    // Force cross-filesystem move using /dev/shm (tmpfs)
+    let target_dir =
+        TempDir::new_in("/dev/shm/").expect("Unable to create temp directory in /dev/shm");
+    let target_path = target_dir.path().to_str().unwrap();
+
+    let result = scene
+        .ucmd()
+        .arg("--verbose")
+        .arg("mv-dir")
+        .arg(target_path)
+        .succeeds();
+
+    // Check that the directory structure was moved
+    assert!(!at.dir_exists("mv-dir"));
+    assert!(target_dir.path().join("mv-dir").exists());
+    assert!(target_dir.path().join("mv-dir/a").exists());
+    assert!(target_dir.path().join("mv-dir/a/b").exists());
+    assert!(target_dir.path().join("mv-dir/a/b/c").exists());
+    assert!(target_dir.path().join("mv-dir/d").exists());
+    assert!(target_dir.path().join("mv-dir/d/e").exists());
+    assert!(target_dir.path().join("mv-dir/d/e/f").exists());
+    assert!(target_dir.path().join("mv-dir/a/b/c/file1").exists());
+    assert!(target_dir.path().join("mv-dir/d/e/f/file2").exists());
+
+    let stdout = result.stdout_str();
+
+    // With cross-filesystem move, we MUST see recursive verbose output
+    assert!(stdout.contains("'mv-dir/a' -> "));
+    assert!(stdout.contains("'mv-dir/a/b' -> "));
+    assert!(stdout.contains("'mv-dir/a/b/c' -> "));
+    assert!(stdout.contains("'mv-dir/a/b/c/file1' -> "));
+    assert!(stdout.contains("'mv-dir/d' -> "));
+    assert!(stdout.contains("'mv-dir/d/e' -> "));
+    assert!(stdout.contains("'mv-dir/d/e/f' -> "));
+    assert!(stdout.contains("'mv-dir/d/e/f/file2' -> "));
+}