status: prevent accidentally overwriting dirstate during native status

Summary:
There was a bug where hg status would take the lock, then write the
treestate and dirstate without checking to see if another process had written
the dirstate/treestate since the current process had read it. This would cause
files to show up as dirty when they weren't, and potentially result in invalid
store id errors.

For example:

```
Process 1: Starts an hg checkout and takes the wlock.
Process 2: Starts hg status and begins updating the in-memory treestate
Process 1: Completes the checkout and writes a new dirstate/treestate showing
all the newly clean files. It releases the wlock.
Process 2: Takes the wlock and writes the watchman file updates it saw during
status. This overwrites process 1's written state.

Future hg status runs now see a bunch of empty files because it's using the
pre-checkout treestate to compare against the post-checkout working copy.
```

The new code prevents the status process from writing the update (since it's
mostly just an optimization anyway) if the dirstate changed out from under it.

Reviewed By: quark-zju

Differential Revision: D41052635

fbshipit-source-id: aa5459eab5b4fe5e86e037e7ffa364149cb686a8

Author: Durham Goode

eden/scm/lib/treestate/src/dirstate.rs

@@ -19,6 +19,7 @@ use types::HgId;
 use crate::serialization::Serializable;
 use crate::store::BlockId;
 use crate::treestate::TreeState;
+use crate::ErrorKind;
 
 /// A dirstate object. This maintains .hg/dirstate file
 #[derive(Debug, PartialEq)]
@@ -49,6 +50,23 @@ pub fn flush(config: &dyn Config, root: &Path, treestate: &mut TreeState) -> Res
         let dirstate_input = util::file::read(&dirstate_path)?;
         let mut dirstate = Dirstate::deserialize(&mut dirstate_input.as_slice())?;
 
+        // If the dirstate has changed since we last loaded it, don't flush since we might
+        // overwrite data. For instance, if we start running 'hg status', it loads the dirstate and
+        // treestate and starts updating the treestate.  Before status gets to this flush, if
+        // another process, like 'hg checkout' writes to the dirstate/treestate, then if we let
+        // this 'hg status' flush it's old data, we'd result in a dirty working copy where the
+        // clean checkout data was thought to be dirty because we had old treestate data.
+        //
+        // In that situation, just return an error and the client can decide if that's ok or not.
+
+        if let Some(dirstate_fields) = &dirstate.tree_state {
+            if treestate.file_name()? != dirstate_fields.tree_filename
+                || treestate.original_root_id() != dirstate_fields.tree_root_id
+            {
+                return Err(ErrorKind::TreestateOutOfDate.into());
+            }
+        }
+
         dirstate.p1 = treestate
             .get_metadata_by_key("p1")?
             .map_or(Ok(NULL_ID), |p| HgId::from_hex(p.as_bytes()))?;
@@ -62,6 +80,7 @@ pub fn flush(config: &dyn Config, root: &Path, treestate: &mut TreeState) -> Res
         })?;
 
         let root_id = treestate.flush()?;
+        treestate_fields.tree_filename = treestate.file_name()?;
         treestate_fields.tree_root_id = root_id;
 
         let mut dirstate_output: Vec<u8> = Vec::new();

eden/scm/lib/treestate/src/errors.rs

@@ -11,7 +11,7 @@ use std::path::PathBuf;
 
 use thiserror::Error;
 
-#[derive(Debug, Error)]
+#[derive(Debug, Error, PartialEq)]
 pub enum ErrorKind {
     #[error("the provided store file is not a valid store file: {0}")]
     NotAStoreFile(PathBuf),
@@ -27,4 +27,6 @@ pub enum ErrorKind {
     CorruptTree,
     #[error("callback error: {0}")]
     CallbackError(String),
+    #[error("dirstate/treestate was out of date and therefore did not flush")]
+    TreestateOutOfDate,
 }

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

@@ -46,6 +46,7 @@ pub struct TreeState {
     store: FileStore,
     tree: Tree<FileStateV2>,
     root: TreeStateRoot,
+    original_root_id: BlockId,
     // eden_dirstate_path is only used in the case the case that the treestate is
     // wrapping a legacy eden dirstate which is necessary for EdenFS compatility.
     // TODO: Remove once EdenFS has migrated to treestate.
@@ -72,6 +73,7 @@ impl TreeState {
             store,
             tree,
             root,
+            original_root_id: root_id,
             eden_dirstate_path: None,
             case_sensitive,
         })
@@ -88,6 +90,7 @@ impl TreeState {
             store,
             tree,
             root,
+            original_root_id: BlockId(0),
             eden_dirstate_path: None,
             case_sensitive,
         };
@@ -119,6 +122,7 @@ impl TreeState {
             store,
             tree,
             root,
+            original_root_id: BlockId(0),
             eden_dirstate_path: Some(path),
             case_sensitive,
         };
@@ -144,6 +148,11 @@ impl TreeState {
             .to_string())
     }
 
+    /// The root_id from when the treestate was loaded or last saved. Gets updated upon flush.
+    pub fn original_root_id(&self) -> BlockId {
+        self.original_root_id
+    }
+
     pub fn dirty(&self) -> bool {
         self.tree.dirty() || self.root.dirty()
     }
@@ -183,6 +192,7 @@ impl TreeState {
             write_eden_dirstate(&eden_dirstate_path, metadata, entries)?;
         }
 
+        self.original_root_id = result;
         Ok(result)
     }
 

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

@@ -18,6 +18,7 @@ use treestate::filestate::StateFlags;
 use treestate::metadata::Metadata;
 use treestate::serialization::Serializable;
 use treestate::treestate::TreeState;
+use treestate::ErrorKind;
 use types::RepoPathBuf;
 use watchman_client::prelude::*;
 
@@ -115,7 +116,15 @@ impl WatchmanTreeStateWrite for WatchmanTreeState<'_> {
     }
 
     fn flush(self, config: &dyn Config) -> Result<()> {
-        dirstate::flush(config, &self.root, &mut self.treestate.lock())
+        match dirstate::flush(config, &self.root, &mut self.treestate.lock()) {
+            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),
+            },
+        }
     }
 }