ignore: use named fields in Ignore

The root-specific absolute base now lives alongside the cached matcher
state in `Ignore`. Give both fields names to make their different roles
clearer.

Author: MichaReiser

crates/ignore/src/dir.rs

@@ -91,13 +91,15 @@ struct IgnoreOptions {
 
 /// Ignore is a matcher useful for recursively walking one or more directories.
 #[derive(Clone, Debug)]
-pub(crate) struct Ignore(
-    Arc<IgnoreInner>,
-    // Parent matchers are cached independently of the root they are used
-    // from, but matching them still needs the canonicalized path originally
-    // passed to `add_parents` to rewrite paths relative to that walk root.
-    Option<Arc<PathBuf>>,
-);
+pub(crate) struct Ignore {
+    inner: Arc<IgnoreInner>,
+    // Parent matchers are cached independently of the path being walked, but
+    // matching them still needs the canonicalized path originally passed to
+    // `add_parents`. For example, when walking `/tmp/project/src`, parent
+    // matchers use `/tmp/project/src` to rewrite `foo.py` before matching it
+    // against ignore files from `/tmp/project` and its ancestors.
+    absolute_base: Option<Arc<PathBuf>>,
+}
 
 #[derive(Clone, Debug)]
 struct IgnoreInner {
@@ -157,20 +159,20 @@ impl Ignore {
     /// Return the directory path of this matcher.
     #[cfg(test)]
     pub(crate) fn path(&self) -> &Path {
-        &self.0.dir
+        &self.inner.dir
     }
 
     /// Return true if this matcher has no parent.
     pub(crate) fn is_root(&self) -> bool {
-        self.0.parent.is_none()
+        self.inner.parent.is_none()
     }
 
     /// Return this matcher's parent, if one exists.
     pub(crate) fn parent(&self) -> Option<Ignore> {
-        self.0
-            .parent
-            .as_ref()
-            .map(|parent| Ignore(parent.clone(), self.1.clone()))
+        self.inner.parent.as_ref().map(|parent| Ignore {
+            inner: parent.clone(),
+            absolute_base: self.absolute_base.clone(),
+        })
     }
 
     /// Create a new `Ignore` matcher with the parent directories of `dir`.
@@ -181,10 +183,10 @@ 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 !self.inner.opts.parents
+            && !self.inner.opts.git_ignore
+            && !self.inner.opts.git_exclude
+            && !self.inner.opts.git_global
         {
             // If we never need info from parent directories, then don't do
             // anything.
@@ -213,24 +215,30 @@ impl Ignore {
         let mut errs = PartialErrorBuilder::default();
         let mut ig = self.clone();
         for parent in parents.into_iter().rev() {
-            let mut compiled = self.0.compiled.write().unwrap();
+            let mut compiled = self.inner.compiled.write().unwrap();
             if let Some(weak) = compiled.get(parent.as_os_str()) {
                 if let Some(prebuilt) = weak.upgrade() {
-                    ig = Ignore(prebuilt, Some(absolute_base.clone()));
+                    ig = Ignore {
+                        inner: prebuilt,
+                        absolute_base: Some(absolute_base.clone()),
+                    };
                     continue;
                 }
             }
             let (mut igtmp, err) = ig.add_child_path(parent);
             errs.maybe_push(err);
             igtmp.is_absolute_parent = true;
             igtmp.has_git =
-                if self.0.opts.require_git && self.0.opts.git_ignore {
+                if self.inner.opts.require_git && self.inner.opts.git_ignore {
                     parent.join(".git").exists() || parent.join(".jj").exists()
                 } else {
                     false
                 };
             let ig_arc = Arc::new(igtmp);
-            ig = Ignore(ig_arc.clone(), Some(absolute_base.clone()));
+            ig = Ignore {
+                inner: ig_arc.clone(),
+                absolute_base: Some(absolute_base.clone()),
+            };
             compiled.insert(
                 parent.as_os_str().to_os_string(),
                 Arc::downgrade(&ig_arc),
@@ -252,13 +260,19 @@ impl Ignore {
         dir: P,
     ) -> (Ignore, Option<Error>) {
         let (ig, err) = self.add_child_path(dir.as_ref());
-        (Ignore(Arc::new(ig), self.1.clone()), err)
+        (
+            Ignore {
+                inner: Arc::new(ig),
+                absolute_base: self.absolute_base.clone(),
+            },
+            err,
+        )
     }
 
     /// Like add_child, but takes a full path and returns an IgnoreInner.
     fn add_child_path(&self, dir: &Path) -> (IgnoreInner, Option<Error>) {
-        let check_vcs_dir = self.0.opts.require_git
-            && (self.0.opts.git_ignore || self.0.opts.git_exclude);
+        let check_vcs_dir = self.inner.opts.require_git
+            && (self.inner.opts.git_ignore || self.inner.opts.git_exclude);
         let git_type = if check_vcs_dir {
             dir.join(".git").metadata().ok().map(|md| md.file_type())
         } else {
@@ -268,44 +282,45 @@ impl Ignore {
             check_vcs_dir && (git_type.is_some() || dir.join(".jj").exists());
 
         let mut errs = PartialErrorBuilder::default();
-        let custom_ig_matcher = if self.0.custom_ignore_filenames.is_empty() {
-            Gitignore::empty()
-        } else {
-            let (m, err) = create_gitignore(
-                &dir,
-                &dir,
-                &self.0.custom_ignore_filenames,
-                self.0.opts.ignore_case_insensitive,
-            );
-            errs.maybe_push(err);
-            m
-        };
-        let ig_matcher = if !self.0.opts.ignore {
+        let custom_ig_matcher =
+            if self.inner.custom_ignore_filenames.is_empty() {
+                Gitignore::empty()
+            } else {
+                let (m, err) = create_gitignore(
+                    &dir,
+                    &dir,
+                    &self.inner.custom_ignore_filenames,
+                    self.inner.opts.ignore_case_insensitive,
+                );
+                errs.maybe_push(err);
+                m
+            };
+        let ig_matcher = if !self.inner.opts.ignore {
             Gitignore::empty()
         } else {
             let (m, err) = create_gitignore(
                 &dir,
                 &dir,
                 &[".ignore"],
-                self.0.opts.ignore_case_insensitive,
+                self.inner.opts.ignore_case_insensitive,
             );
             errs.maybe_push(err);
             m
         };
-        let gi_matcher = if !self.0.opts.git_ignore {
+        let gi_matcher = if !self.inner.opts.git_ignore {
             Gitignore::empty()
         } else {
             let (m, err) = create_gitignore(
                 &dir,
                 &dir,
                 &[".gitignore"],
-                self.0.opts.ignore_case_insensitive,
+                self.inner.opts.ignore_case_insensitive,
             );
             errs.maybe_push(err);
             m
         };
 
-        let gi_exclude_matcher = if !self.0.opts.git_exclude {
+        let gi_exclude_matcher = if !self.inner.opts.git_exclude {
             Gitignore::empty()
         } else {
             match resolve_git_commondir(dir, git_type) {
@@ -314,7 +329,7 @@ impl Ignore {
                         &dir,
                         &git_dir,
                         &["info/exclude"],
-                        self.0.opts.ignore_case_insensitive,
+                        self.inner.opts.ignore_case_insensitive,
                     );
                     errs.maybe_push(err);
                     m
@@ -326,35 +341,38 @@ impl Ignore {
             }
         };
         let ig = IgnoreInner {
-            compiled: self.0.compiled.clone(),
+            compiled: self.inner.compiled.clone(),
             dir: dir.to_path_buf(),
-            overrides: self.0.overrides.clone(),
-            types: self.0.types.clone(),
-            parent: Some(self.0.clone()),
+            overrides: self.inner.overrides.clone(),
+            types: self.inner.types.clone(),
+            parent: Some(self.inner.clone()),
             is_absolute_parent: false,
             global_gitignores_relative_to: self
-                .0
+                .inner
                 .global_gitignores_relative_to
                 .clone(),
-            explicit_ignores: self.0.explicit_ignores.clone(),
-            custom_ignore_filenames: self.0.custom_ignore_filenames.clone(),
+            explicit_ignores: self.inner.explicit_ignores.clone(),
+            custom_ignore_filenames: self
+                .inner
+                .custom_ignore_filenames
+                .clone(),
             custom_ignore_matcher: custom_ig_matcher,
             ignore_matcher: ig_matcher,
-            git_global_matcher: self.0.git_global_matcher.clone(),
+            git_global_matcher: self.inner.git_global_matcher.clone(),
             git_ignore_matcher: gi_matcher,
             git_exclude_matcher: gi_exclude_matcher,
             has_git,
-            opts: self.0.opts,
+            opts: self.inner.opts,
         };
         (ig, errs.into_error_option())
     }
 
     /// Returns true if at least one type of ignore rule should be matched.
     fn has_any_ignore_rules(&self) -> bool {
-        let opts = self.0.opts;
+        let opts = self.inner.opts;
         let has_custom_ignore_files =
-            !self.0.custom_ignore_filenames.is_empty();
-        let has_explicit_ignores = !self.0.explicit_ignores.is_empty();
+            !self.inner.custom_ignore_filenames.is_empty();
+        let has_explicit_ignores = !self.inner.explicit_ignores.is_empty();
 
         opts.ignore
             || opts.git_global
@@ -370,7 +388,7 @@ impl Ignore {
         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) {
+        if m.is_none() && self.inner.opts.hidden && is_hidden(dent) {
             return Match::Ignore(IgnoreMatch::hidden());
         }
         m
@@ -395,9 +413,9 @@ impl Ignore {
         // regardless of whether it's whitelist/ignore, then we quit and
         // return that result immediately. Overrides have the highest
         // precedence.
-        if !self.0.overrides.is_empty() {
+        if !self.inner.overrides.is_empty() {
             let mat = self
-                .0
+                .inner
                 .overrides
                 .matched(path, is_dir)
                 .map(IgnoreMatch::overrides);
@@ -414,9 +432,9 @@ impl Ignore {
                 whitelisted = mat;
             }
         }
-        if !self.0.types.is_empty() {
+        if !self.inner.types.is_empty() {
             let mat =
-                self.0.types.matched(path, is_dir).map(IgnoreMatch::types);
+                self.inner.types.matched(path, is_dir).map(IgnoreMatch::types);
             if mat.is_ignore() {
                 return mat;
             } else if mat.is_whitelist() {
@@ -440,8 +458,8 @@ impl Ignore {
             mut m_gi_exclude,
             mut m_explicit,
         ) = (Match::None, Match::None, Match::None, Match::None, Match::None);
-        let any_git =
-            !self.0.opts.require_git || self.parents().any(|ig| ig.0.has_git);
+        let any_git = !self.inner.opts.require_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() {
@@ -470,7 +488,7 @@ impl Ignore {
             }
             saw_git = saw_git || ig.0.has_git;
         }
-        if self.0.opts.parents {
+        if self.inner.opts.parents {
             if let Some(abs_parent_path) = self.absolute_base() {
                 // What we want to do here is take the absolute base path of
                 // this directory and join it with the path we're searching.
@@ -532,14 +550,14 @@ impl Ignore {
                 }
             }
         }
-        for gi in self.0.explicit_ignores.iter().rev() {
+        for gi in self.inner.explicit_ignores.iter().rev() {
             if !m_explicit.is_none() {
                 break;
             }
             m_explicit = gi.matched(&path, is_dir).map(IgnoreMatch::gitignore);
         }
         let m_global = if any_git {
-            self.0
+            self.inner
                 .git_global_matcher
                 .matched(&path, is_dir)
                 .map(IgnoreMatch::gitignore)
@@ -557,17 +575,16 @@ impl Ignore {
 
     /// Returns an iterator over parent ignore matchers, including this one.
     pub(crate) fn parents(&self) -> Parents<'_> {
-        Parents(Some(IgnoreRef(&self.0)))
+        Parents(Some(IgnoreRef(&self.inner)))
     }
 
     /// Returns the first absolute path of the first absolute parent, if
     /// one exists.
     fn absolute_base(&self) -> Option<&Path> {
-        self.1.as_ref().map(|p| &***p)
+        self.absolute_base.as_ref().map(|p| &***p)
     }
 }
 
-/// An iterator over all parents of an ignore matcher, including itself.
 #[derive(Clone, Copy)]
 pub(crate) struct IgnoreRef<'a>(&'a IgnoreInner);
 
@@ -581,6 +598,7 @@ impl IgnoreRef<'_> {
     }
 }
 
+/// An iterator over all parents of an ignore matcher, including itself.
 pub(crate) struct Parents<'a>(Option<IgnoreRef<'a>>);
 
 impl<'a> Iterator for Parents<'a> {
@@ -685,8 +703,8 @@ impl IgnoreBuilder {
             Gitignore::empty()
         };
 
-        Ignore(
-            Arc::new(IgnoreInner {
+        Ignore {
+            inner: Arc::new(IgnoreInner {
                 compiled: Arc::new(RwLock::new(HashMap::new())),
                 dir: self.dir.clone(),
                 overrides: self.overrides.clone(),
@@ -706,8 +724,8 @@ impl IgnoreBuilder {
                 has_git: false,
                 opts: self.opts,
             }),
-            None,
-        )
+            absolute_base: None,
+        }
     }
 
     /// Set the current directory used for matching global gitignores.
@@ -1289,7 +1307,7 @@ mod tests {
         let (tests, err) = tests_parents.add_child(td.path().join("tests"));
         assert!(err.is_none());
 
-        assert!(Arc::ptr_eq(&src_parents.0, &tests_parents.0));
+        assert!(Arc::ptr_eq(&src_parents.inner, &tests_parents.inner));
         assert!(src.matched("build", true).is_none());
         assert!(tests.matched("build", true).is_ignore());
     }