workingcopy: get rid of treestate abstraction

Summary: These traits were previously used to abstract the treetate in tests, but the treestate is easy enough to work with. I think it's much simpler overall to use the concrete TreeState in code and tests (more tests to follow).

Reviewed By: zzl0

Differential Revision: D43501754

fbshipit-source-id: 6b106d0d814afbaba14b58e5f2f2fd23c519eeb6

eden/scm/lib/workingcopy/src/watchmanfs/treestate.rs

@@ -10,7 +10,6 @@ use std::sync::Arc;
 
 use anyhow::anyhow;
 use anyhow::Result;
-use parking_lot::Mutex;
 use pathmatcher::Matcher;
 use repolock::RepoLocker;
 use treestate::dirstate;
@@ -26,147 +25,110 @@ use watchman_client::prelude::*;
 
 use crate::util::walk_treestate;
 
-pub trait WatchmanTreeStateWrite {
-    fn mark_needs_check(&mut self, path: &RepoPathBuf) -> Result<bool>;
-
-    fn clear_needs_check(&mut self, path: &RepoPathBuf) -> Result<bool>;
-
-    fn set_clock(&mut self, clock: Clock) -> Result<()>;
-
-    fn flush(self, locker: &RepoLocker) -> Result<()>;
-}
-
-pub trait WatchmanTreeStateRead {
-    fn list_needs_check(
-        &mut self,
-        matcher: Arc<dyn Matcher + Send + Sync + 'static>,
-    ) -> Result<(Vec<RepoPathBuf>, Vec<ParseError>)>;
-
-    fn get_clock(&self) -> Result<Option<Clock>>;
-}
-
-#[derive(Clone)]
-pub struct WatchmanTreeState<'a> {
-    pub treestate: Arc<Mutex<TreeState>>,
-    pub root: &'a Path,
-}
-
-impl WatchmanTreeStateWrite for WatchmanTreeState<'_> {
-    fn mark_needs_check(&mut self, path: &RepoPathBuf) -> Result<bool> {
-        let mut treestate = self.treestate.lock();
-
-        let state = treestate.get(path)?;
-        let filestate = match state {
-            Some(filestate) => {
-                let filestate = filestate.clone();
-                if filestate.state.intersects(StateFlags::NEED_CHECK) {
-                    // It's already marked need_check, so return early so we don't mutate the
-                    // treestate.
-                    return Ok(false);
-                }
-                FileStateV2 {
-                    state: filestate.state | StateFlags::NEED_CHECK,
-                    ..filestate
-                }
-            }
-            // The file is currently untracked
-            None => FileStateV2 {
-                state: StateFlags::NEED_CHECK,
-                mode: 0o666,
-                size: -1,
-                mtime: -1,
-                copied: None,
-            },
-        };
-        treestate.insert(path, &filestate)?;
-        Ok(true)
-    }
-
-    fn clear_needs_check(&mut self, path: &RepoPathBuf) -> Result<bool> {
-        let mut treestate = self.treestate.lock();
-
-        let state = treestate.get(path)?;
-        if let Some(filestate) = state {
+pub fn mark_needs_check(ts: &mut TreeState, path: &RepoPathBuf) -> Result<bool> {
+    let state = ts.get(path)?;
+    let filestate = match state {
+        Some(filestate) => {
             let filestate = filestate.clone();
-            if !filestate.state.intersects(StateFlags::NEED_CHECK) {
-                // It's already clear.
+            if filestate.state.intersects(StateFlags::NEED_CHECK) {
+                // It's already marked need_check, so return early so we don't mutate the
+                // treestate.
                 return Ok(false);
             }
-            let filestate = FileStateV2 {
-                state: filestate.state & !StateFlags::NEED_CHECK,
+            FileStateV2 {
+                state: filestate.state | StateFlags::NEED_CHECK,
                 ..filestate
-            };
-            treestate.insert(path, &filestate)?;
-            return Ok(true);
+            }
         }
-        Ok(false)
-    }
+        // The file is currently untracked
+        None => FileStateV2 {
+            state: StateFlags::NEED_CHECK,
+            mode: 0o666,
+            size: -1,
+            mtime: -1,
+            copied: None,
+        },
+    };
+    ts.insert(path, &filestate)?;
+    Ok(true)
+}
 
-    fn set_clock(&mut self, clock: Clock) -> Result<()> {
-        let mut treestate = self.treestate.lock();
-
-        let clock_string = match clock {
-            Clock::Spec(ClockSpec::StringClock(string)) => Ok(string),
-            clock => Err(anyhow!(
-                "Watchman implementation only handles opaque string type. Got the following clock instead: {:?}",
-                clock
-            )),
-        }?;
-
-        let mut metadata_buf = treestate.get_metadata();
-        let mut metadata = Metadata::deserialize(&mut metadata_buf)?;
-        metadata.0.insert("clock".to_string(), clock_string);
-        let mut metadata_buf = vec![];
-        metadata.serialize(&mut metadata_buf)?;
-        treestate.set_metadata(&metadata_buf);
-
-        Ok(())
-    }
-
-    fn flush(self, locker: &RepoLocker) -> Result<()> {
-        match dirstate::flush(&self.root, &mut self.treestate.lock(), locker) {
-            Ok(()) => Ok(()),
-            // If the dirstate was changed before we flushed, that's ok. Let the other write win
-            // since writes during status are just optimizations.
-            Err(e) => match e.downcast_ref::<ErrorKind>() {
-                Some(e) if *e == ErrorKind::TreestateOutOfDate => Ok(()),
-                _ => Err(e),
-            },
+pub fn clear_needs_check(ts: &mut TreeState, path: &RepoPathBuf) -> Result<bool> {
+    let state = ts.get(path)?;
+    if let Some(filestate) = state {
+        let filestate = filestate.clone();
+        if !filestate.state.intersects(StateFlags::NEED_CHECK) {
+            // It's already clear.
+            return Ok(false);
         }
+        let filestate = FileStateV2 {
+            state: filestate.state & !StateFlags::NEED_CHECK,
+            ..filestate
+        };
+        ts.insert(path, &filestate)?;
+        return Ok(true);
+    }
+    Ok(false)
+}
+
+pub fn set_clock(ts: &mut TreeState, clock: Clock) -> Result<()> {
+    let clock_string = match clock {
+        Clock::Spec(ClockSpec::StringClock(string)) => Ok(string),
+        clock => Err(anyhow!(
+            "Watchman implementation only handles opaque string type. Got the following clock instead: {:?}",
+            clock
+        )),
+    }?;
+
+    let mut metadata_buf = ts.get_metadata();
+    let mut metadata = Metadata::deserialize(&mut metadata_buf)?;
+    metadata.0.insert("clock".to_string(), clock_string);
+    let mut metadata_buf = vec![];
+    metadata.serialize(&mut metadata_buf)?;
+    ts.set_metadata(&metadata_buf);
+
+    Ok(())
+}
+
+pub fn flush(root: &Path, ts: &mut TreeState, locker: &RepoLocker) -> Result<()> {
+    match dirstate::flush(root, ts, locker) {
+        Ok(()) => Ok(()),
+        // If the dirstate was changed before we flushed, that's ok. Let the other write win
+        // since writes during status are just optimizations.
+        Err(e) => match e.downcast_ref::<ErrorKind>() {
+            Some(e) if *e == ErrorKind::TreestateOutOfDate => Ok(()),
+            _ => Err(e),
+        },
     }
 }
 
-impl WatchmanTreeStateRead for WatchmanTreeState<'_> {
-    fn list_needs_check(
-        &mut self,
-        matcher: Arc<dyn Matcher + Send + Sync + 'static>,
-    ) -> Result<(Vec<RepoPathBuf>, Vec<ParseError>)> {
-        let mut needs_check = Vec::new();
+pub fn list_needs_check(
+    ts: &mut TreeState,
+    matcher: Arc<dyn Matcher + Send + Sync + 'static>,
+) -> Result<(Vec<RepoPathBuf>, Vec<ParseError>)> {
+    let mut needs_check = Vec::new();
 
-        let parse_errs = walk_treestate(
-            &mut self.treestate.lock(),
-            matcher,
-            StateFlags::NEED_CHECK,
-            StateFlags::empty(),
-            |path, _state| {
-                needs_check.push(path);
-                Ok(())
-            },
-        )?;
+    let parse_errs = walk_treestate(
+        ts,
+        matcher,
+        StateFlags::NEED_CHECK,
+        StateFlags::empty(),
+        |path, _state| {
+            needs_check.push(path);
+            Ok(())
+        },
+    )?;
 
-        Ok((needs_check, parse_errs))
-    }
+    Ok((needs_check, parse_errs))
+}
 
-    fn get_clock(&self) -> Result<Option<Clock>> {
-        let treestate = self.treestate.lock();
-
-        let mut metadata_buf = treestate.get_metadata();
-        let metadata = Metadata::deserialize(&mut metadata_buf)?;
-        Ok(metadata
-            .0
-            .get(&"clock".to_string())
-            .map(|clock| Clock::Spec(ClockSpec::StringClock(clock.clone()))))
-    }
+pub fn get_clock(ts: &mut TreeState) -> Result<Option<Clock>> {
+    let mut metadata_buf = ts.get_metadata();
+    let metadata = Metadata::deserialize(&mut metadata_buf)?;
+    Ok(metadata
+        .0
+        .get(&"clock".to_string())
+        .map(|clock| Clock::Spec(ClockSpec::StringClock(clock.clone()))))
 }
 
 #[cfg(test)]
@@ -199,12 +161,7 @@ mod tests {
             false,
         ));
 
-        let mut wm_ts = WatchmanTreeState {
-            treestate: Arc::new(Mutex::new(ts)),
-            root: "/dev/null".as_ref(),
-        };
-
-        let (needs_check, _) = wm_ts.list_needs_check(matcher)?;
+        let (needs_check, _) = list_needs_check(&mut ts, matcher)?;
         assert_eq!(
             needs_check,
             vec![RepoPathBuf::from_string("include_me".to_string())?],

eden/scm/lib/workingcopy/src/watchmanfs/watchmanfs.rs

@@ -7,6 +7,7 @@
 
 use std::collections::HashSet;
 use std::io::Write;
+use std::path::Path;
 use std::path::PathBuf;
 use std::sync::Arc;
 use std::time::Duration;
@@ -29,8 +30,10 @@ use types::RepoPathBuf;
 use vfs::VFS;
 use watchman_client::prelude::*;
 
-use super::treestate::WatchmanTreeState;
-use super::treestate::WatchmanTreeStateWrite;
+use super::treestate::clear_needs_check;
+use super::treestate::flush;
+use super::treestate::mark_needs_check;
+use super::treestate::set_clock;
 use crate::filechangedetector::ArcReadFileContents;
 use crate::filechangedetector::FileChangeDetector;
 use crate::filechangedetector::FileChangeDetectorTrait;
@@ -38,7 +41,8 @@ use crate::filechangedetector::FileChangeResult;
 use crate::filechangedetector::ResolvedFileChangeResult;
 use crate::filesystem::PendingChangeResult;
 use crate::filesystem::PendingChanges;
-use crate::watchmanfs::treestate::WatchmanTreeStateRead;
+use crate::watchmanfs::treestate::get_clock;
+use crate::watchmanfs::treestate::list_needs_check;
 use crate::workingcopy::WorkingCopy;
 
 type ArcReadTreeManifest = Arc<dyn ReadTreeManifest + Send + Sync>;
@@ -124,12 +128,9 @@ impl PendingChanges for WatchmanFileSystem {
         config: &dyn Config,
         io: &IO,
     ) -> Result<Box<dyn Iterator<Item = Result<PendingChangeResult>>>> {
-        let mut watchman_ts = WatchmanTreeState {
-            treestate: self.treestate.clone(),
-            root: self.vfs.root(),
-        };
+        let ts = &mut *self.treestate.lock();
 
-        let prev_clock = watchman_ts.get_clock()?;
+        let prev_clock = get_clock(ts)?;
 
         let result = async_runtime::block_on(self.query_result(WatchmanConfig {
             clock: prev_clock.clone(),
@@ -160,8 +161,7 @@ impl PendingChanges for WatchmanFileSystem {
                 .as_ref()
                 .map_or(false, |f| f.len() > file_change_threshold);
 
-        let manifests =
-            WorkingCopy::current_manifests(&self.treestate.lock(), &self.tree_resolver)?;
+        let manifests = WorkingCopy::current_manifests(ts, &self.tree_resolver)?;
 
         let file_change_detector = FileChangeDetector::new(
             self.vfs.clone(),
@@ -170,7 +170,7 @@ impl PendingChanges for WatchmanFileSystem {
             self.store.clone(),
         );
 
-        let (ts_needs_check, ts_errors) = watchman_ts.list_needs_check(matcher)?;
+        let (ts_needs_check, ts_errors) = list_needs_check(ts, matcher)?;
 
         let mut wm_errors: Vec<ParseError> = Vec::new();
         let wm_needs_check: Vec<RepoPathBuf> = result
@@ -190,7 +190,7 @@ impl PendingChanges for WatchmanFileSystem {
 
         let mut pending_changes = detect_changes(
             file_change_detector,
-            &mut self.treestate.lock(),
+            ts,
             ts_needs_check,
             wm_needs_check,
             result.clock,
@@ -205,7 +205,7 @@ impl PendingChanges for WatchmanFileSystem {
                 .map(|e| Err(anyhow!(e))),
         );
 
-        pending_changes.persist(watchman_ts, should_update_clock, &self.locker)?;
+        pending_changes.persist(self.vfs.root(), ts, should_update_clock, &self.locker)?;
 
         Ok(Box::new(pending_changes.into_iter()))
     }
@@ -317,13 +317,14 @@ impl WatchmanPendingChanges {
     #[tracing::instrument(skip_all)]
     pub fn persist(
         &mut self,
-        mut treestate: impl WatchmanTreeStateWrite,
+        root: &Path,
+        ts: &mut TreeState,
         should_update_clock: bool,
         locker: &RepoLocker,
     ) -> Result<()> {
         let mut wrote = false;
         for path in self.needs_clear.iter() {
-            match treestate.clear_needs_check(path) {
+            match clear_needs_check(ts, path) {
                 Ok(v) => wrote |= v,
                 Err(e) =>
                 // We can still build a valid result if we fail to clear the
@@ -336,16 +337,16 @@ impl WatchmanPendingChanges {
         }
 
         for path in self.needs_mark.iter() {
-            wrote |= treestate.mark_needs_check(path)?;
+            wrote |= mark_needs_check(ts, path)?;
         }
 
         // If the treestate is already dirty, we're going to write it anyway, so let's go ahead and
         // update the clock while we're at it.
         if should_update_clock || wrote {
-            treestate.set_clock(self.clock.clone())?;
+            set_clock(ts, self.clock.clone())?;
         }
 
-        treestate.flush(locker)
+        flush(root, ts, locker)
     }
 }