Merge pull request uutils#2467 from Funky185540/backup_mode_determina…

…tion backup_control: Fix backup mode determination
dhodder · Jul 20, 2021 · eae8c72 · eae8c72
2 parents fd89d59 + e07b4e9
commit eae8c72
Show file tree

Hide file tree

Showing 7 changed files with 453 additions and 88 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs
@@ -98,6 +98,9 @@ quick_error! {
         /// path, but those that are not implemented yet should return
         /// a NotImplemented error.
         NotImplemented(opt: String) { display("Option '{}' not yet implemented.", opt) }
+
+        /// Invalid arguments to backup
+        Backup(description: String) { display("{}\nTry 'cp --help' for more information.", description) }
     }
 }
 
@@ -359,7 +362,6 @@ pub fn uu_app() -> App<'static, 'static> {
              .takes_value(true)
              .require_equals(true)
              .min_values(0)
-             .possible_values(backup_control::BACKUP_CONTROL_VALUES)
              .value_name("CONTROL")
         )
         .arg(Arg::with_name(options::BACKUP_NO_ARG)
@@ -604,9 +606,17 @@ impl Options {
             || matches.is_present(options::ARCHIVE);
 
         let backup_mode = backup_control::determine_backup_mode(
-            matches.is_present(options::BACKUP_NO_ARG) || matches.is_present(options::BACKUP),
+            matches.is_present(options::BACKUP_NO_ARG),
+            matches.is_present(options::BACKUP),
             matches.value_of(options::BACKUP),
         );
+        let backup_mode = match backup_mode {
+            Err(err) => {
+                return Err(Error::Backup(err));
+            }
+            Ok(mode) => mode,
+        };
+
         let backup_suffix =
             backup_control::determine_backup_suffix(matches.value_of(options::SUFFIX));
 

diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs
@@ -308,15 +308,25 @@ fn behavior(matches: &ArgMatches) -> Result<Behavior, i32> {
         None
     };
 
+    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),
+    );
+    let backup_mode = match backup_mode {
+        Err(err) => {
+            show_usage_error!("{}", err);
+            return Err(1);
+        }
+        Ok(mode) => mode,
+    };
+
     let target_dir = matches.value_of(OPT_TARGET_DIRECTORY).map(|d| d.to_owned());
 
     Ok(Behavior {
         main_function,
         specified_mode,
-        backup_mode: backup_control::determine_backup_mode(
-            matches.is_present(OPT_BACKUP_NO_ARG) || matches.is_present(OPT_BACKUP),
-            matches.value_of(OPT_BACKUP),
-        ),
+        backup_mode,
         suffix: backup_control::determine_backup_suffix(matches.value_of(OPT_SUFFIX)),
         owner: matches.value_of(OPT_OWNER).unwrap_or("").to_string(),
         group: matches.value_of(OPT_GROUP).unwrap_or("").to_string(),

diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs
@@ -22,6 +22,7 @@ use std::os::unix::fs::symlink;
 #[cfg(windows)]
 use std::os::windows::fs::{symlink_dir, symlink_file};
 use std::path::{Path, PathBuf};
+use uucore::backup_control::{self, BackupMode};
 use uucore::fs::{canonicalize, CanonicalizeMode};
 
 pub struct Settings {
@@ -43,14 +44,6 @@ pub enum OverwriteMode {
     Force,
 }
 
-#[derive(Clone, Debug, Eq, PartialEq)]
-pub enum BackupMode {
-    NoBackup,
-    SimpleBackup,
-    NumberedBackup,
-    ExistingBackup,
-}
-
 fn get_usage() -> String {
     format!(
         "{0} [OPTION]... [-T] TARGET LINK_NAME   (1st form)
@@ -78,7 +71,7 @@ fn get_long_usage() -> String {
 static ABOUT: &str = "change file owner and group";
 
 mod options {
-    pub const B: &str = "b";
+    pub const BACKUP_NO_ARG: &str = "b";
     pub const BACKUP: &str = "backup";
     pub const FORCE: &str = "force";
     pub const INTERACTIVE: &str = "interactive";
@@ -99,7 +92,11 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
 
     let matches = uu_app()
         .usage(&usage[..])
-        .after_help(&long_usage[..])
+        .after_help(&*format!(
+            "{}\n{}",
+            long_usage,
+            backup_control::BACKUP_CONTROL_LONG_HELP
+        ))
         .get_matches_from(args);
 
     /* the list of files */
@@ -118,33 +115,25 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
         OverwriteMode::NoClobber
     };
 
-    let backup_mode = if matches.is_present(options::B) {
-        BackupMode::ExistingBackup
-    } else if matches.is_present(options::BACKUP) {
-        match matches.value_of(options::BACKUP) {
-            None => BackupMode::ExistingBackup,
-            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
-            },
+    let backup_mode = backup_control::determine_backup_mode(
+        matches.is_present(options::BACKUP_NO_ARG),
+        matches.is_present(options::BACKUP),
+        matches.value_of(options::BACKUP),
+    );
+    let backup_mode = match backup_mode {
+        Err(err) => {
+            show_usage_error!("{}", err);
+            return 1;
         }
-    } else {
-        BackupMode::NoBackup
+        Ok(mode) => mode,
     };
 
-    let backup_suffix = if matches.is_present(options::SUFFIX) {
-        matches.value_of(options::SUFFIX).unwrap()
-    } else {
-        "~"
-    };
+    let backup_suffix = backup_control::determine_backup_suffix(matches.value_of(options::SUFFIX));
 
     let settings = Settings {
         overwrite: overwrite_mode,
         backup: backup_mode,
-        suffix: backup_suffix.to_string(),
+        suffix: backup_suffix,
         symbolic: matches.is_present(options::SYMBOLIC),
         relative: matches.is_present(options::RELATIVE),
         target_dir: matches
@@ -162,22 +151,19 @@ pub fn uu_app() -> App<'static, 'static> {
     App::new(executable!())
         .version(crate_version!())
         .about(ABOUT)
-        .arg(Arg::with_name(options::B).short(options::B).help(
-            "make a backup of each file that would otherwise be overwritten or \
-             removed",
-        ))
         .arg(
             Arg::with_name(options::BACKUP)
                 .long(options::BACKUP)
-                .help(
-                    "make a backup of each file that would otherwise be overwritten \
-                     or removed",
-                )
+                .help("make a backup of each existing destination file")
                 .takes_value(true)
-                .possible_values(&[
-                    "simple", "never", "numbered", "t", "existing", "nil", "none", "off",
-                ])
-                .value_name("METHOD"),
+                .require_equals(true)
+                .min_values(0)
+                .value_name("CONTROL"),
+        )
+        .arg(
+            Arg::with_name(options::BACKUP_NO_ARG)
+                .short(options::BACKUP_NO_ARG)
+                .help("like --backup but does not accept an argument"),
         )
         // TODO: opts.arg(
         //    Arg::with_name(("d", "directory", "allow users with appropriate privileges to attempt \

diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs
@@ -86,9 +86,17 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
 
     let overwrite_mode = determine_overwrite_mode(&matches);
     let backup_mode = backup_control::determine_backup_mode(
-        matches.is_present(OPT_BACKUP_NO_ARG) || matches.is_present(OPT_BACKUP),
+        matches.is_present(OPT_BACKUP_NO_ARG),
+        matches.is_present(OPT_BACKUP),
         matches.value_of(OPT_BACKUP),
     );
+    let backup_mode = match backup_mode {
+        Err(err) => {
+            show_usage_error!("{}", err);
+            return 1;
+        }
+        Ok(mode) => mode,
+    };
 
     if overwrite_mode == OverwriteMode::NoClobber && backup_mode != BackupMode::NoBackup {
         show_usage_error!("options --backup and --no-clobber are mutually exclusive");
@@ -135,7 +143,6 @@ pub fn uu_app() -> App<'static, 'static> {
             .takes_value(true)
             .require_equals(true)
             .min_values(0)
-            .possible_values(backup_control::BACKUP_CONTROL_VALUES)
             .value_name("CONTROL")
     )
     .arg(

diff --git a/src/uucore/Cargo.toml b/src/uucore/Cargo.toml
@@ -30,6 +30,10 @@ time = { version="<= 0.1.43", optional=true }
 data-encoding = { version="~2.1", optional=true } ## data-encoding: require v2.1; but v2.2.0 breaks the build for MinSRV v1.31.0
 libc = { version="0.2.15, <= 0.2.85", optional=true } ## libc: initial utmp support added in v0.2.15; but v0.2.68 breaks the build for MinSRV v1.31.0
 
+[dev-dependencies]
+clap = "2.33.3"
+lazy_static = "1.3"
+
 [target.'cfg(target_os = "windows")'.dependencies]
 winapi = { version = "0.3", features = ["errhandlingapi", "fileapi", "handleapi", "winerror"] }