Override the names of . and ..

There was a problem when displaying . and .. in directory listings: their names would normalise to actual names! So instead of literally seeing `.`, you’d see the current directory’s name, inserted in sort order into the list of results. Obviously this is not what we want.

In unrelated news, putting `.` and `..` into the list of paths read from a directory just takes up more heap space for something that’s basically constant.

We can solve both these problems at once by moving the DotFilter to the files iterator in Dir, rather than at the Dir’s creation. Having the iterator know whether it should display `.` and `..` means it can emit those files first, and because it knows what those files really represent, it can override their file names to actually be those sequences of dots.

This is not a perfect solution: the main casualty is that a File can now be constructed with a name, some metadata, both, or neither. This is currently handled with a bunch of Options, and returns IOResult even without doing any IO operations.

But at least all the tests pass!

Vagrantfile

@@ -369,6 +369,36 @@ Vagrant.configure(2) do |config|
     EOF
 
 
+    # Hidden and dot file testcases.
+    # We need to set the permissions of `.` and `..` because they actually
+    # get displayed in the output here, so this has to come last.
+    config.vm.provision :shell, privileged: false, inline: <<-EOF
+        set -xe
+        shopt -u dotglob
+        GLOBIGNORE=".:.."
+
+        mkdir "#{test_dir}/hiddens"
+        touch "#{test_dir}/hiddens/visible"
+        touch "#{test_dir}/hiddens/.hidden"
+        touch "#{test_dir}/hiddens/..extra-hidden"
+
+        # ./hiddens/
+        touch -t #{some_date}      "#{test_dir}/hiddens/"*
+        chmod 644                  "#{test_dir}/hiddens/"*
+        sudo chown #{user}:#{user} "#{test_dir}/hiddens/"*
+
+        # .
+        touch -t #{some_date} "#{test_dir}/hiddens"
+        chmod 755 "#{test_dir}/hiddens"
+        sudo chown #{user}:#{user} "#{test_dir}/hiddens"
+
+        # ..
+        sudo touch -t #{some_date} "#{test_dir}"
+        sudo chmod 755 "#{test_dir}"
+        sudo chown #{user}:#{user} "#{test_dir}"
+    EOF
+
+
     # Install kcov for test coverage
     # This doesn’t run coverage over the xtests so it’s less useful for now
     if ENV.key?('INSTALL_KCOV')

src/exa.rs

@@ -75,14 +75,14 @@ impl<'w, W: Write + 'w> Exa<'w, W> {
         }
 
         for file_name in &self.args {
-            match File::from_path(Path::new(&file_name), None) {
+            match File::new(Path::new(&file_name), None, None, None) {
                 Err(e) => {
                     exit_status = 2;
                     writeln!(stderr(), "{}: {}", file_name, e)?;
                 },
                 Ok(f) => {
                     if f.is_directory() && !self.options.dir_action.treat_dirs_as_files() {
-                        match f.to_dir(self.options.filter.dot_filter, self.options.should_scan_for_git()) {
+                        match f.to_dir(self.options.should_scan_for_git()) {
                             Ok(d) => dirs.push(d),
                             Err(e) => writeln!(stderr(), "{}: {}", file_name, e)?,
                         }
@@ -126,7 +126,7 @@ impl<'w, W: Write + 'w> Exa<'w, W> {
             }
 
             let mut children = Vec::new();
-            for file in dir.files() {
+            for file in dir.files(self.options.filter.dot_filter) {
                 match file {
                     Ok(file)       => children.push(file),
                     Err((path, e)) => writeln!(stderr(), "[{}: {}]", path.display(), e)?,
@@ -142,7 +142,7 @@ impl<'w, W: Write + 'w> Exa<'w, W> {
 
                     let mut child_dirs = Vec::new();
                     for child_dir in children.iter().filter(|f| f.is_directory()) {
-                        match child_dir.to_dir(self.options.filter.dot_filter, false) {
+                        match child_dir.to_dir(false) {
                             Ok(d)  => child_dirs.push(d),
                             Err(e) => writeln!(stderr(), "{}: {}", child_dir.path.display(), e)?,
                         }

src/fs/dir.rs

@@ -36,18 +36,10 @@ impl Dir {
     /// The `read_dir` iterator doesn’t actually yield the `.` and `..`
     /// entries, so if the user wants to see them, we’ll have to add them
     /// ourselves after the files have been read.
-    pub fn read_dir(path: PathBuf, dots: DotFilter, git: bool) -> IOResult<Dir> {
-        let mut contents: Vec<PathBuf> = try!(fs::read_dir(&path)?
+    pub fn read_dir(path: PathBuf, git: bool) -> IOResult<Dir> {
+        let contents: Vec<PathBuf> = try!(fs::read_dir(&path)?
                                                  .map(|result| result.map(|entry| entry.path()))
                                                  .collect());
-        match dots {
-            DotFilter::JustFiles => contents.retain(|p| p.file_name().and_then(|name| name.to_str()).map(|s| !s.starts_with('.')).unwrap_or(true)),
-            DotFilter::Dotfiles => {/* Don’t add or remove anything */},
-            DotFilter::DotfilesAndDots => {
-                contents.insert(0, path.join(".."));
-                contents.insert(0, path.join("."));
-            }
-        }
 
         let git = if git { Git::scan(&path).ok() } else { None };
         Ok(Dir { contents, path, git })
@@ -55,10 +47,12 @@ impl Dir {
 
     /// Produce an iterator of IO results of trying to read all the files in
     /// this directory.
-    pub fn files(&self) -> Files {
+    pub fn files(&self, dots: DotFilter) -> Files {
         Files {
-            inner: self.contents.iter(),
-            dir: self,
+            inner:     self.contents.iter(),
+            dir:       self,
+            dotfiles:  dots.shows_dotfiles(),
+            dots:      dots.dots(),
         }
     }
 
@@ -90,15 +84,83 @@ impl Dir {
 
 /// Iterator over reading the contents of a directory as `File` objects.
 pub struct Files<'dir> {
+
+    /// The internal iterator over the paths that have been read already.
     inner: SliceIter<'dir, PathBuf>,
+
+    /// The directory that begat those paths.
     dir: &'dir Dir,
+
+    /// Whether to include dotfiles in the list.
+    dotfiles: bool,
+
+    /// Whether the `.` or `..` directories should be produced first, before
+    /// any files have been listed.
+    dots: Dots,
 }
 
+impl<'dir> Files<'dir> {
+    fn parent(&self) -> PathBuf {
+        // We can’t use `Path#parent` here because all it does is remove the
+        // last path component, which is no good for us if the path is
+        // relative. For example, while the parent of `/testcases/files` is
+        // `/testcases`, the parent of `.` is an empty path. Adding `..` on
+        // the end is the only way to get to the *actual* parent directory.
+        self.dir.path.join("..")
+    }
+
+    /// 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);
+                if !self.dotfiles && filen.starts_with(".") { continue }
+
+                return Some(File::new(path, Some(self.dir), Some(filen), None)
+                                 .map_err(|e| (path.clone(), e)))
+            }
+            else {
+                return None
+            }
+        }
+    }
+}
+
+/// The dot directories that need to be listed before actual files, if any.
+/// If these aren’t being printed, then `FilesNext` is used to skip them.
+enum Dots {
+
+    /// List the `.` directory next.
+    DotNext,
+
+    /// List the `..` directory next.
+    DotDotNext,
+
+    /// Forget about the dot directories and just list files.
+    FilesNext,
+}
+
+
 impl<'dir> Iterator for Files<'dir> {
     type Item = Result<File<'dir>, (PathBuf, io::Error)>;
 
     fn next(&mut self) -> Option<Self::Item> {
-        self.inner.next().map(|path| File::from_path(path, Some(self.dir)).map_err(|t| (path.clone(), t)))
+        if let Dots::DotNext = self.dots {
+            self.dots = Dots::DotDotNext;
+            Some(File::new(&self.dir.path, Some(self.dir), Some(String::from(".")), None)
+                      .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("..")), None)
+                      .map_err(|e| (self.parent(), e)))
+        }
+        else {
+            self.next_visible_file()
+        }
     }
 }
 
@@ -124,3 +186,24 @@ impl Default for DotFilter {
         DotFilter::JustFiles
     }
 }
+
+impl DotFilter {
+
+    /// Whether this filter should show dotfiles in a listing.
+    fn shows_dotfiles(&self) -> bool {
+        match *self {
+            DotFilter::JustFiles       => false,
+            DotFilter::Dotfiles        => true,
+            DotFilter::DotfilesAndDots => true,
+        }
+    }
+
+    /// Whether this filter should add dot directories to a listing.
+    fn dots(&self) -> Dots {
+        match *self {
+            DotFilter::JustFiles       => Dots::FilesNext,
+            DotFilter::Dotfiles        => Dots::FilesNext,
+            DotFilter::DotfilesAndDots => Dots::DotNext,
+        }
+    }
+}

src/fs/file.rs

@@ -6,7 +6,7 @@ use std::io::Result as IOResult;
 use std::os::unix::fs::{MetadataExt, PermissionsExt, FileTypeExt};
 use std::path::{Path, PathBuf};
 
-use fs::dir::{Dir, DotFilter};
+use fs::dir::Dir;
 use fs::fields as f;
 
 
@@ -56,31 +56,33 @@ pub struct File<'dir> {
     pub dir: Option<&'dir Dir>,
 }
 
-impl<'dir> File<'dir> {
-
-    /// Create a new `File` object from the given `Path`, inside the given
-    /// `Dir`, if appropriate.
-    ///
-    /// This uses `symlink_metadata` instead of `metadata`, which doesn't
-    /// follow symbolic links.
-    pub fn from_path(path: &Path, parent: Option<&'dir Dir>) -> IOResult<File<'dir>> {
-        fs::symlink_metadata(path).map(|metadata| File::with_metadata(metadata, path, parent))
+/// 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 path_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
     }
+}
 
-    /// Create a new File object from the given metadata result, and other data.
-    pub fn with_metadata(metadata: fs::Metadata, path: &Path, parent: Option<&'dir Dir>) -> File<'dir> {
-        let filename = match path.components().next_back() {
-            Some(comp) => comp.as_os_str().to_string_lossy().to_string(),
-            None       => String::new(),
-        };
+impl<'dir> File<'dir> {
+    pub fn new(path: &Path, parent: Option<&'dir Dir>, mut filename: Option<String>, mut metadata: Option<fs::Metadata>) -> IOResult<File<'dir>> {
+        if filename.is_none() {
+            filename = Some(path_filename(path));
+        }
 
-        File {
+        if metadata.is_none() {
+            metadata = Some(fs::symlink_metadata(path)?);
+        }
+
+        Ok(File {
             path:      path.to_path_buf(),
             dir:       parent,
-            metadata:  metadata,
+            metadata:  metadata.unwrap(),
             ext:       ext(path),
-            name:      filename,
-        }
+            name:      filename.unwrap(),
+        })
     }
 
     /// Whether this file is a directory on the filesystem.
@@ -94,8 +96,8 @@ impl<'dir> File<'dir> {
     ///
     /// Returns an IO error upon failure, but this shouldn't be used to check
     /// if a `File` is a directory or not! For that, just use `is_directory()`.
-    pub fn to_dir(&self, dots: DotFilter, scan_for_git: bool) -> IOResult<Dir> {
-        Dir::read_dir(self.path.clone(), dots, scan_for_git)
+    pub fn to_dir(&self, scan_for_git: bool) -> IOResult<Dir> {
+        Dir::read_dir(self.path.clone(), scan_for_git)
     }
 
     /// Whether this file is a regular file on the filesystem - that is, not a
@@ -182,7 +184,7 @@ impl<'dir> File<'dir> {
         // Use plain `metadata` instead of `symlink_metadata` - we *want* to
         // follow links.
         if let Ok(metadata) = fs::metadata(&target_path) {
-            FileTarget::Ok(File::with_metadata(metadata, &*display_path, None))
+            FileTarget::Ok(File::new(&*display_path, None, None, Some(metadata)).unwrap())
         }
         else {
             FileTarget::Broken(display_path)

src/output/details.rs

@@ -315,7 +315,7 @@ impl<'a> Render<'a> {
 
                     if let Some(r) = self.recurse {
                         if file.is_directory() && r.tree && !r.is_too_deep(depth) {
-                            if let Ok(d) = file.to_dir(self.filter.dot_filter, false) {
+                            if let Ok(d) = file.to_dir(false) {
                                 dir = Some(d);
                             }
                         }
@@ -344,7 +344,7 @@ impl<'a> Render<'a> {
             table.rows.push(row);
 
             if let Some(ref dir) = egg.dir {
-                for file_to_add in dir.files() {
+                for file_to_add in dir.files(self.filter.dot_filter) {
                     match file_to_add {
                         Ok(f)          => files.push(f),
                         Err((path, e)) => errors.push((e, Some(path)))

xtests/hiddens

@@ -0,0 +1 @@
+visible

xtests/hiddens_a

@@ -0,0 +1 @@
+..extra-hidden  .hidden  visible

xtests/hiddens_aa

@@ -0,0 +1 @@
+.  ..  ..extra-hidden  .hidden  visible

xtests/hiddens_l

@@ -0,0 +1 @@
+.rw-r--r-- 0 cassowary  1 Jan 12:34 visible

xtests/hiddens_la

@@ -0,0 +1,3 @@
+.rw-r--r-- 0 cassowary  1 Jan 12:34 ..extra-hidden
+.rw-r--r-- 0 cassowary  1 Jan 12:34 .hidden
+.rw-r--r-- 0 cassowary  1 Jan 12:34 visible

xtests/hiddens_laa

@@ -0,0 +1,5 @@
+drwxr-xr-x - cassowary  1 Jan 12:34 .
+drwxr-xr-x - cassowary  1 Jan 12:34 ..
+.rw-r--r-- 0 cassowary  1 Jan 12:34 ..extra-hidden
+.rw-r--r-- 0 cassowary  1 Jan 12:34 .hidden
+.rw-r--r-- 0 cassowary  1 Jan 12:34 visible

xtests/run.sh

@@ -38,7 +38,7 @@ COLUMNS=120 $exa $testcases/files | diff -q - $results/files_120  || exit 1
 COLUMNS=160 $exa $testcases/files | diff -q - $results/files_160  || exit 1
 COLUMNS=200 $exa $testcases/files | diff -q - $results/files_200  || exit 1
 
-COLUMNS=100 $exa $testcases/files/* | diff -q - $results/files_star_100   || exit 1
+COLUMNS=100 $exa $testcases/files/* | diff -q - $results/files_star_100  || exit 1
 COLUMNS=150 $exa $testcases/files/* | diff -q - $results/files_star_150  || exit 1
 COLUMNS=200 $exa $testcases/files/* | diff -q - $results/files_star_200  || exit 1
 
@@ -120,8 +120,20 @@ COLUMNS=80 $exa_binary --colour=automatic $testcases/files -l | diff -q - $resul
 $exa $testcases/git/additions -l --git 2>&1 | diff -q - $results/git_additions  || exit 1
 $exa $testcases/git/edits     -l --git 2>&1 | diff -q - $results/git_edits      || exit 1
 
+
+# Hidden files
+COLUMNS=80 $exa $testcases/hiddens     2>&1 | diff -q - $results/hiddens     || exit 1
+COLUMNS=80 $exa $testcases/hiddens -a  2>&1 | diff -q - $results/hiddens_a   || exit 1
+COLUMNS=80 $exa $testcases/hiddens -aa 2>&1 | diff -q - $results/hiddens_aa  || exit 1
+
+$exa $testcases/hiddens -l     2>&1 | diff -q - $results/hiddens_l    || exit 1
+$exa $testcases/hiddens -l -a  2>&1 | diff -q - $results/hiddens_la   || exit 1
+$exa $testcases/hiddens -l -aa 2>&1 | diff -q - $results/hiddens_laa  || exit 1
+
+
 # And finally...
 $exa --help        | diff -q - $results/help      || exit 1
 $exa --help --long | diff -q - $results/help_long || exit 1
 
+
 echo "All the tests passed!"