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

5 files changed, 83 insertions, 41 deletions

diff --git a/Cargo.lock b/Cargo.lock
index 22c48e26..50bf552c 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -170,7 +170,7 @@ dependencies = [
  "regex 1.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "same-file 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)",
  "termcolor 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)",
- "winapi-util 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
+ "winapi-util 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [[package]]
@@ -244,7 +244,7 @@ dependencies = [
  "tempfile 3.0.5 (registry+https://github.com/rust-lang/crates.io-index)",
  "thread_local 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)",
  "walkdir 2.2.7 (registry+https://github.com/rust-lang/crates.io-index)",
- "winapi-util 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
+ "winapi-util 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [[package]]
@@ -537,7 +537,7 @@ name = "same-file"
 version = "1.0.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
- "winapi-util 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
+ "winapi-util 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [[package]]
@@ -703,7 +703,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
  "same-file 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)",
  "winapi 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)",
- "winapi-util 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
+ "winapi-util 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [[package]]
@@ -722,7 +722,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 
 [[package]]
 name = "winapi-util"
-version = "0.1.1"
+version = "0.1.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
  "winapi 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -739,7 +739,7 @@ version = "1.0.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
  "winapi 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)",
- "winapi-util 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
+ "winapi-util 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [metadata]
@@ -820,6 +820,6 @@ dependencies = [
 "checksum walkdir 2.2.7 (registry+https://github.com/rust-lang/crates.io-index)" = "9d9d7ed3431229a144296213105a390676cc49c9b6a72bd19f3176c98e129fa1"
 "checksum winapi 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)" = "92c1eb33641e276cfa214a0522acad57be5c56b10cb348b3c5117db75f3ac4b0"
 "checksum winapi-i686-pc-windows-gnu 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)" = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6"
-"checksum winapi-util 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "afc5508759c5bf4285e61feb862b6083c8480aec864fa17a81fdec6f69b461ab"
+"checksum winapi-util 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)" = "7168bab6e1daee33b4557efd0e95d5ca70a03706d39fa5f3fe7a236f584b03c9"
 "checksum winapi-x86_64-pc-windows-gnu 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)" = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f"
 "checksum wincolor 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "561ed901ae465d6185fa7864d63fbd5720d0ef718366c9a4dc83cf6170d7e9ba"
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