workingcopy: fix lock to be more Rusty

Summary:
The Rust repo locks were a direct translation of Python:

```
let _wlock = wc.lock()?;

// ... do stuff with lock
```

That led to the common mistake:

```
// Oops - didn't check error.
let _wlock = wc.lock();
```

Let's change to a more Rusty flow where locking yields an object explicitly. In our case, (WorkingCopy).lock() now yields a LockedWorkingCopy, which has some must-be-locked methods moved to it.

Reviewed By: quark-zju

Differential Revision: D52055451

fbshipit-source-id: a41f58bcdd34ded6feeb42623e593079c0b683c0

eden/scm/lib/checkout/src/edenfs.rs

@@ -26,14 +26,15 @@ use treestate::filestate::StateFlags;
 use types::HgId;
 use types::RepoPath;
 use workingcopy::util::walk_treestate;
+use workingcopy::workingcopy::LockedWorkingCopy;
 use workingcopy::workingcopy::WorkingCopy;
 
 use crate::errors::EdenConflictError;
 
 pub fn edenfs_checkout(
     io: &IO,
     repo: &mut Repo,
-    wc: &mut WorkingCopy,
+    wc: &LockedWorkingCopy,
     target_commit: HgId,
     checkout_mode: edenfs_client::CheckoutMode,
 ) -> anyhow::Result<()> {
@@ -55,7 +56,7 @@ pub fn edenfs_checkout(
     Ok(())
 }
 
-fn clear_edenfs_dirstate(wc: &mut WorkingCopy) -> anyhow::Result<()> {
+fn clear_edenfs_dirstate(wc: &LockedWorkingCopy) -> anyhow::Result<()> {
     let tbind = wc.treestate();
     let mut treestate = tbind.lock();
     let matcher = Arc::new(AlwaysMatcher::new());

eden/scm/lib/checkout/src/lib.rs

@@ -58,7 +58,7 @@ use types::RepoPathBuf;
 use vfs::UpdateFlag;
 use vfs::VFS;
 use workingcopy::sparse;
-use workingcopy::workingcopy::WorkingCopy;
+use workingcopy::workingcopy::LockedWorkingCopy;
 
 #[allow(dead_code)]
 mod actions;
@@ -837,7 +837,7 @@ fn truncate_u64(f: &str, path: &RepoPath, v: u64) -> i32 {
 pub fn checkout(
     io: &IO,
     repo: &mut Repo,
-    wc: &mut WorkingCopy,
+    wc: &LockedWorkingCopy,
     target_commit: HgId,
 ) -> Result<Option<(usize, usize)>> {
     #[cfg(feature = "eden")]
@@ -858,7 +858,7 @@ pub fn checkout(
 pub fn sparse_checkout(
     io: &IO,
     repo: &mut Repo,
-    wc: &mut WorkingCopy,
+    wc: &LockedWorkingCopy,
     target_commit: HgId,
 ) -> Result<(usize, usize)> {
     wc.ensure_locked()?;

eden/scm/lib/hgcommands/src/commands/debug/mergestate.rs

@@ -29,7 +29,7 @@ pub fn run(ctx: ReqCtx<DebugMergeStateOpts>, _repo: &mut Repo, wc: &mut WorkingC
     if ctx.opts.add_unsupported_mandatory_record || ctx.opts.add_unsupported_advisory_record {
         ensure!(std::env::var_os("TESTTMP").is_some(), "only for tests");
 
-        let _wlock = wc.lock()?;
+        let wc = wc.lock()?;
 
         let mut ms = wc.read_merge_state()?.unwrap_or_default();
         if ctx.opts.add_unsupported_mandatory_record {

eden/scm/lib/hgcommands/src/commands/goto.rs

@@ -18,6 +18,7 @@ use configmodel::ConfigExt;
 use fs_err as fs;
 use repo::repo::Repo;
 use repostate::command_state::Operation;
+use workingcopy::workingcopy::LockedWorkingCopy;
 use workingcopy::workingcopy::WorkingCopy;
 
 use super::MergeToolOpts;
@@ -102,12 +103,12 @@ pub fn run(ctx: ReqCtx<GotoOpts>, repo: &mut Repo, wc: &mut WorkingCopy) -> Resu
     }
 
     // Protect the various ".hg" state file checks.
-    let wc_lock = wc.lock()?;
+    let wc = wc.lock()?;
 
     // Clean up the "updatemergestate" file if we are done merging.
     // We do this before try_operation since that will error on "updatemergestate".
     // This should happen even without "--continue".
-    let cleaned_mergestate = maybe_clear_update_merge_state(wc, ctx.opts.clean)?;
+    let cleaned_mergestate = maybe_clear_update_merge_state(&wc, ctx.opts.clean)?;
 
     let updatestate_path = wc.dot_hg_path().join("updatestate");
 
@@ -139,7 +140,7 @@ pub fn run(ctx: ReqCtx<GotoOpts>, repo: &mut Repo, wc: &mut WorkingCopy) -> Resu
 
     // This aborts if there are unresolved conflicts, or have some other
     // operation (e.g. "graft") in progress.
-    repostate::command_state::try_operation(&wc_lock, op)?;
+    repostate::command_state::try_operation(wc.locked_dot_hg_path(), op)?;
 
     if dest.len() > 1 {
         abort!(
@@ -166,7 +167,7 @@ pub fn run(ctx: ReqCtx<GotoOpts>, repo: &mut Repo, wc: &mut WorkingCopy) -> Resu
     tracing::debug!(target: "checkout_info", checkout_mode="rust");
 
     let _lock = repo.lock()?;
-    let update_result = checkout::checkout(ctx.io(), repo, wc, target)?;
+    let update_result = checkout::checkout(ctx.io(), repo, &wc, target)?;
 
     if !ctx.global_opts().quiet {
         if let Some((updated, removed)) = update_result {
@@ -184,7 +185,7 @@ pub fn run(ctx: ReqCtx<GotoOpts>, repo: &mut Repo, wc: &mut WorkingCopy) -> Resu
 
 // Clear use out of the "updatemergestate" state if there are no unresolved
 // files or user specified "--clean". Returns whether state was cleared.
-fn maybe_clear_update_merge_state(wc: &WorkingCopy, clean: bool) -> Result<bool> {
+fn maybe_clear_update_merge_state(wc: &LockedWorkingCopy, clean: bool) -> Result<bool> {
     let ums_path = wc.dot_hg_path().join("updatemergestate");
 
     if !ums_path.try_exists().context("updatemergestate")? {

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

@@ -32,6 +32,7 @@ use pathmatcher::IntersectMatcher;
 use pathmatcher::Matcher;
 use pathmatcher::NegateMatcher;
 use pathmatcher::UnionMatcher;
+use repolock::LockedPath;
 use repolock::RepoLocker;
 use repostate::MergeState;
 use status::FileStatus;
@@ -149,8 +150,12 @@ impl WorkingCopy {
         &self.dot_hg_path
     }
 
-    pub fn lock(&self) -> Result<repolock::LockedPath, repolock::LockError> {
-        self.locker.lock_working_copy(self.dot_hg_path.clone())
+    pub fn lock(&self) -> Result<LockedWorkingCopy, repolock::LockError> {
+        let locked_path = self.locker.lock_working_copy(self.dot_hg_path.clone())?;
+        Ok(LockedWorkingCopy {
+            dot_hg_path: locked_path,
+            wc: self,
+        })
     }
 
     pub fn ensure_locked(&self) -> Result<(), repolock::LockError> {
@@ -169,20 +174,6 @@ impl WorkingCopy {
         self.treestate.lock().parents().collect()
     }
 
-    pub fn set_parents(
-        &mut self,
-        parents: Vec<HgId>,
-        parent_tree_hash: Option<HgId>,
-    ) -> Result<()> {
-        let p1 = parents
-            .get(0)
-            .context("At least one parent is required for setting parents")?
-            .clone();
-        let p2 = parents.get(1).copied();
-        self.treestate.lock().set_parents(&mut parents.iter())?;
-        self.filesystem.lock().set_parents(p1, p2, parent_tree_hash)
-    }
-
     pub fn filestore(&self) -> ArcFileStore {
         self.filestore.clone()
     }
@@ -514,15 +505,42 @@ impl WorkingCopy {
 
         MergeState::read(&self.dot_hg_path().join("merge/state2"))
     }
+}
 
-    pub fn write_merge_state(&self, ms: &MergeState) -> Result<()> {
-        self.ensure_locked()?;
+pub struct LockedWorkingCopy<'a> {
+    dot_hg_path: LockedPath,
+    wc: &'a WorkingCopy,
+}
+
+impl<'a> std::ops::Deref for LockedWorkingCopy<'a> {
+    type Target = WorkingCopy;
+
+    fn deref(&self) -> &Self::Target {
+        self.wc
+    }
+}
+
+impl<'a> LockedWorkingCopy<'a> {
+    pub fn locked_dot_hg_path(&self) -> &LockedPath {
+        &self.dot_hg_path
+    }
 
-        let dir = self.dot_hg_path().join("merge");
+    pub fn write_merge_state(&self, ms: &MergeState) -> Result<()> {
+        let dir = self.dot_hg_path.join("merge");
         fs_err::create_dir_all(&dir)?;
         let mut f = util::file::atomic_open(&dir.join("state2"))?;
         ms.serialize(f.as_file())?;
         f.save()?;
         Ok(())
     }
+
+    pub fn set_parents(&self, parents: Vec<HgId>, parent_tree_hash: Option<HgId>) -> Result<()> {
+        let p1 = parents
+            .get(0)
+            .context("At least one parent is required for setting parents")?
+            .clone();
+        let p2 = parents.get(1).copied();
+        self.treestate.lock().set_parents(&mut parents.iter())?;
+        self.filesystem.lock().set_parents(p1, p2, parent_tree_hash)
+    }
 }

eden/scm/saplingnative/bindings/modules/pyworkingcopy/src/lib.rs

@@ -215,7 +215,7 @@ py_class!(pub class workingcopy |py| {
 
     def writemergestate(&self, ms: mergestate) -> PyResult<PyNone> {
         let ms = ms.extract_inner_ref(py);
-        self.inner(py).read().write_merge_state(&*ms.borrow()).map_pyerr(py)?;
+        self.inner(py).read().lock().map_pyerr(py)?.write_merge_state(&*ms.borrow()).map_pyerr(py)?;
         Ok(PyNone)
     }
 
@@ -226,9 +226,9 @@ py_class!(pub class workingcopy |py| {
     // as "commit").
     def commandstate(&self, op: Option<Serde<Operation>> = None) -> PyResult<Option<(String, String)>> {
         let wc = self.inner(py).read();
-        let locked_path = wc.lock().map_pyerr(py)?;
+        let wc = wc.lock().map_pyerr(py)?;
         let op = op.map_or(Operation::Other, |op| *op);
-        match repostate::command_state::try_operation(&locked_path, op) {
+        match repostate::command_state::try_operation(wc.locked_dot_hg_path(), op) {
             Ok(()) => Ok(None),
             Err(err) => {
                 if let Some(conflict) = err.downcast_ref::<repostate::command_state::Conflict>() {