Match up fields with parameter names

The arguments passed to File’s constructor were different from the field names used — these might as well both be the same.

Also, move ext and filename to be File methods to save an import, and add tests.

Also also, by passing a PathBuf in to the constructor directly, we can save one (possibly two) instance/s where we pass in a reference to something we were going to lose ownership of anyway, only to have it basically cloned.

src/exa.rs

@@ -24,7 +24,7 @@ extern crate lazy_static;
 
 use std::ffi::OsStr;
 use std::io::{stderr, Write, Result as IOResult};
-use std::path::{Component, Path};
+use std::path::{Component, PathBuf};
 
 use ansi_term::{ANSIStrings, Style};
 
@@ -75,7 +75,7 @@ impl<'w, W: Write + 'w> Exa<'w, W> {
         }
 
         for file_name in &self.args {
-            match File::new(Path::new(&file_name), None, None) {
+            match File::new(PathBuf::from(file_name), None, None) {
                 Err(e) => {
                     exit_status = 2;
                     writeln!(stderr(), "{}: {}", file_name, e)?;

src/fs/dir.rs

@@ -112,14 +112,12 @@ impl<'dir> Files<'dir> {
     /// Go through the directory until we encounter a file we can list (which
     /// varies depending on the dotfile visibility flag)
     fn next_visible_file(&mut self) -> Option<Result<File<'dir>, (PathBuf, io::Error)>> {
-        use fs::file::path_filename;
-
         loop {
             if let Some(path) = self.inner.next() {
-                let filen = path_filename(path);
+                let filen = File::filename(path);
                 if !self.dotfiles && filen.starts_with(".") { continue }
 
-                return Some(File::new(path, Some(self.dir), Some(filen))
+                return Some(File::new(path.clone(), Some(self.dir), Some(filen))
                                  .map_err(|e| (path.clone(), e)))
             }
             else {
@@ -150,12 +148,12 @@ impl<'dir> Iterator for Files<'dir> {
     fn next(&mut self) -> Option<Self::Item> {
         if let Dots::DotNext = self.dots {
             self.dots = Dots::DotDotNext;
-            Some(File::new(&self.dir.path, Some(self.dir), Some(String::from(".")))
+            Some(File::new(self.dir.path.to_path_buf(), Some(self.dir), Some(String::from(".")))
                       .map_err(|e| (Path::new(".").to_path_buf(), e)))
         }
         else if let Dots::DotDotNext = self.dots {
             self.dots = Dots::FilesNext;
-            Some(File::new(&self.parent(), Some(self.dir), Some(String::from("..")))
+            Some(File::new(self.parent(), Some(self.dir), Some(String::from("..")))
                       .map_err(|e| (self.parent(), e)))
         }
         else {

src/fs/file.rs

@@ -53,34 +53,48 @@ pub struct File<'dir> {
     /// However, *directories* that get passed in will produce files that
     /// contain a reference to it, which is used in certain operations (such
     /// as looking up a file's Git status).
-    pub dir: Option<&'dir Dir>,
+    pub parent_dir: Option<&'dir Dir>,
 }
 
-/// A file’s name is derived from its string. This needs to handle directories
+impl<'dir> File<'dir> {
-/// such as `/` or `..`, which have no `file_name` component. So instead, just
+    pub fn new(path: PathBuf, parent_dir: Option<&'dir Dir>, mut filename: Option<String>) -> IOResult<File<'dir>> {
-/// use the last component as the name.
+        if filename.is_none() {
-pub fn path_filename(path: &Path) -> String {
+            filename = Some(File::filename(&path));
+        }
+
+        let metadata = fs::symlink_metadata(&path)?;
+        let ext      = File::ext(&path);
+
+        Ok(File { path, parent_dir, metadata, ext, name: filename.unwrap() })
+    }
+
+    /// A file’s name is derived from its string. This needs to handle directories
+    /// such as `/` or `..`, which have no `file_name` component. So instead, just
+    /// use the last component as the name.
+    pub fn filename(path: &Path) -> String {
         match path.components().next_back() {
             Some(back) => back.as_os_str().to_string_lossy().to_string(),
             None       => path.display().to_string(),  // use the path as fallback
         }
-}
-
-impl<'dir> File<'dir> {
-    pub fn new(path: &Path, parent: Option<&'dir Dir>, mut filename: Option<String>) -> IOResult<File<'dir>> {
-        if filename.is_none() {
-            filename = Some(path_filename(path));
     }
 
-        let metadata = fs::symlink_metadata(path)?;
+    /// Extract an extension from a file path, if one is present, in lowercase.
+    ///
+    /// The extension is the series of characters after the last dot. This
+    /// deliberately counts dotfiles, so the ".git" folder has the extension "git".
+    ///
+    /// ASCII lowercasing is used because these extensions are only compared
+    /// against a pre-compiled list of extensions which are known to only exist
+    /// within ASCII, so it's alright.
+    fn ext(path: &Path) -> Option<String> {
+        use std::ascii::AsciiExt;
 
-        Ok(File {
+        let name = match path.file_name() {
-            path:      path.to_path_buf(),
+            Some(f) => f.to_string_lossy().to_string(),
-            dir:       parent,
+            None => return None,
-            metadata:  metadata,
+        };
-            ext:       ext(path),
+
-            name:      filename.unwrap(),
+        name.rfind('.').map(|p| name[p+1..].to_ascii_lowercase())
-        })
     }
 
     /// Whether this file is a directory on the filesystem.
@@ -145,7 +159,7 @@ impl<'dir> File<'dir> {
         if path.is_absolute() {
             path.to_path_buf()
         }
-        else if let Some(dir) = self.dir {
+        else if let Some(dir) = self.parent_dir {
             dir.join(&*path)
         }
         else if let Some(parent) = self.path.parent() {
@@ -172,26 +186,22 @@ impl<'dir> File<'dir> {
         // this file -- which could be absolute or relative -- to the path
         // we actually look up and turn into a `File` -- which needs to be
         // absolute to be accessible from any directory.
-        let display_path = match fs::read_link(&self.path) {
+        let path = match fs::read_link(&self.path) {
-            Ok(path)  => path,
+            Ok(p)   => p,
             Err(e)  => return FileTarget::Err(e),
         };
 
-        let target_path = self.reorient_target_path(&*display_path);
+        let absolute_path = self.reorient_target_path(&path);
 
         // Use plain `metadata` instead of `symlink_metadata` - we *want* to
         // follow links.
-        if let Ok(metadata) = fs::metadata(&target_path) {
+        if let Ok(metadata) = fs::metadata(&absolute_path) {
-            FileTarget::Ok(File {
+            let ext  = File::ext(&path);
-                dir:       None,
+            let name = File::filename(&path);
-                ext:       ext(&display_path),
+            FileTarget::Ok(File { parent_dir: None, path, ext, metadata, name })
-                metadata:  metadata,
-                name:      path_filename(&display_path),
-                path:      display_path,
-            })
         }
         else {
-            FileTarget::Broken(display_path)
+            FileTarget::Broken(path)
         }
     }
 
@@ -355,7 +365,7 @@ impl<'dir> File<'dir> {
     pub fn git_status(&self) -> f::Git {
         use std::env::current_dir;
 
-        match self.dir {
+        match self.parent_dir {
             None    => f::Git { staged: f::GitStatus::NotModified, unstaged: f::GitStatus::NotModified },
             Some(d) => {
                 let cwd = match current_dir() {
@@ -377,26 +387,6 @@ impl<'a> AsRef<File<'a>> for File<'a> {
 }
 
 
-/// Extract an extension from a file path, if one is present, in lowercase.
-///
-/// The extension is the series of characters after the last dot. This
-/// deliberately counts dotfiles, so the ".git" folder has the extension "git".
-///
-/// ASCII lowercasing is used because these extensions are only compared
-/// against a pre-compiled list of extensions which are known to only exist
-/// within ASCII, so it's alright.
-fn ext(path: &Path) -> Option<String> {
-    use std::ascii::AsciiExt;
-
-    let name = match path.file_name() {
-        Some(f) => f.to_string_lossy().to_string(),
-        None => return None,
-    };
-
-    name.rfind('.').map(|p| name[p+1..].to_ascii_lowercase())
-}
-
-
 /// The result of following a symlink.
 pub enum FileTarget<'dir> {
 
@@ -458,22 +448,59 @@ mod modes {
 
 
 #[cfg(test)]
-mod test {
+mod ext_test {
-    use super::ext;
+    use super::File;
     use std::path::Path;
 
     #[test]
     fn extension() {
-        assert_eq!(Some("dat".to_string()), ext(Path::new("fester.dat")))
+        assert_eq!(Some("dat".to_string()), File::ext(Path::new("fester.dat")))
     }
 
     #[test]
     fn dotfile() {
-        assert_eq!(Some("vimrc".to_string()), ext(Path::new(".vimrc")))
+        assert_eq!(Some("vimrc".to_string()), File::ext(Path::new(".vimrc")))
     }
 
     #[test]
     fn no_extension() {
-        assert_eq!(None, ext(Path::new("jarlsberg")))
+        assert_eq!(None, File::ext(Path::new("jarlsberg")))
+    }
+}
+
+
+#[cfg(test)]
+mod filename_test {
+    use super::File;
+    use std::path::Path;
+
+    #[test]
+    fn file() {
+        assert_eq!("fester.dat", File::filename(Path::new("fester.dat")))
+    }
+
+    #[test]
+    fn no_path() {
+        assert_eq!("foo.wha", File::filename(Path::new("/var/cache/foo.wha")))
+    }
+
+    #[test]
+    fn here() {
+        assert_eq!(".", File::filename(Path::new(".")))
+    }
+
+    #[test]
+    fn there() {
+        assert_eq!("..", File::filename(Path::new("..")))
+    }
+
+    #[test]
+    fn everywhere() {
+        assert_eq!("..", File::filename(Path::new("./..")))
+    }
+
+    #[test]
+    fn topmost() {
+        assert_eq!("/", File::filename(Path::new("/")))
     }
 }

src/info/filetype.rs

@@ -80,7 +80,7 @@ impl<'a> File<'a> {
         if self.extension_is_one_of( &[ "class", "elc", "hi", "o", "pyc" ]) {
             true
         }
-        else if let Some(dir) = self.dir {
+        else if let Some(dir) = self.parent_dir {
             self.get_source_files().iter().any(|path| dir.contains(path))
         }
         else {

src/output/file_name.rs

@@ -49,7 +49,7 @@ impl<'a, 'dir> FileName<'a, 'dir> {
     pub fn paint(&self) -> TextCellContents {
         let mut bits = Vec::new();
 
-        if self.file.dir.is_none() {
+        if self.file.parent_dir.is_none() {
             if let Some(parent) = self.file.path.parent() {
                 self.add_parent_bits(&mut bits, parent);
             }