mv: cleanup the code

README.md

@@ -194,7 +194,7 @@ To do
 * [x] mkfifo
 * [x] mknod
 * [x] mktemp
-* [ ] mv (almost done, one more option)
+* [x] mv
 * [x] nice
 * [x] nl
 * [x] nohup

src/mv/Cargo.toml

@@ -9,11 +9,10 @@ path = "mv.rs"
 
 [dependencies]
 getopts = "*"
-libc = "*"
-uucore = { path="../uucore" }
 
-[dev-dependencies]
+[dependencies.uucore]
-time = "*"
+path = "../uucore"
+default-features = false
 
 [[bin]]
 name = "mv"

src/mv/mv.rs

@@ -1,22 +1,21 @@
 #![crate_name = "uu_mv"]
 
-/*
+// This file is part of the uutils coreutils package.
- * This file is part of the uutils coreutils package.
+//
- *
+// (c) Orvar Segerström <orvarsegerstrom@gmail.com>
- * (c) Orvar Segerström <orvarsegerstrom@gmail.com>
+// (c) Sokovikov Evgeniy  <skv-headless@yandex.ru>
- * (c) Sokovikov Evgeniy  <skv-headless@yandex.ru>
+//
- *
+// For the full copyright and license information, please view the LICENSE file
- * For the full copyright and license information, please view the LICENSE file
+// that was distributed with this source code.
- * that was distributed with this source code.
+//
- */
 
 extern crate getopts;
-extern crate libc;
 
 #[macro_use]
 extern crate uucore;
 
 use std::fs;
+use std::env;
 use std::io::{BufRead, BufReader, Result, stdin, Write};
 use std::os::unix::fs::MetadataExt;
 use std::path::{Path, PathBuf};
@@ -41,7 +40,7 @@ pub enum OverwriteMode {
     Force,
 }
 
-#[derive(Clone, Eq, PartialEq)]
+#[derive(Clone, Copy, Eq, PartialEq)]
 pub enum BackupMode {
     NoBackup,
     SimpleBackup,
@@ -52,17 +51,27 @@ pub enum BackupMode {
 pub fn uumain(args: Vec<String>) -> i32 {
     let mut opts = getopts::Options::new();
 
-    opts.optflagopt("",  "backup", "make a backup of each existing destination file", "CONTROL");
+    opts.optflagopt("",
+                    "backup",
+                    "make a backup of each existing destination file",
+                    "CONTROL");
     opts.optflag("b", "", "like --backup but does not accept an argument");
     opts.optflag("f", "force", "do not prompt before overwriting");
     opts.optflag("i", "interactive", "prompt before override");
     opts.optflag("n", "no-clobber", "do not overwrite an existing file");
-    opts.optflag("",  "strip-trailing-slashes", "remove any trailing slashes from each SOURCE\n \
+    opts.optflag("",
+                 "strip-trailing-slashes",
+                 "remove any trailing slashes from each SOURCE\n \
                                                  argument");
     opts.optopt("S", "suffix", "override the usual backup suffix", "SUFFIX");
-    opts.optopt("t", "target-directory", "move all SOURCE arguments into DIRECTORY", "DIRECTORY");
+    opts.optopt("t",
+                "target-directory",
+                "move all SOURCE arguments into DIRECTORY",
+                "DIRECTORY");
     opts.optflag("T", "no-target-directory", "treat DEST as a normal file");
-    opts.optflag("u", "update", "move only when the SOURCE file is newer\n \
+    opts.optflag("u",
+                 "update",
+                 "move only when the SOURCE file is newer\n \
                                 than the destination file or when the\n \
                                 destination file is missing");
     opts.optflag("v", "verbose", "explain what is being done");
@@ -78,11 +87,11 @@ pub fn uumain(args: Vec<String>) -> i32 {
     };
     let usage = opts.usage("Move SOURCE to DEST, or multiple SOURCE(s) to DIRECTORY.");
 
-    /* This does not exactly match the GNU implementation:
+    // This does not exactly match the GNU implementation:
-     * The GNU mv defaults to Force, but if more than one of the
+    // The GNU mv defaults to Force, but if more than one of the
-     * overwrite options are supplied, only the last takes effect.
+    // overwrite options are supplied, only the last takes effect.
-     * To default to no-clobber in that situation seems safer:
+    // To default to no-clobber in that situation seems safer:
-     */
+    //
     let overwrite_mode = if matches.opt_present("no-clobber") {
         OverwriteMode::NoClobber
     } else if matches.opt_present("interactive") {
@@ -96,15 +105,19 @@ pub fn uumain(args: Vec<String>) -> i32 {
     } else if matches.opt_present("backup") {
         match matches.opt_str("backup") {
             None => BackupMode::SimpleBackup,
-            Some(mode) => match &mode[..] {
+            Some(mode) => {
-                "simple" | "never" => BackupMode::SimpleBackup,
+                match &mode[..] {
-                "numbered" | "t"   => BackupMode::NumberedBackup,
+                    "simple" | "never" => BackupMode::SimpleBackup,
-                "existing" | "nil" => BackupMode::ExistingBackup,
+                    "numbered" | "t" => BackupMode::NumberedBackup,
-                "none" | "off"     => BackupMode::NoBackup,
+                    "existing" | "nil" => BackupMode::ExistingBackup,
-                x => {
+                    "none" | "off" => BackupMode::NoBackup,
-                    show_error!("invalid argument ‘{}’ for ‘backup type’\n\
+                    x => {
-                                Try '{} --help' for more information.", x, NAME);
+                        show_error!("invalid argument ‘{}’ for ‘backup type’\n\
-                    return 1;
+                                Try '{} --help' for more information.",
+                                    x,
+                                    NAME);
+                        return 1;
+                    }
                 }
             }
         }
@@ -114,7 +127,8 @@ pub fn uumain(args: Vec<String>) -> i32 {
 
     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.", NAME);
+                    Try '{} --help' for more information.",
+                    NAME);
         return 1;
     }
 
@@ -123,12 +137,17 @@ pub fn uumain(args: Vec<String>) -> i32 {
             Some(x) => x,
             None => {
                 show_error!("option '--suffix' requires an argument\n\
-                            Try '{} --help' for more information.", NAME);
+                            Try '{} --help' for more information.",
+                            NAME);
                 return 1;
             }
         }
     } else {
-        "~".to_owned()
+        if let (Ok(s), BackupMode::SimpleBackup) = (env::var("SIMPLE_BACKUP_SUFFIX"), backup_mode) {
+            s
+        } else {
+            "~".to_owned()
+        }
     };
 
     if matches.opt_present("T") && matches.opt_present("t") {
@@ -147,14 +166,11 @@ pub fn uumain(args: Vec<String>) -> i32 {
     };
 
     let paths: Vec<PathBuf> = {
-        fn string_to_path<'a>(s: &'a String) -> &'a Path {
-            Path::new(s)
-        };
         fn strip_slashes<'a>(p: &'a Path) -> &'a Path {
             p.components().as_path()
         }
         let to_owned = |p: &Path| p.to_owned();
-        let arguments = matches.free.iter().map(string_to_path);
+        let arguments = matches.free.iter().map(Path::new);
         if matches.opt_present("strip-trailing-slashes") {
             arguments.map(strip_slashes).map(to_owned).collect()
         } else {
@@ -177,25 +193,29 @@ fn help(usage: &str) {
     println!("{0} {1}\n\n\
     Usage: {0} SOURCE DEST\n   \
        or: {0} SOURCE... DIRECTORY\n\n\
-    {2}", NAME, VERSION, usage);
+    {2}",
+             NAME,
+             VERSION,
+             usage);
 }
 
 fn exec(files: &[PathBuf], b: Behaviour) -> i32 {
-    match b.target_dir {
+    if let Some(ref name) = b.target_dir {
-        Some(ref name) => return move_files_into_dir(files, &PathBuf::from(name), &b),
+        return move_files_into_dir(files, &PathBuf::from(name), &b);
-        None => {}
     }
     match files.len() {
         0 | 1 => {
             show_error!("missing file operand\n\
-                        Try '{} --help' for more information.", NAME);
+                        Try '{} --help' for more information.",
+                        NAME);
             return 1;
-        },
+        }
         2 => {
             let source = &files[0];
             let target = &files[1];
             if !source.exists() {
-                show_error!("cannot stat ‘{}’: No such file or directory", source.display());
+                show_error!("cannot stat ‘{}’: No such file or directory",
+                            source.display());
                 return 1;
             }
 
@@ -203,7 +223,7 @@ fn exec(files: &[PathBuf], b: Behaviour) -> i32 {
                 if b.no_target_dir {
                     if !source.is_dir() {
                         show_error!("cannot overwrite directory ‘{}’ with non-directory",
-                            target.display());
+                                    target.display());
                         return 1;
                     }
 
@@ -211,9 +231,9 @@ fn exec(files: &[PathBuf], b: Behaviour) -> i32 {
                         Err(e) => {
                             show_error!("{}", e);
                             1
-                        },
+                        }
-                        _ => 0
+                        _ => 0,
-                    }
+                    };
                 }
 
                 return move_files_into_dir(&[source.clone()], target, &b);
@@ -227,11 +247,13 @@ fn exec(files: &[PathBuf], b: Behaviour) -> i32 {
         _ => {
             if b.no_target_dir {
                 show_error!("mv: extra operand ‘{}’\n\
-                            Try '{} --help' for more information.", files[2].display(), NAME);
+                            Try '{} --help' for more information.",
+                            files[2].display(),
+                            NAME);
                 return 1;
             }
             let target_dir = files.last().unwrap();
-            move_files_into_dir(&files[0..files.len()-1], target_dir, &b);
+            move_files_into_dir(&files[..files.len() - 1], target_dir, &b);
         }
     }
     0
@@ -258,11 +280,17 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &PathBuf, b: &Behaviour) -
 
         if let Err(e) = rename(sourcepath, &targetpath, b) {
             show_error!("mv: cannot move ‘{}’ to ‘{}’: {}",
-                        sourcepath.display(), targetpath.display(), e);
+                        sourcepath.display(),
+                        targetpath.display(),
+                        e);
             all_successful = false;
         }
-    };
+    }
-    if all_successful { 0 } else { 1 }
+    if all_successful {
+        0
+    } else {
+        1
+    }
 }
 
 fn rename(from: &PathBuf, to: &PathBuf, b: &Behaviour) -> Result<()> {
@@ -276,7 +304,7 @@ fn rename(from: &PathBuf, to: &PathBuf, b: &Behaviour) -> Result<()> {
                 if !read_yes() {
                     return Ok(());
                 }
-            },
+            }
             OverwriteMode::Force => {}
         };
 
@@ -284,7 +312,7 @@ fn rename(from: &PathBuf, to: &PathBuf, b: &Behaviour) -> Result<()> {
             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))
+            BackupMode::ExistingBackup => Some(existing_backup_path(to, &b.suffix)),
         };
         if let Some(ref p) = backup_path {
             try!(fs::rename(to, p));
@@ -303,7 +331,7 @@ fn rename(from: &PathBuf, to: &PathBuf, b: &Behaviour) -> Result<()> {
         print!("‘{}’ -> ‘{}’", from.display(), to.display());
         match backup_path {
             Some(path) => println!(" (backup: ‘{}’)", path.display()),
-            None => println!("")
+            None => println!(""),
         }
     }
     Ok(())
@@ -312,35 +340,35 @@ fn rename(from: &PathBuf, to: &PathBuf, b: &Behaviour) -> Result<()> {
 fn read_yes() -> bool {
     let mut s = String::new();
     match BufReader::new(stdin()).read_line(&mut s) {
-        Ok(_) => match s.char_indices().nth(0) {
+        Ok(_) => {
-            Some((_, x)) => x == 'y' || x == 'Y',
+            match s.chars().nth(0) {
-            _ => false
+                Some(x) => x == 'y' || x == 'Y',
-        },
+                _ => false,
-        _ => false
+            }
+        }
+        _ => false,
     }
 }
 
 fn simple_backup_path(path: &PathBuf, suffix: &str) -> PathBuf {
-    let mut p = path.as_os_str().to_str().unwrap().to_owned();
+    let mut p = path.to_string_lossy().into_owned();
     p.push_str(suffix);
     PathBuf::from(p)
 }
 
 fn numbered_backup_path(path: &PathBuf) -> PathBuf {
-    let mut i: u64 = 1;
+    (1_u64..)
-    loop {
+        .map(|i| path.with_extension(format!("~{}~", i)))
-        let new_path = simple_backup_path(path, &format!(".~{}~", i));
+        .skip_while(|p| p.exists())
-        if !new_path.exists() {
+        .next()
-            return new_path;
+        .expect("cannot create backup")
-        }
-        i = i + 1;
-    }
 }
 
 fn existing_backup_path(path: &PathBuf, suffix: &str) -> PathBuf {
-    let test_path = simple_backup_path(path, &".~1~".to_owned());
+    let test_path = path.with_extension("~1~");
     if test_path.exists() {
-        return numbered_backup_path(path);
+        numbered_backup_path(path)
+    } else {
+        simple_backup_path(path, suffix)
     }
-    simple_backup_path(path, suffix)
 }

tests/test_mv.rs

@@ -208,6 +208,25 @@ fn test_mv_custom_backup_suffix() {
     assert!(at.file_exists(&format!("{}{}", file_b, suffix)));
 }
 
+#[test]
+fn test_mv_custom_backup_suffix_via_env() {
+    let (at, mut ucmd) = at_and_ucmd();
+    let file_a = "test_mv_custom_backup_suffix_file_a";
+    let file_b = "test_mv_custom_backup_suffix_file_b";
+    let suffix = "super-suffix-of-the-century";
+    at.touch(file_a);
+    at.touch(file_b);
+    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_mv_backup_numbering() {
     let (at, mut ucmd) = at_and_ucmd();