ignore: only respect .gitignore in git repos

This commit fixes an interesting bug in the `ignore` crate where it
would basically respect any `.gitignore` file anywhere (including global
gitignores in `~/.config/git/ignore`), regardless of whether we were
searching in a git repository or not. This commit rectifies that
behavior to only respect gitignore rules when in a git repo.

The key change here is to move the logic of whether to traverse parents
into the directory matcher rather than putting the onus on the directory
traverser. In particular, we now need to traverse parent directories in
more cases than we previously did, since we need to determine whether
we're in a git repository or not.

Fixes #934

2 files changed, 84 insertions, 50 deletions

diff --git a/ignore/src/dir.rs b/ignore/src/dir.rs
index 3e98f6f5..162b4c03 100644
--- a/ignore/src/dir.rs
+++ b/ignore/src/dir.rs
@@ -65,6 +65,8 @@ struct IgnoreOptions {
     hidden: bool,
     /// Whether to read .ignore files.
     ignore: bool,
+    /// Whether to respect any ignore files in parent directories.
+    parents: bool,
     /// Whether to read git's global gitignore file.
     git_global: bool,
     /// Whether to read .gitignore files.
@@ -151,6 +153,15 @@ impl Ignore {
         &self,
         path: P,
     ) -> (Ignore, Option<Error>) {
+        if !self.0.opts.parents
+            && !self.0.opts.git_ignore
+            && !self.0.opts.git_exclude
+            && !self.0.opts.git_global
+        {
+            // If we never need info from parent directories, then don't do
+            // anything.
+            return (self.clone(), None);
+        }
         if !self.is_root() {
             panic!("Ignore::add_parents called on non-root matcher");
         }
@@ -183,6 +194,7 @@ impl Ignore {
             errs.maybe_push(err);
             igtmp.is_absolute_parent = true;
             igtmp.absolute_base = Some(absolute_base.clone());
+            igtmp.has_git = parent.join(".git").exists();
             ig = Ignore(Arc::new(igtmp));
             compiled.insert(parent.as_os_str().to_os_string(), ig.clone());
         }
@@ -333,6 +345,7 @@ impl Ignore {
     ) -> Match<IgnoreMatch<'a>> {
         let (mut m_custom_ignore, mut m_ignore, mut m_gi, mut m_gi_exclude, mut m_explicit) =
             (Match::None, Match::None, Match::None, Match::None, Match::None);
+        let any_git = self.parents().any(|ig| ig.0.has_git);
         let mut saw_git = false;
         for ig in self.parents().take_while(|ig| !ig.0.is_absolute_parent) {
             if m_custom_ignore.is_none() {
@@ -345,42 +358,44 @@ impl Ignore {
                     ig.0.ignore_matcher.matched(path, is_dir)
                       .map(IgnoreMatch::gitignore);
             }
-            if !saw_git && m_gi.is_none() {
+            if any_git && !saw_git && m_gi.is_none() {
                 m_gi =
                     ig.0.git_ignore_matcher.matched(path, is_dir)
                       .map(IgnoreMatch::gitignore);
             }
-            if !saw_git && m_gi_exclude.is_none() {
+            if any_git && !saw_git && m_gi_exclude.is_none() {
                 m_gi_exclude =
                     ig.0.git_exclude_matcher.matched(path, is_dir)
                       .map(IgnoreMatch::gitignore);
             }
             saw_git = saw_git || ig.0.has_git;
         }
-        if let Some(abs_parent_path) = self.absolute_base() {
-            let path = abs_parent_path.join(path);
-            for ig in self.parents().skip_while(|ig|!ig.0.is_absolute_parent) {
-                if m_custom_ignore.is_none() {
-                    m_custom_ignore =
-                        ig.0.custom_ignore_matcher.matched(&path, is_dir)
-                          .map(IgnoreMatch::gitignore);
-                }
-                if m_ignore.is_none() {
-                    m_ignore =
-                        ig.0.ignore_matcher.matched(&path, is_dir)
-                          .map(IgnoreMatch::gitignore);
-                }
-                if !saw_git && m_gi.is_none() {
-                    m_gi =
-                        ig.0.git_ignore_matcher.matched(&path, is_dir)
-                          .map(IgnoreMatch::gitignore);
+        if self.0.opts.parents {
+            if let Some(abs_parent_path) = self.absolute_base() {
+                let path = abs_parent_path.join(path);
+                for ig in self.parents().skip_while(|ig|!ig.0.is_absolute_parent) {
+                    if m_custom_ignore.is_none() {
+                        m_custom_ignore =
+                            ig.0.custom_ignore_matcher.matched(&path, is_dir)
+                              .map(IgnoreMatch::gitignore);
+                    }
+                    if m_ignore.is_none() {
+                        m_ignore =
+                            ig.0.ignore_matcher.matched(&path, is_dir)
+                              .map(IgnoreMatch::gitignore);
+                    }
+                    if any_git && !saw_git && m_gi.is_none() {
+                        m_gi =
+                            ig.0.git_ignore_matcher.matched(&path, is_dir)
+                              .map(IgnoreMatch::gitignore);
+                    }
+                    if any_git && !saw_git && m_gi_exclude.is_none() {
+                        m_gi_exclude =
+                            ig.0.git_exclude_matcher.matched(&path, is_dir)
+                              .map(IgnoreMatch::gitignore);
+                    }
+                    saw_git = saw_git || ig.0.has_git;
                 }
-                if !saw_git && m_gi_exclude.is_none() {
-                    m_gi_exclude =
-                        ig.0.git_exclude_matcher.matched(&path, is_dir)
-                          .map(IgnoreMatch::gitignore);
-                }
-                saw_git = saw_git || ig.0.has_git;
             }
         }
         for gi in self.0.explicit_ignores.iter().rev() {
@@ -389,8 +404,14 @@ impl Ignore {
             }
             m_explicit = gi.matched(&path, is_dir).map(IgnoreMatch::gitignore);
         }
-        let m_global = self.0.git_global_matcher.matched(&path, is_dir)
-                           .map(IgnoreMatch::gitignore);
+        let m_global =
+            if any_git {
+                self.0.git_global_matcher
+                    .matched(&path, is_dir)
+                    .map(IgnoreMatch::gitignore)
+            } else {
+                Match::None
+            };
 
         m_custom_ignore.or(m_ignore).or(m_gi).or(m_gi_exclude).or(m_global).or(m_explicit)
     }
@@ -458,6 +479,7 @@ impl IgnoreBuilder {
             opts: IgnoreOptions {
                 hidden: true,
                 ignore: true,
+                parents: true,
                 git_global: true,
                 git_ignore: true,
                 git_exclude: true,
@@ -560,6 +582,17 @@ impl IgnoreBuilder {
         self
     }
 
+    /// Enables reading ignore files from parent directories.
+    ///
+    /// If this is enabled, then .gitignore files in parent directories of each
+    /// file path given are respected. Otherwise, they are ignored.
+    ///
+    /// This is enabled by default.
+    pub fn parents(&mut self, yes: bool) -> &mut IgnoreBuilder {
+        self.opts.parents = yes;
+        self
+    }
+
     /// Add a global gitignore matcher.
     ///
     /// Its precedence is lower than both normal `.gitignore` files and
@@ -681,6 +714,7 @@ mod tests {
     #[test]
     fn gitignore() {
         let td = TempDir::new("ignore-test-").unwrap();
+        mkdirp(td.path().join(".git"));
         wfile(td.path().join(".gitignore"), "foo\n!bar");
 
         let (ig, err) = IgnoreBuilder::new().build().add_child(td.path());
@@ -691,6 +725,18 @@ mod tests {
     }
 
     #[test]
+    fn gitignore_no_git() {
+        let td = TempDir::new("ignore-test-").unwrap();
+        wfile(td.path().join(".gitignore"), "foo\n!bar");
+
+        let (ig, err) = IgnoreBuilder::new().build().add_child(td.path());
+        assert!(err.is_none());
+        assert!(ig.matched("foo", false).is_none());
+        assert!(ig.matched("bar", false).is_none());
+        assert!(ig.matched("baz", false).is_none());
+    }
+
+    #[test]
     fn ignore() {
         let td = TempDir::new("ignore-test-").unwrap();
         wfile(td.path().join(".ignore"), "foo\n!bar");
@@ -799,6 +845,7 @@ mod tests {
     #[test]
     fn errored_partial() {
         let td = TempDir::new("ignore-test-").unwrap();
+        mkdirp(td.path().join(".git"));
         wfile(td.path().join(".gitignore"), "f**oo\nbar");
 
         let (ig, err) = IgnoreBuilder::new().build().add_child(td.path());
diff --git a/ignore/src/walk.rs b/ignore/src/walk.rs
index b790f2b4..fc36b4e2 100644
--- a/ignore/src/walk.rs
+++ b/ignore/src/walk.rs
@@ -457,7 +457,6 @@ impl DirEntryRaw {
 pub struct WalkBuilder {
     paths: Vec<PathBuf>,
     ig_builder: IgnoreBuilder,
-    parents: bool,
     max_depth: Option<usize>,
     max_filesize: Option<u64>,
     follow_links: bool,
@@ -472,7 +471,6 @@ impl fmt::Debug for WalkBuilder {
         f.debug_struct("WalkBuilder")
             .field("paths", &self.paths)
             .field("ig_builder", &self.ig_builder)
-            .field("parents", &self.parents)
             .field("max_depth", &self.max_depth)
             .field("max_filesize", &self.max_filesize)
             .field("follow_links", &self.follow_links)
@@ -492,7 +490,6 @@ impl WalkBuilder {
         WalkBuilder {
             paths: vec![path.as_ref().to_path_buf()],
             ig_builder: IgnoreBuilder::new(),
-            parents: true,
             max_depth: None,
             max_filesize: None,
             follow_links: false,
@@ -531,7 +528,6 @@ impl WalkBuilder {
             ig_root: ig_root.clone(),
             ig: ig_root.clone(),
             max_filesize: self.max_filesize,
-            parents: self.parents,
         }
     }
 
@@ -547,7 +543,6 @@ impl WalkBuilder {
             max_depth: self.max_depth,
             max_filesize: self.max_filesize,
             follow_links: self.follow_links,
-            parents: self.parents,
             threads: self.threads,
         }
     }
@@ -679,14 +674,12 @@ impl WalkBuilder {
 
     /// Enables reading ignore files from parent directories.
     ///
-    /// If this is enabled, then the parent directories of each file path given
-    /// are traversed for ignore files (subject to the ignore settings on
-    /// this builder). Note that file paths are canonicalized with respect to
-    /// the current working directory in order to determine parent directories.
+    /// If this is enabled, then .gitignore files in parent directories of each
+    /// file path given are respected. Otherwise, they are ignored.
     ///
     /// This is enabled by default.
     pub fn parents(&mut self, yes: bool) -> &mut WalkBuilder {
-        self.parents = yes;
+        self.ig_builder.parents(yes);
         self
     }
 
@@ -765,7 +758,6 @@ pub struct Walk {
     ig_root: Ignore,
     ig: Ignore,
     max_filesize: Option<u64>,
-    parents: bool,
 }
 
 impl Walk {
@@ -812,7 +804,7 @@ impl Iterator for Walk {
                         }
                         Some((path, Some(it))) => {
                             self.it = Some(it);
-                            if self.parents && path_is_dir(&path) {
+                            if path_is_dir(&path) {
                                 let (ig, err) = self.ig_root.add_parents(path);
                                 self.ig = ig;
                                 if let Some(err) = err {
@@ -948,7 +940,6 @@ impl WalkState {
 pub struct WalkParallel {
     paths: vec::IntoIter<PathBuf>,
     ig_root: Ignore,
-    parents: bool,
     max_filesize: Option<u64>,
     max_depth: Option<usize>,
     follow_links: bool,
@@ -1010,7 +1001,6 @@ impl WalkParallel {
                 num_waiting: num_waiting.clone(),
                 num_quitting: num_quitting.clone(),
                 threads: threads,
-                parents: self.parents,
                 max_depth: self.max_depth,
                 max_filesize: self.max_filesize,
                 follow_links: self.follow_links,
@@ -1126,9 +1116,6 @@ struct Worker {
     num_quitting: Arc<AtomicUsize>,
     /// The total number of workers.
     threads: usize,
-    /// Whether to create ignore matchers for parents of caller specified
-    /// directories.
-    parents: bool,
     /// The maximum depth of directories to descend. A value of `0` means no
     /// descension at all.
     max_depth: Option<usize>,
@@ -1156,12 +1143,10 @@ impl Worker {
                 }
                 continue;
             }
-            if self.parents {
-                if let Some(err) = work.add_parents() {
-                    if (self.f)(Err(err)).is_quit() {
-                        self.quit_now();
-                        return;
-                    }
+            if let Some(err) = work.add_parents() {
+                if (self.f)(Err(err)).is_quit() {
+                    self.quit_now();
+                    return;
                 }
             }
             let readdir = match work.read_dir() {
@@ -1645,6 +1630,7 @@ mod tests {
     #[test]
     fn gitignore() {
         let td = TempDir::new("walk-test-").unwrap();
+        mkdirp(td.path().join(".git"));
         mkdirp(td.path().join("a"));
         wfile(td.path().join(".gitignore"), "foo");
         wfile(td.path().join("foo"), "");
@@ -1694,6 +1680,7 @@ mod tests {
     #[test]
     fn gitignore_parent() {
         let td = TempDir::new("walk-test-").unwrap();
+        mkdirp(td.path().join(".git"));
         mkdirp(td.path().join("a"));
         wfile(td.path().join(".gitignore"), "foo");
         wfile(td.path().join("a/foo"), "");