Merge pull request #2457 from Funky185540/install-implement-backup

install: implement --backup, -b and -S

src/uu/install/src/install.rs

@@ -15,6 +15,7 @@ extern crate uucore;
 use clap::{crate_version, App, Arg, ArgMatches};
 use file_diff::diff;
 use filetime::{set_file_times, FileTime};
+use uucore::backup_control::{self, BackupMode};
 use uucore::entries::{grp2gid, usr2uid};
 use uucore::perms::{wrap_chgrp, wrap_chown, Verbosity};
 
@@ -33,6 +34,7 @@ const DEFAULT_STRIP_PROGRAM: &str = "strip";
 pub struct Behavior {
     main_function: MainFunction,
     specified_mode: Option<u32>,
+    backup_mode: BackupMode,
     suffix: String,
     owner: String,
     group: String,
@@ -68,7 +70,7 @@ static ABOUT: &str = "Copy SOURCE to DEST or multiple SOURCE(s) to the existing
 
 static OPT_COMPARE: &str = "compare";
 static OPT_BACKUP: &str = "backup";
-static OPT_BACKUP_2: &str = "backup2";
+static OPT_BACKUP_NO_ARG: &str = "backup2";
 static OPT_DIRECTORY: &str = "directory";
 static OPT_IGNORED: &str = "ignored";
 static OPT_CREATE_LEADING: &str = "create-leading";
@@ -130,14 +132,17 @@ pub fn uu_app() -> App<'static, 'static> {
         .arg(
                 Arg::with_name(OPT_BACKUP)
                 .long(OPT_BACKUP)
-                .help("(unimplemented) make a backup of each existing destination file")
+                .help("make a backup of each existing destination file")
+                .takes_value(true)
+                .require_equals(true)
+                .min_values(0)
                 .value_name("CONTROL")
         )
         .arg(
             // TODO implement flag
-            Arg::with_name(OPT_BACKUP_2)
+            Arg::with_name(OPT_BACKUP_NO_ARG)
             .short("b")
-            .help("(unimplemented) like --backup but does not accept an argument")
+            .help("like --backup but does not accept an argument")
         )
         .arg(
             Arg::with_name(OPT_IGNORED)
@@ -210,7 +215,7 @@ pub fn uu_app() -> App<'static, 'static> {
             Arg::with_name(OPT_SUFFIX)
                 .short("S")
                 .long(OPT_SUFFIX)
-                .help("(unimplemented) override the usual backup suffix")
+                .help("override the usual backup suffix")
                 .value_name("SUFFIX")
                 .takes_value(true)
                 .min_values(1)
@@ -265,13 +270,7 @@ pub fn uu_app() -> App<'static, 'static> {
 ///
 ///
 fn check_unimplemented<'a>(matches: &ArgMatches) -> Result<(), &'a str> {
-    if matches.is_present(OPT_BACKUP) {
+    if matches.is_present(OPT_NO_TARGET_DIRECTORY) {
-        Err("--backup")
-    } else if matches.is_present(OPT_BACKUP_2) {
-        Err("-b")
-    } else if matches.is_present(OPT_SUFFIX) {
-        Err("--suffix, -S")
-    } else if matches.is_present(OPT_NO_TARGET_DIRECTORY) {
         Err("--no-target-directory, -T")
     } else if matches.is_present(OPT_PRESERVE_CONTEXT) {
         Err("--preserve-context, -P")
@@ -309,18 +308,16 @@ fn behavior(matches: &ArgMatches) -> Result<Behavior, i32> {
         None
     };
 
-    let backup_suffix = if matches.is_present(OPT_SUFFIX) {
-        matches.value_of(OPT_SUFFIX).ok_or(1)?
-    } else {
-        "~"
-    };
-
     let target_dir = matches.value_of(OPT_TARGET_DIRECTORY).map(|d| d.to_owned());
 
     Ok(Behavior {
         main_function,
         specified_mode,
-        suffix: backup_suffix.to_string(),
+        backup_mode: backup_control::determine_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(),
         verbose: matches.is_present(OPT_VERBOSE),
@@ -517,6 +514,28 @@ fn copy(from: &Path, to: &Path, b: &Behavior) -> Result<(), ()> {
     if b.compare && !need_copy(from, to, b) {
         return Ok(());
     }
+    // Declare the path here as we may need it for the verbose output below.
+    let mut backup_path = None;
+
+    // Perform backup, if any, before overwriting 'to'
+    //
+    // The codes actually making use of the backup process don't seem to agree
+    // on how best to approach the issue. (mv and ln, for example)
+    if to.exists() {
+        backup_path = backup_control::get_backup_path(b.backup_mode, to, &b.suffix);
+        if let Some(ref backup_path) = backup_path {
+            // TODO!!
+            if let Err(err) = fs::rename(to, backup_path) {
+                show_error!(
+                    "install: cannot backup file '{}' to '{}': {}",
+                    to.display(),
+                    backup_path.display(),
+                    err
+                );
+                return Err(());
+            }
+        }
+    }
 
     if from.to_string_lossy() == "/dev/null" {
         /* workaround a limitation of fs::copy
@@ -624,7 +643,11 @@ fn copy(from: &Path, to: &Path, b: &Behavior) -> Result<(), ()> {
     }
 
     if b.verbose {
-        show_error!("'{}' -> '{}'", from.display(), to.display());
+        print!("'{}' -> '{}'", from.display(), to.display());
+        match backup_path {
+            Some(path) => println!(" (backup: '{}')", path.display()),
+            None => println!(),
+        }
     }
 
     Ok(())

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

@@ -37,6 +37,19 @@ pub fn determine_backup_suffix(supplied_suffix: Option<&str>) -> String {
     }
 }
 
+/// # TODO
+///
+/// This function currently deviates slightly from how the [manual][1] describes
+/// that it should work. In particular, the current implementation:
+///
+/// 1. Doesn't strictly respect the order in which to determine the backup type,
+///    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) {

tests/by-util/test_install.rs

@@ -696,3 +696,388 @@ fn test_install_dir() {
     assert!(at.file_exists(&format!("{}/{}", dir, file1)));
     assert!(at.file_exists(&format!("{}/{}", dir, file2)));
 }
+//
+// test backup functionality
+#[test]
+fn test_install_backup_short_no_args_files() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file_a = "test_install_simple_backup_file_a";
+    let file_b = "test_install_simple_backup_file_b";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    scene
+        .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_install_backup_short_no_args_file_to_dir() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file = "test_install_simple_backup_file_a";
+    let dest_dir = "test_install_dest/";
+    let expect = format!("{}{}", dest_dir, file);
+
+    at.touch(file);
+    at.mkdir(dest_dir);
+    at.touch(&expect);
+    scene
+        .ucmd()
+        .arg("-b")
+        .arg(file)
+        .arg(dest_dir)
+        .succeeds()
+        .no_stderr();
+
+    assert!(at.file_exists(file));
+    assert!(at.file_exists(&expect));
+    assert!(at.file_exists(&format!("{}~", expect)));
+}
+
+// Long --backup option is tested separately as it requires a slightly different
+// handling than '-b' does.
+#[test]
+fn test_install_backup_long_no_args_files() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file_a = "test_install_simple_backup_file_a";
+    let file_b = "test_install_simple_backup_file_b";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    scene
+        .ucmd()
+        .arg("--backup")
+        .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_install_backup_long_no_args_file_to_dir() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file = "test_install_simple_backup_file_a";
+    let dest_dir = "test_install_dest/";
+    let expect = format!("{}{}", dest_dir, file);
+
+    at.touch(file);
+    at.mkdir(dest_dir);
+    at.touch(&expect);
+    scene
+        .ucmd()
+        .arg("--backup")
+        .arg(file)
+        .arg(dest_dir)
+        .succeeds()
+        .no_stderr();
+
+    assert!(at.file_exists(file));
+    assert!(at.file_exists(&expect));
+    assert!(at.file_exists(&format!("{}~", expect)));
+}
+
+#[test]
+fn test_install_backup_short_custom_suffix() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file_a = "test_install_backup_custom_suffix_file_a";
+    let file_b = "test_install_backup_custom_suffix_file_b";
+    let suffix = "super-suffix-of-the-century";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    scene
+        .ucmd()
+        .arg("-b")
+        .arg(format!("--suffix={}", suffix))
+        .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, suffix)));
+}
+
+#[test]
+fn test_install_backup_custom_suffix_via_env() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file_a = "test_install_backup_custom_suffix_file_a";
+    let file_b = "test_install_backup_custom_suffix_file_b";
+    let suffix = "super-suffix-of-the-century";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    scene
+        .ucmd()
+        .arg("-b")
+        .env("SIMPLE_BACKUP_SUFFIX", suffix)
+        .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, suffix)));
+}
+
+#[test]
+fn test_install_backup_numbered_with_t() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file_a = "test_install_backup_numbering_file_a";
+    let file_b = "test_install_backup_numbering_file_b";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    scene
+        .ucmd()
+        .arg("--backup=t")
+        .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_install_backup_numbered_with_numbered() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file_a = "test_install_backup_numbering_file_a";
+    let file_b = "test_install_backup_numbering_file_b";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    scene
+        .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_install_backup_existing() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file_a = "test_install_backup_numbering_file_a";
+    let file_b = "test_install_backup_numbering_file_b";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    scene
+        .ucmd()
+        .arg("--backup=existing")
+        .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_install_backup_nil() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file_a = "test_install_backup_numbering_file_a";
+    let file_b = "test_install_backup_numbering_file_b";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    scene
+        .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_install_backup_numbered_if_existing_backup_existing() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file_a = "test_install_backup_numbering_file_a";
+    let file_b = "test_install_backup_numbering_file_b";
+    let file_b_backup = "test_install_backup_numbering_file_b.~1~";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    at.touch(file_b_backup);
+    scene
+        .ucmd()
+        .arg("--backup=existing")
+        .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(file_b_backup));
+    assert!(at.file_exists(&*format!("{}.~2~", file_b)));
+}
+
+#[test]
+fn test_install_backup_numbered_if_existing_backup_nil() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file_a = "test_install_backup_numbering_file_a";
+    let file_b = "test_install_backup_numbering_file_b";
+    let file_b_backup = "test_install_backup_numbering_file_b.~1~";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    at.touch(file_b_backup);
+    scene
+        .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(file_b_backup));
+    assert!(at.file_exists(&*format!("{}.~2~", file_b)));
+}
+
+#[test]
+fn test_install_backup_simple() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file_a = "test_install_backup_numbering_file_a";
+    let file_b = "test_install_backup_numbering_file_b";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    scene
+        .ucmd()
+        .arg("--backup=simple")
+        .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_install_backup_never() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file_a = "test_install_backup_numbering_file_a";
+    let file_b = "test_install_backup_numbering_file_b";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    scene
+        .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_install_backup_none() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file_a = "test_install_backup_numbering_file_a";
+    let file_b = "test_install_backup_numbering_file_b";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    scene
+        .ucmd()
+        .arg("--backup=none")
+        .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_install_backup_off() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file_a = "test_install_backup_numbering_file_a";
+    let file_b = "test_install_backup_numbering_file_b";
+
+    at.touch(file_a);
+    at.touch(file_b);
+    scene
+        .ucmd()
+        .arg("--backup=off")
+        .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)));
+}