Fix panic when previewing symlink to ., .., or /

The old implementation blindly assumed that a symlink target would have a directory compoment, which the current directory, parent directory, and root directory technically don't have.

Fixes #20.

1 files changed, 72 insertions, 23 deletions

diff --git a/src/file.rs b/src/file.rs
index d1cd7b8..6d89fef 100644
--- a/src/file.rs
+++ b/src/file.rs
@@ -54,18 +54,7 @@ impl<'a> File<'a> {
 
     /// Create a new File object from the given Stat result, and other data.
     pub fn with_stat(stat: io::FileStat, path: &Path, parent: Option<&'a Dir>, recurse: bool) -> File<'a> {
-
-        // The filename to display is the last component of the path. However,
-        // the path has no components for `.`, `..`, and `/`, so in these
-        // cases, the entire path is used.
-        let bytes = match path.components().last() {
-            Some(b) => b,
-            None => path.as_vec(),
-        };
-
-        // Convert the string to UTF-8, replacing any invalid characters with
-        // replacement characters.
-        let filename = String::from_utf8_lossy(bytes);
+        let filename = path_filename(path);
 
         // If we are recursing, then the `this` field contains a Dir object
         // that represents the current File as a directory, if it is a
@@ -81,8 +70,8 @@ impl<'a> File<'a> {
             path:  path.clone(),
             dir:   parent,
             stat:  stat,
-            name:  filename.to_string(),
             ext:   ext(&filename),
+            name:  filename,
             this:  this,
         }
     }
@@ -146,12 +135,44 @@ impl<'a> File<'a> {
             };
 
             match self.target_file(&target_path) {
-                Ok(file) => format!("{} {} {}{}{}",
-                                   style.paint(name),
-                                   GREY.paint("=>"),
-                                   Cyan.paint(target_path.dirname_str().unwrap()),
-                                   Cyan.paint("/"),
-                                   file.file_colour().paint(&file.name)),
+                Ok(file) => {
+
+                    // Generate a preview for the path this symlink links to.
+                    // The preview should consist of the directory of the file
+                    // (if present) in cyan, an extra slash if necessary, then
+                    // the target file, colourised in the appropriate style.
+                    let mut path_prefix = String::new();
+
+                    // The root directory has the name "/", which has to be
+                    // catered for separately, otherwise there'll be two
+                    // slashes in the resulting output.
+                    if file.path.is_absolute() && file.name != "/" {
+                        path_prefix.push_str("/");
+                    }
+
+                    let path_bytes: Vec<&[u8]> = file.path.components().collect();
+                    if !path_bytes.is_empty() {
+                        // Use init() to add all but the last component of the
+                        // path to the prefix. init() panics when given an
+                        // empty list, hence the check.
+                        for component in path_bytes.init().iter() {
+                            let string = String::from_utf8_lossy(component).to_string();
+                            path_prefix.push_str(&string);
+                        }
+                    }
+
+                    // Only add a slash when there's something in the path
+                    // prefix so far.
+                    if path_bytes.len() > 1 {
+                        path_prefix.push_str("/");
+                    }
+
+                    format!("{} {} {}",
+                            style.paint(name),
+                            GREY.paint("=>"),
+                            ANSIStrings(&[ Cyan.paint(&path_prefix),
+                                           file.file_colour().paint(&file.name) ]))
+                },
                 Err(filename) => format!("{} {} {}",
                                          style.paint(name),
                                          Red.paint("=>"),
@@ -184,8 +205,7 @@ impl<'a> File<'a> {
     /// other end doesn't exist), returns the *filename* of the file
     /// that should be there.
     fn target_file(&self, target_path: &Path) -> Result<File, String> {
-        let v = target_path.filename().unwrap();
-        let filename = String::from_utf8_lossy(v);
+        let filename = path_filename(target_path);
 
         // Use stat instead of lstat - we *want* to follow links.
         if let Ok(stat) = fs::stat(target_path) {
@@ -193,8 +213,8 @@ impl<'a> File<'a> {
                 path:  target_path.clone(),
                 dir:   self.dir,
                 stat:  stat,
-                name:  filename.to_string(),
                 ext:   ext(&filename),
+                name:  filename,
                 this:  None,
             })
         }
@@ -425,6 +445,21 @@ impl<'a> File<'a> {
     }
 }
 
+/// Extract the filename to display from a path, converting it from UTF-8
+/// lossily, into a String.
+///
+/// The filename to display is the last component of the path. However,
+/// the path has no components for `.`, `..`, and `/`, so in these
+/// cases, the entire path is used.
+fn path_filename(path: &Path) -> String {
+    let bytes = match path.components().last() {
+        Some(b) => b,
+        None => path.as_vec(),
+    };
+
+    String::from_utf8_lossy(bytes).to_string()
+}
+
 /// Extract an extension from a string, if one is present, in lowercase.
 ///
 /// The extension is the series of characters after the last dot. This
@@ -440,15 +475,29 @@ fn ext<'a>(name: &'a str) -> Option<String> {
 #[cfg(test)]
 pub mod test {
     pub use super::*;
+    pub use super::path_filename;
+
     pub use column::{Cell, Column};
     pub use std::old_io as io;
+    pub use output::details::UserLocale;
+
     pub use users::{User, Group};
     pub use users::mock::MockUsers;
 
     pub use ansi_term::Style::Plain;
     pub use ansi_term::Colour::Yellow;
 
-    pub use output::details::UserLocale;
+    #[test]
+    fn current_filename() {
+        let filename = path_filename(&Path::new("."));
+        assert_eq!(&filename[..], ".")
+    }
+
+    #[test]
+    fn parent_filename() {
+        let filename = path_filename(&Path::new(".."));
+        assert_eq!(&filename[..], "..")
+    }
 
     #[test]
     fn extension() {