mv: refactor backup logic to use shared uucore backup control

- add mv backup tests

src/uu/mv/src/mv.rs

@@ -20,6 +20,7 @@ use std::os::unix;
 #[cfg(windows)]
 use std::os::windows;
 use std::path::{Path, PathBuf};
+use uucore::backup_control::{self, BackupMode};
 
 use fs_extra::dir::{move_dir, CopyOptions as DirCopyOptions};
 
@@ -40,16 +41,9 @@ pub enum OverwriteMode {
     Force,
 }
 
-#[derive(Clone, Copy, Eq, PartialEq)]
-pub enum BackupMode {
-    NoBackup,
-    SimpleBackup,
-    NumberedBackup,
-    ExistingBackup,
-}
-
 static ABOUT: &str = "Move SOURCE to DEST, or multiple SOURCE(s) to DIRECTORY.";
 static VERSION: &str = env!("CARGO_PKG_VERSION");
+static LONG_HELP: &str = "";
 
 static OPT_BACKUP: &str = "backup";
 static OPT_BACKUP_NO_ARG: &str = "b";
@@ -80,20 +74,16 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
     let matches = App::new(executable!())
         .version(VERSION)
         .about(ABOUT)
+        .after_help(&*format!("{}\n{}", LONG_HELP, backup_control::BACKUP_CONTROL_LONG_HELP))
         .usage(&usage[..])
     .arg(
             Arg::with_name(OPT_BACKUP)
             .long(OPT_BACKUP)
             .help("make a backup of each existing destination file")
             .takes_value(true)
-            .possible_value("simple")
-            .possible_value("never")
-            .possible_value("numbered")
-            .possible_value("t")
-            .possible_value("existing")
-            .possible_value("nil")
-            .possible_value("none")
-            .possible_value("off")
+            .require_equals(true)
+            .min_values(0)
+            .possible_values(backup_control::BACKUP_CONTROL_VALUES)
             .value_name("CONTROL")
     )
     .arg(
@@ -172,18 +162,17 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
         .unwrap_or_default();
 
     let overwrite_mode = determine_overwrite_mode(&matches);
-    let backup_mode = determine_backup_mode(&matches);
+    let backup_mode = backup_control::determine_backup_mode(
+        matches.is_present(OPT_BACKUP_NO_ARG) || matches.is_present(OPT_BACKUP),
+        matches.value_of(OPT_BACKUP),
+    );
 
     if overwrite_mode == OverwriteMode::NoClobber && backup_mode != BackupMode::NoBackup {
-        show_error!(
-            "options --backup and --no-clobber are mutually exclusive\n\
-             Try '{} --help' for more information.",
-            executable!()
-        );
+        show_usage_error!("options --backup and --no-clobber are mutually exclusive");
         return 1;
     }
 
-    let backup_suffix = determine_backup_suffix(backup_mode, &matches);
+    let backup_suffix = backup_control::determine_backup_suffix(matches.value_of(OPT_SUFFIX));
 
     let behavior = Behavior {
         overwrite: overwrite_mode,
@@ -227,37 +216,6 @@ fn determine_overwrite_mode(matches: &ArgMatches) -> OverwriteMode {
     }
 }
 
-fn determine_backup_mode(matches: &ArgMatches) -> BackupMode {
-    if matches.is_present(OPT_BACKUP_NO_ARG) {
-        BackupMode::SimpleBackup
-    } else if matches.is_present(OPT_BACKUP) {
-        match matches.value_of(OPT_BACKUP).map(String::from) {
-            None => BackupMode::SimpleBackup,
-            Some(mode) => match &mode[..] {
-                "simple" | "never" => BackupMode::SimpleBackup,
-                "numbered" | "t" => BackupMode::NumberedBackup,
-                "existing" | "nil" => BackupMode::ExistingBackup,
-                "none" | "off" => BackupMode::NoBackup,
-                _ => panic!(), // cannot happen as it is managed by clap
-            },
-        }
-    } else {
-        BackupMode::NoBackup
-    }
-}
-
-fn determine_backup_suffix(backup_mode: BackupMode, matches: &ArgMatches) -> String {
-    if matches.is_present(OPT_SUFFIX) {
-        matches.value_of(OPT_SUFFIX).map(String::from).unwrap()
-    } else if let (Ok(s), BackupMode::SimpleBackup) =
-        (env::var("SIMPLE_BACKUP_SUFFIX"), backup_mode)
-    {
-        s
-    } else {
-        "~".to_owned()
-    }
-}
-
 fn exec(files: &[PathBuf], b: Behavior) -> i32 {
     if let Some(ref name) = b.target_dir {
         return move_files_into_dir(files, &PathBuf::from(name), &b);
@@ -389,12 +347,7 @@ fn rename(from: &Path, to: &Path, b: &Behavior) -> io::Result<()> {
             OverwriteMode::Force => {}
         };
 
-        backup_path = match b.backup {
-            BackupMode::NoBackup => None,
-            BackupMode::SimpleBackup => Some(simple_backup_path(to, &b.suffix)),
-            BackupMode::NumberedBackup => Some(numbered_backup_path(to)),
-            BackupMode::ExistingBackup => Some(existing_backup_path(to, &b.suffix)),
-        };
+        backup_path = backup_control::get_backup_path(b.backup, to, &b.suffix);
         if let Some(ref backup_path) = backup_path {
             rename_with_fallback(to, backup_path)?;
         }
@@ -514,28 +467,6 @@ fn read_yes() -> bool {
     }
 }
 
-fn simple_backup_path(path: &Path, suffix: &str) -> PathBuf {
-    let mut p = path.to_string_lossy().into_owned();
-    p.push_str(suffix);
-    PathBuf::from(p)
-}
-
-fn numbered_backup_path(path: &Path) -> PathBuf {
-    (1_u64..)
-        .map(|i| path.with_extension(format!("~{}~", i)))
-        .find(|p| !p.exists())
-        .expect("cannot create backup")
-}
-
-fn existing_backup_path(path: &Path, suffix: &str) -> PathBuf {
-    let test_path = path.with_extension("~1~");
-    if test_path.exists() {
-        numbered_backup_path(path)
-    } else {
-        simple_backup_path(path, suffix)
-    }
-}
-
 fn is_empty_dir(path: &Path) -> bool {
     match fs::read_dir(path) {
         Ok(contents) => contents.peekable().peek().is_none(),

tests/by-util/test_mv.rs

@@ -251,6 +251,40 @@ fn test_mv_simple_backup() {
     assert!(at.file_exists(&format!("{}~", file_b)));
 }
 
+#[test]
+fn test_mv_simple_backup_with_file_extension() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    let file_a = "test_mv_simple_backup_file_a.txt";
+    let file_b = "test_mv_simple_backup_file_b.txt";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    ucmd.arg("-b")
+        .arg(file_a)
+        .arg(file_b)
+        .succeeds()
+        .no_stderr();
+
+    assert!(!at.file_exists(file_a));
+    assert!(at.file_exists(file_b));
+    assert!(at.file_exists(&format!("{}~", file_b)));
+}
+
+#[test]
+fn test_mv_arg_backup_arg_first() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    let file_a = "test_mv_simple_backup_file_a";
+    let file_b = "test_mv_simple_backup_file_b";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    ucmd.arg("--backup").arg(file_a).arg(file_b).succeeds();
+
+    assert!(!at.file_exists(file_a));
+    assert!(at.file_exists(file_b));
+    assert!(at.file_exists(&format!("{}~", file_b)));
+}
+
 #[test]
 fn test_mv_custom_backup_suffix() {
     let (at, mut ucmd) = at_and_ucmd!();
@@ -293,7 +327,7 @@ fn test_mv_custom_backup_suffix_via_env() {
 }
 
 #[test]
-fn test_mv_backup_numbering() {
+fn test_mv_backup_numbered_with_t() {
     let (at, mut ucmd) = at_and_ucmd!();
     let file_a = "test_mv_backup_numbering_file_a";
     let file_b = "test_mv_backup_numbering_file_b";
@@ -311,6 +345,25 @@ fn test_mv_backup_numbering() {
     assert!(at.file_exists(&format!("{}.~1~", file_b)));
 }
 
+#[test]
+fn test_mv_backup_numbered() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    let file_a = "test_mv_backup_numbering_file_a";
+    let file_b = "test_mv_backup_numbering_file_b";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    ucmd.arg("--backup=numbered")
+        .arg(file_a)
+        .arg(file_b)
+        .succeeds()
+        .no_stderr();
+
+    assert!(!at.file_exists(file_a));
+    assert!(at.file_exists(file_b));
+    assert!(at.file_exists(&format!("{}.~1~", file_b)));
+}
+
 #[test]
 fn test_mv_backup_existing() {
     let (at, mut ucmd) = at_and_ucmd!();
@@ -330,6 +383,67 @@ fn test_mv_backup_existing() {
     assert!(at.file_exists(&format!("{}~", file_b)));
 }
 
+#[test]
+fn test_mv_backup_nil() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    let file_a = "test_mv_backup_numbering_file_a";
+    let file_b = "test_mv_backup_numbering_file_b";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    ucmd.arg("--backup=nil")
+        .arg(file_a)
+        .arg(file_b)
+        .succeeds()
+        .no_stderr();
+
+    assert!(!at.file_exists(file_a));
+    assert!(at.file_exists(file_b));
+    assert!(at.file_exists(&format!("{}~", file_b)));
+}
+
+#[test]
+fn test_mv_numbered_if_existing_backup_existing() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    let file_a = "test_mv_backup_numbering_file_a";
+    let file_b = "test_mv_backup_numbering_file_b";
+    let file_b_backup = "test_mv_backup_numbering_file_b.~1~";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    at.touch(file_b_backup);
+    ucmd.arg("--backup=existing")
+        .arg(file_a)
+        .arg(file_b)
+        .succeeds()
+        .no_stderr();
+
+    assert!(at.file_exists(file_b));
+    assert!(at.file_exists(file_b_backup));
+    assert!(at.file_exists(&*format!("{}.~2~", file_b)));
+}
+
+#[test]
+fn test_mv_numbered_if_existing_backup_nil() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    let file_a = "test_mv_backup_numbering_file_a";
+    let file_b = "test_mv_backup_numbering_file_b";
+    let file_b_backup = "test_mv_backup_numbering_file_b.~1~";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    at.touch(file_b_backup);
+    ucmd.arg("--backup=nil")
+        .arg(file_a)
+        .arg(file_b)
+        .succeeds()
+        .no_stderr();
+
+    assert!(at.file_exists(file_b));
+    assert!(at.file_exists(file_b_backup));
+    assert!(at.file_exists(&*format!("{}.~2~", file_b)));
+}
+
 #[test]
 fn test_mv_backup_simple() {
     let (at, mut ucmd) = at_and_ucmd!();
@@ -349,6 +463,25 @@ fn test_mv_backup_simple() {
     assert!(at.file_exists(&format!("{}~", file_b)));
 }
 
+#[test]
+fn test_mv_backup_never() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    let file_a = "test_mv_backup_numbering_file_a";
+    let file_b = "test_mv_backup_numbering_file_b";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    ucmd.arg("--backup=never")
+        .arg(file_a)
+        .arg(file_b)
+        .succeeds()
+        .no_stderr();
+
+    assert!(!at.file_exists(file_a));
+    assert!(at.file_exists(file_b));
+    assert!(at.file_exists(&format!("{}~", file_b)));
+}
+
 #[test]
 fn test_mv_backup_none() {
     let (at, mut ucmd) = at_and_ucmd!();
@@ -369,17 +502,14 @@ fn test_mv_backup_none() {
 }
 
 #[test]
-fn test_mv_existing_backup() {
+fn test_mv_backup_off() {
     let (at, mut ucmd) = at_and_ucmd!();
-    let file_a = "test_mv_existing_backup_file_a";
-    let file_b = "test_mv_existing_backup_file_b";
-    let file_b_backup = "test_mv_existing_backup_file_b.~1~";
-    let resulting_backup = "test_mv_existing_backup_file_b.~2~";
+    let file_a = "test_mv_backup_numbering_file_a";
+    let file_b = "test_mv_backup_numbering_file_b";
 
     at.touch(file_a);
     at.touch(file_b);
-    at.touch(file_b_backup);
-    ucmd.arg("--backup=nil")
+    ucmd.arg("--backup=off")
         .arg(file_a)
         .arg(file_b)
         .succeeds()
@@ -387,8 +517,19 @@ fn test_mv_existing_backup() {
 
     assert!(!at.file_exists(file_a));
     assert!(at.file_exists(file_b));
-    assert!(at.file_exists(file_b_backup));
-    assert!(at.file_exists(resulting_backup));
+    assert!(!at.file_exists(&format!("{}~", file_b)));
+}
+
+#[test]
+fn test_mv_backup_no_clobber_conflicting_options() {
+    let (_, mut ucmd) = at_and_ucmd!();
+
+    ucmd.arg("--backup")
+        .arg("--no-clobber")
+        .arg("file1")
+        .arg("file2")
+        .fails()
+        .stderr_is("mv: options --backup and --no-clobber are mutually exclusive\nTry 'mv --help' for more information.");
 }
 
 #[test]