Merge pull request #2467 from Funky185540/backup_mode_determination

backup_control: Fix backup mode determination

Cargo.lock

@@ -1,5 +1,7 @@
 # This file is automatically @generated by Cargo.
 # It is not intended for manual editing.
+version = 3
+
 [[package]]
 name = "Inflector"
 version = "0.11.4"
@@ -2768,6 +2770,7 @@ dependencies = [
 name = "uucore"
 version = "0.0.9"
 dependencies = [
+ "clap",
  "data-encoding",
  "dns-lookup",
  "dunce",

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));
 

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(
+        backup_mode,
-            matches.is_present(OPT_BACKUP_NO_ARG) || matches.is_present(OPT_BACKUP),
-            matches.value_of(OPT_BACKUP),
-        ),
         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(),

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) {
+    let backup_mode = backup_control::determine_backup_mode(
-        BackupMode::ExistingBackup
+        matches.is_present(options::BACKUP_NO_ARG),
-    } else if matches.is_present(options::BACKUP) {
+        matches.is_present(options::BACKUP),
-        match matches.value_of(options::BACKUP) {
+        matches.value_of(options::BACKUP),
-            None => BackupMode::ExistingBackup,
+    );
-            Some(mode) => match mode {
+    let backup_mode = match backup_mode {
-                "simple" | "never" => BackupMode::SimpleBackup,
+        Err(err) => {
-                "numbered" | "t" => BackupMode::NumberedBackup,
+            show_usage_error!("{}", err);
-                "existing" | "nil" => BackupMode::ExistingBackup,
+            return 1;
-                "none" | "off" => BackupMode::NoBackup,
-                _ => panic!(), // cannot happen as it is managed by clap
-            },
         }
-    } else {
+        Ok(mode) => mode,
-        BackupMode::NoBackup
     };
 
-    let backup_suffix = if matches.is_present(options::SUFFIX) {
+    let backup_suffix = backup_control::determine_backup_suffix(matches.value_of(options::SUFFIX));
-        matches.value_of(options::SUFFIX).unwrap()
-    } else {
-        "~"
-    };
 
     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(
+                .help("make a backup of each existing destination file")
-                    "make a backup of each file that would otherwise be overwritten \
-                     or removed",
-                )
                 .takes_value(true)
-                .possible_values(&[
+                .require_equals(true)
-                    "simple", "never", "numbered", "t", "existing", "nil", "none", "off",
+                .min_values(0)
-                ])
+                .value_name("CONTROL"),
-                .value_name("METHOD"),
+        )
+        .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 \

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(

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"] }
 

src/uucore/src/lib/mods/backup_control.rs

@@ -7,19 +7,15 @@ pub static BACKUP_CONTROL_VALUES: &[&str] = &[
     "simple", "never", "numbered", "t", "existing", "nil", "none", "off",
 ];
 
-pub static BACKUP_CONTROL_LONG_HELP: &str = "The backup suffix is '~', unless set with --suffix or SIMPLE_BACKUP_SUFFIX. Here are the version control values:
+pub static BACKUP_CONTROL_LONG_HELP: &str =
+    "The backup suffix is '~', unless set with --suffix or SIMPLE_BACKUP_SUFFIX.
+The version control method may be selected via the --backup option or through
+the VERSION_CONTROL environment variable.  Here are the values:
 
-none, off
+  none, off       never make backups (even if --backup is given)
-    never make backups (even if --backup is given)
+  numbered, t     make numbered backups
-
+  existing, nil   numbered if numbered backups exist, simple otherwise
-numbered, t
+  simple, never   always make simple backups";
-    make numbered backups
-
-existing, nil
-    numbered if numbered backups exist, simple otherwise
-
-simple, never
-    always make simple backups";
 
 #[derive(Debug, Clone, Copy, Eq, PartialEq)]
 pub enum BackupMode {
@@ -37,35 +33,177 @@ pub fn determine_backup_suffix(supplied_suffix: Option<&str>) -> String {
     }
 }
 
-/// # TODO
+/// Determine the "mode" for the backup operation to perform, if any.
 ///
-/// This function currently deviates slightly from how the [manual][1] describes
+/// Parses the backup options according to the [GNU manual][1], and converts
-/// that it should work. In particular, the current implementation:
+/// them to an instance of `BackupMode` for further processing.
 ///
-/// 1. Doesn't strictly respect the order in which to determine the backup type,
+/// For an explanation of what the arguments mean, refer to the examples below.
-///    which is (in order of precedence)
-///     1. Take a valid value to the '--backup' option
-///     2. Take the value of the `VERSION_CONTROL` env var
-///     3. default to 'existing'
-/// 2. Doesn't accept abbreviations to the 'backup_option' parameter
 ///
 /// [1]: https://www.gnu.org/software/coreutils/manual/html_node/Backup-options.html
-pub fn determine_backup_mode(backup_opt_exists: bool, backup_opt: Option<&str>) -> BackupMode {
+///
-    if backup_opt_exists {
+///
-        match backup_opt.map(String::from) {
+/// # Errors
-            // default is existing, see:
+///
-            // https://www.gnu.org/software/coreutils/manual/html_node/Backup-options.html
+/// If an argument supplied directly to the long `backup` option, or read in
-            None => BackupMode::ExistingBackup,
+/// through the `VERSION CONTROL` env var is ambiguous (i.e. may resolve to
-            Some(mode) => match &mode[..] {
+/// multiple backup modes) or invalid, an error is returned. The error contains
-                "simple" | "never" => BackupMode::SimpleBackup,
+/// the formatted error string which may then be passed to the
-                "numbered" | "t" => BackupMode::NumberedBackup,
+/// [`show_usage_error`] macro.
-                "existing" | "nil" => BackupMode::ExistingBackup,
+///
-                "none" | "off" => BackupMode::NoBackup,
+///
-                _ => panic!(), // cannot happen as it is managed by clap
+/// # Examples
-            },
+///
+/// Here's how one would integrate the backup mode determination into an
+/// application.
+///
+/// ```
+/// #[macro_use]
+/// extern crate uucore;
+/// use uucore::backup_control::{self, BackupMode};
+/// use clap::{App, Arg};
+///
+/// fn main() {
+///     let OPT_BACKUP: &str = "backup";
+///     let OPT_BACKUP_NO_ARG: &str = "b";
+///     let matches = App::new("app")
+///         .arg(Arg::with_name(OPT_BACKUP_NO_ARG)
+///             .short(OPT_BACKUP_NO_ARG))
+///         .arg(Arg::with_name(OPT_BACKUP)
+///             .long(OPT_BACKUP)
+///             .takes_value(true)
+///             .require_equals(true)
+///             .min_values(0))
+///         .get_matches_from(vec![
+///             "app", "-b", "--backup=t"
+///         ]);
+///    
+///     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;
+///         },
+///         Ok(mode) => mode,
+///     };
+/// }
+/// ```
+///
+/// This example shows an ambiguous input, as 'n' may resolve to 4 different
+/// backup modes.
+///
+///
+/// ```
+/// #[macro_use]
+/// extern crate uucore;
+/// use uucore::backup_control::{self, BackupMode};
+/// use clap::{crate_version, App, Arg, ArgMatches};
+///
+/// fn main() {
+///     let OPT_BACKUP: &str = "backup";
+///     let OPT_BACKUP_NO_ARG: &str = "b";
+///     let matches = App::new("app")
+///         .arg(Arg::with_name(OPT_BACKUP_NO_ARG)
+///             .short(OPT_BACKUP_NO_ARG))
+///         .arg(Arg::with_name(OPT_BACKUP)
+///             .long(OPT_BACKUP)
+///             .takes_value(true)
+///             .require_equals(true)
+///             .min_values(0))
+///         .get_matches_from(vec![
+///             "app", "-b", "--backup=n"
+///         ]);
+///    
+///     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;
+///         },
+///         Ok(mode) => mode,
+///     };
+/// }
+/// ```
+pub fn determine_backup_mode(
+    short_opt_present: bool,
+    long_opt_present: bool,
+    long_opt_value: Option<&str>,
+) -> Result<BackupMode, String> {
+    if long_opt_present {
+        // Use method to determine the type of backups to make. When this option
+        // is used but method is not specified, then the value of the
+        // VERSION_CONTROL environment variable is used. And if VERSION_CONTROL
+        // is not set, the default backup type is ‘existing’.
+        if let Some(method) = long_opt_value {
+            // Second argument is for the error string that is returned.
+            match_method(method, "backup type")
+        } else if let Ok(method) = env::var("VERSION_CONTROL") {
+            // Second argument is for the error string that is returned.
+            match_method(&method, "$VERSION_CONTROL")
+        } else {
+            Ok(BackupMode::ExistingBackup)
+        }
+    } else if short_opt_present {
+        // the short form of this option, -b does not accept any argument.
+        // Using -b is equivalent to using --backup=existing.
+        Ok(BackupMode::ExistingBackup)
+    } else {
+        // No option was present at all
+        Ok(BackupMode::NoBackup)
+    }
+}
+
+/// Match a backup option string to a `BackupMode`.
+///
+/// The GNU manual specifies that abbreviations to options are valid as long as
+/// they aren't ambiguous. This function matches the given `method` argument
+/// against all valid backup options (via `starts_with`), and returns a valid
+/// [`BackupMode`] if exactly one backup option matches the `method` given.
+///
+/// `origin` is required in order to format the generated error message
+/// properly, when an error occurs.
+///
+///
+/// # Errors
+///
+/// If `method` is ambiguous (i.e. may resolve to multiple backup modes) or
+/// invalid, an error is returned. The error contains the formatted error string
+/// which may then be passed to the [`show_usage_error`] macro.
+fn match_method(method: &str, origin: &str) -> Result<BackupMode, String> {
+    let matches: Vec<&&str> = BACKUP_CONTROL_VALUES
+        .iter()
+        .filter(|val| val.starts_with(method))
+        .collect();
+    if matches.len() == 1 {
+        match *matches[0] {
+            "simple" | "never" => Ok(BackupMode::SimpleBackup),
+            "numbered" | "t" => Ok(BackupMode::NumberedBackup),
+            "existing" | "nil" => Ok(BackupMode::ExistingBackup),
+            "none" | "off" => Ok(BackupMode::NoBackup),
+            _ => unreachable!(), // cannot happen as we must have exactly one match
+                                 // from the list above.
         }
     } else {
-        BackupMode::NoBackup
+        let error_type = if matches.is_empty() {
+            "invalid"
+        } else {
+            "ambiguous"
+        };
+        Err(format!(
+            "{0} argument ‘{1}’ for ‘{2}’
+Valid arguments are:
+  - ‘none’, ‘off’
+  - ‘simple’, ‘never’
+  - ‘existing’, ‘nil’
+  - ‘numbered’, ‘t’",
+            error_type, method, origin
+        ))
     }
 }
 
@@ -82,13 +220,13 @@ pub fn get_backup_path(
     }
 }
 
-pub fn simple_backup_path(path: &Path, suffix: &str) -> PathBuf {
+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)
 }
 
-pub fn numbered_backup_path(path: &Path) -> PathBuf {
+fn numbered_backup_path(path: &Path) -> PathBuf {
     for i in 1_u64.. {
         let path_str = &format!("{}.~{}~", path.to_string_lossy(), i);
         let path = Path::new(path_str);
@@ -99,7 +237,7 @@ pub fn numbered_backup_path(path: &Path) -> PathBuf {
     panic!("cannot create backup")
 }
 
-pub fn existing_backup_path(path: &Path, suffix: &str) -> PathBuf {
+fn existing_backup_path(path: &Path, suffix: &str) -> PathBuf {
     let test_path_str = &format!("{}.~1~", path.to_string_lossy());
     let test_path = Path::new(test_path_str);
     if test_path.exists() {
@@ -108,3 +246,210 @@ pub fn existing_backup_path(path: &Path, suffix: &str) -> PathBuf {
         simple_backup_path(path, suffix)
     }
 }
+
+//
+// Tests for this module
+//
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::env;
+    // Required to instantiate mutex in shared context
+    use lazy_static::lazy_static;
+    use std::sync::Mutex;
+
+    // The mutex is required here as by default all tests are run as separate
+    // threads under the same parent process. As environment variables are
+    // specific to processes (and thus shared among threads), data races *will*
+    // occur if no precautions are taken. Thus we have all tests that rely on
+    // environment variables lock this empty mutex to ensure they don't access
+    // it concurrently.
+    lazy_static! {
+        static ref TEST_MUTEX: Mutex<()> = Mutex::new(());
+    }
+
+    // Environment variable for "VERSION_CONTROL"
+    static ENV_VERSION_CONTROL: &str = "VERSION_CONTROL";
+
+    // Defaults to --backup=existing
+    #[test]
+    fn test_backup_mode_short_only() {
+        let short_opt_present = true;
+        let long_opt_present = false;
+        let long_opt_value = None;
+        let _dummy = TEST_MUTEX.lock().unwrap();
+
+        let result =
+            determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap();
+
+        assert_eq!(result, BackupMode::ExistingBackup);
+    }
+
+    // --backup takes precedence over -b
+    #[test]
+    fn test_backup_mode_long_preferred_over_short() {
+        let short_opt_present = true;
+        let long_opt_present = true;
+        let long_opt_value = Some("none");
+        let _dummy = TEST_MUTEX.lock().unwrap();
+
+        let result =
+            determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap();
+
+        assert_eq!(result, BackupMode::NoBackup);
+    }
+
+    // --backup can be passed without an argument
+    #[test]
+    fn test_backup_mode_long_without_args_no_env() {
+        let short_opt_present = false;
+        let long_opt_present = true;
+        let long_opt_value = None;
+        let _dummy = TEST_MUTEX.lock().unwrap();
+
+        let result =
+            determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap();
+
+        assert_eq!(result, BackupMode::ExistingBackup);
+    }
+
+    // --backup can be passed with an argument only
+    #[test]
+    fn test_backup_mode_long_with_args() {
+        let short_opt_present = false;
+        let long_opt_present = true;
+        let long_opt_value = Some("simple");
+        let _dummy = TEST_MUTEX.lock().unwrap();
+
+        let result =
+            determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap();
+
+        assert_eq!(result, BackupMode::SimpleBackup);
+    }
+
+    // --backup errors on invalid argument
+    #[test]
+    fn test_backup_mode_long_with_args_invalid() {
+        let short_opt_present = false;
+        let long_opt_present = true;
+        let long_opt_value = Some("foobar");
+        let _dummy = TEST_MUTEX.lock().unwrap();
+
+        let result = determine_backup_mode(short_opt_present, long_opt_present, long_opt_value);
+
+        assert!(result.is_err());
+        let text = result.unwrap_err();
+        assert!(text.contains("invalid argument ‘foobar’ for ‘backup type’"));
+    }
+
+    // --backup errors on ambiguous argument
+    #[test]
+    fn test_backup_mode_long_with_args_ambiguous() {
+        let short_opt_present = false;
+        let long_opt_present = true;
+        let long_opt_value = Some("n");
+        let _dummy = TEST_MUTEX.lock().unwrap();
+
+        let result = determine_backup_mode(short_opt_present, long_opt_present, long_opt_value);
+
+        assert!(result.is_err());
+        let text = result.unwrap_err();
+        assert!(text.contains("ambiguous argument ‘n’ for ‘backup type’"));
+    }
+
+    // --backup accepts shortened arguments (si for simple)
+    #[test]
+    fn test_backup_mode_long_with_arg_shortened() {
+        let short_opt_present = false;
+        let long_opt_present = true;
+        let long_opt_value = Some("si");
+        let _dummy = TEST_MUTEX.lock().unwrap();
+
+        let result =
+            determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap();
+
+        assert_eq!(result, BackupMode::SimpleBackup);
+    }
+
+    // -b ignores the "VERSION_CONTROL" environment variable
+    #[test]
+    fn test_backup_mode_short_only_ignore_env() {
+        let short_opt_present = true;
+        let long_opt_present = false;
+        let long_opt_value = None;
+        let _dummy = TEST_MUTEX.lock().unwrap();
+        env::set_var(ENV_VERSION_CONTROL, "none");
+
+        let result =
+            determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap();
+
+        assert_eq!(result, BackupMode::ExistingBackup);
+        env::remove_var(ENV_VERSION_CONTROL);
+    }
+
+    // --backup can be passed without an argument, but reads env var if existent
+    #[test]
+    fn test_backup_mode_long_without_args_with_env() {
+        let short_opt_present = false;
+        let long_opt_present = true;
+        let long_opt_value = None;
+        let _dummy = TEST_MUTEX.lock().unwrap();
+        env::set_var(ENV_VERSION_CONTROL, "none");
+
+        let result =
+            determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap();
+
+        assert_eq!(result, BackupMode::NoBackup);
+        env::remove_var(ENV_VERSION_CONTROL);
+    }
+
+    // --backup errors on invalid VERSION_CONTROL env var
+    #[test]
+    fn test_backup_mode_long_with_env_var_invalid() {
+        let short_opt_present = false;
+        let long_opt_present = true;
+        let long_opt_value = None;
+        let _dummy = TEST_MUTEX.lock().unwrap();
+        env::set_var(ENV_VERSION_CONTROL, "foobar");
+
+        let result = determine_backup_mode(short_opt_present, long_opt_present, long_opt_value);
+
+        assert!(result.is_err());
+        let text = result.unwrap_err();
+        assert!(text.contains("invalid argument ‘foobar’ for ‘$VERSION_CONTROL’"));
+        env::remove_var(ENV_VERSION_CONTROL);
+    }
+
+    // --backup errors on ambiguous VERSION_CONTROL env var
+    #[test]
+    fn test_backup_mode_long_with_env_var_ambiguous() {
+        let short_opt_present = false;
+        let long_opt_present = true;
+        let long_opt_value = None;
+        let _dummy = TEST_MUTEX.lock().unwrap();
+        env::set_var(ENV_VERSION_CONTROL, "n");
+
+        let result = determine_backup_mode(short_opt_present, long_opt_present, long_opt_value);
+
+        assert!(result.is_err());
+        let text = result.unwrap_err();
+        assert!(text.contains("ambiguous argument ‘n’ for ‘$VERSION_CONTROL’"));
+        env::remove_var(ENV_VERSION_CONTROL);
+    }
+
+    // --backup accepts shortened env vars (si for simple)
+    #[test]
+    fn test_backup_mode_long_with_env_var_shortened() {
+        let short_opt_present = false;
+        let long_opt_present = true;
+        let long_opt_value = None;
+        let _dummy = TEST_MUTEX.lock().unwrap();
+        env::set_var(ENV_VERSION_CONTROL, "si");
+
+        let result =
+            determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap();
+
+        assert_eq!(result, BackupMode::SimpleBackup);
+        env::remove_var(ENV_VERSION_CONTROL);
+    }
+}