ignore: correctly detect hidden files on Windows

This commit fixes a bug where ripgrep only treated files beginning with
a `.` as hidden. On Windows, we continue this tradition, but
additionally check whether a file has the special Windows "hidden"
attribute set. If so, we treat it as a hidden file.

In order to make this work without an additional stat call, we had to
rearrange some of the plumbing from the directory traverser.

Fixes #1154

4 files changed, 76 insertions, 34 deletions

diff --git a/ignore/Cargo.toml b/ignore/Cargo.toml
index 0d4b4a7e..9d5e13d9 100644
--- a/ignore/Cargo.toml
+++ b/ignore/Cargo.toml
@@ -29,7 +29,7 @@ thread_local = "0.3.6"
 walkdir = "2.2.7"
 
 [target.'cfg(windows)'.dependencies.winapi-util]
-version = "0.1.1"
+version = "0.1.2"
 
 [dev-dependencies]
 tempfile = "3.0.5"
diff --git a/ignore/src/dir.rs b/ignore/src/dir.rs
index 3ba0ede8..be2b8351 100644
--- a/ignore/src/dir.rs
+++ b/ignore/src/dir.rs
@@ -22,6 +22,7 @@ use gitignore::{self, Gitignore, GitignoreBuilder};
 use pathutil::{is_hidden, strip_prefix};
 use overrides::{self, Override};
 use types::{self, Types};
+use walk::DirEntry;
 use {Error, Match, PartialErrorBuilder};
 
 /// IgnoreMatch represents information about where a match came from when using
@@ -306,11 +307,23 @@ impl Ignore {
                     || has_explicit_ignores
     }
 
+    /// Like `matched`, but works with a directory entry instead.
+    pub fn matched_dir_entry<'a>(
+        &'a self,
+        dent: &DirEntry,
+    ) -> Match<IgnoreMatch<'a>> {
+        let m = self.matched(dent.path(), dent.is_dir());
+        if m.is_none() && self.0.opts.hidden && is_hidden(dent) {
+            return Match::Ignore(IgnoreMatch::hidden());
+        }
+        m
+    }
+
     /// Returns a match indicating whether the given file path should be
     /// ignored or not.
     ///
     /// The match contains information about its origin.
-    pub fn matched<'a, P: AsRef<Path>>(
+    fn matched<'a, P: AsRef<Path>>(
         &'a self,
         path: P,
         is_dir: bool,
@@ -351,9 +364,6 @@ impl Ignore {
                 whitelisted = mat;
             }
         }
-        if whitelisted.is_none() && self.0.opts.hidden && is_hidden(path) {
-            return Match::Ignore(IgnoreMatch::hidden());
-        }
         whitelisted
     }
 
diff --git a/ignore/src/pathutil.rs b/ignore/src/pathutil.rs
index bfd43de3..fbbc0f89 100644
--- a/ignore/src/pathutil.rs
+++ b/ignore/src/pathutil.rs
@@ -1,22 +1,56 @@
 use std::ffi::OsStr;
 use std::path::Path;
 
-/// Returns true if and only if this file path is considered to be hidden.
+use walk::DirEntry;
+
+/// Returns true if and only if this entry is considered to be hidden.
+///
+/// This only returns true if the base name of the path starts with a `.`.
+///
+/// On Unix, this implements a more optimized check.
 #[cfg(unix)]
-pub fn is_hidden<P: AsRef<Path>>(path: P) -> bool {
+pub fn is_hidden(dent: &DirEntry) -> bool {
     use std::os::unix::ffi::OsStrExt;
 
-    if let Some(name) = file_name(path.as_ref()) {
+    if let Some(name) = file_name(dent.path()) {
         name.as_bytes().get(0) == Some(&b'.')
     } else {
         false
     }
 }
 
-/// Returns true if and only if this file path is considered to be hidden.
-#[cfg(not(unix))]
-pub fn is_hidden<P: AsRef<Path>>(path: P) -> bool {
-    if let Some(name) = file_name(path.as_ref()) {
+/// Returns true if and only if this entry is considered to be hidden.
+///
+/// On Windows, this returns true if one of the following is true:
+///
+/// * The base name of the path starts with a `.`.
+/// * The file attributes have the `HIDDEN` property set.
+#[cfg(windows)]
+pub fn is_hidden(dent: &DirEntry) -> bool {
+    use std::os::windows::fs::MetadataExt;
+    use winapi_util::file;
+
+    // This looks like we're doing an extra stat call, but on Windows, the
+    // directory traverser reuses the metadata retrieved from each directory
+    // entry and stores it on the DirEntry itself. So this is "free."
+    if let Ok(md) = dent.metadata() {
+        if file::is_hidden(md.file_attributes() as u64) {
+            return true;
+        }
+    }
+    if let Some(name) = file_name(dent.path()) {
+        name.to_str().map(|s| s.starts_with(".")).unwrap_or(false)
+    } else {
+        false
+    }
+}
+
+/// Returns true if and only if this entry is considered to be hidden.
+///
+/// This only returns true if the base name of the path starts with a `.`.
+#[cfg(not(any(unix, windows)))]
+pub fn is_hidden(dent: &DirEntry) -> bool {
+    if let Some(name) = file_name(dent.path()) {
         name.to_str().map(|s| s.starts_with(".")).unwrap_or(false)
     } else {
         false
diff --git a/ignore/src/walk.rs b/ignore/src/walk.rs
index ae1f58ba..72459220 100644
--- a/ignore/src/walk.rs
+++ b/ignore/src/walk.rs
@@ -99,7 +99,7 @@ impl DirEntry {
     }
 
     /// Returns true if and only if this entry points to a directory.
-    fn is_dir(&self) -> bool {
+    pub(crate) fn is_dir(&self) -> bool {
         self.dent.is_dir()
     }
 
@@ -883,16 +883,17 @@ impl Walk {
                 return Ok(true);
             }
         }
-        let is_dir = ent.file_type().map_or(false, |ft| ft.is_dir());
-        let max_size = self.max_filesize;
-        let should_skip_path = skip_path(&self.ig, ent.path(), is_dir);
-        let should_skip_filesize = if !is_dir && max_size.is_some() {
-            skip_filesize(max_size.unwrap(), ent.path(), &ent.metadata().ok())
-        } else {
-            false
-        };
-
-        Ok(should_skip_path || should_skip_filesize)
+        if should_skip_entry(&self.ig, ent) {
+            return Ok(true);
+        }
+        if self.max_filesize.is_some() && !ent.is_dir() {
+            return Ok(skip_filesize(
+                self.max_filesize.unwrap(),
+                ent.path(),
+                &ent.metadata().ok(),
+            ));
+        }
+        Ok(false)
     }
 }
 
@@ -1420,13 +1421,11 @@ impl Worker {
                 return WalkState::Continue;
             }
         }
-        let is_dir = dent.is_dir();
-        let max_size = self.max_filesize;
-        let should_skip_path = skip_path(ig, dent.path(), is_dir);
+        let should_skip_path = should_skip_entry(ig, &dent);
         let should_skip_filesize =
-            if !is_dir && max_size.is_some() {
+            if self.max_filesize.is_some() && !dent.is_dir() {
                 skip_filesize(
-                    max_size.unwrap(),
+                    self.max_filesize.unwrap(),
                     dent.path(),
                     &dent.metadata().ok(),
                 )
@@ -1609,17 +1608,16 @@ fn skip_filesize(
     }
 }
 
-fn skip_path(
+fn should_skip_entry(
     ig: &Ignore,
-    path: &Path,
-    is_dir: bool,
+    dent: &DirEntry,
 ) -> bool {
-    let m = ig.matched(path, is_dir);
+    let m = ig.matched_dir_entry(dent);
     if m.is_ignore() {
-        debug!("ignoring {}: {:?}", path.display(), m);
+        debug!("ignoring {}: {:?}", dent.path().display(), m);
         true
     } else if m.is_whitelist() {
-        debug!("whitelisting {}: {:?}", path.display(), m);
+        debug!("whitelisting {}: {:?}", dent.path().display(), m);
         false
     } else {
         false