revisionstore: multiplex stores should return a path on flush

Summary:
On repack, when the Rust stores are in use, the repack code relies on
ContentStore::commit_pending to return the path of a newly created packfile, so
it won't delete it when going over the repacked ones. When LFS is enabled, both
the shared and the local stores are behind the LfsMultiplexer store that
unfortunately would always return `Ok(None)`. In this situation, the repack
code would delete all the repacked packfiles, which usually is the expected
behvior, unless only one packfile is being repacked, in which case the repack
code effectively re-creates the same packfile, and is then subsequently
deleted.

The solution is for the multiplex stores to properly return a path if one was
returned from the underlying stores.

Reviewed By: DurhamG

Differential Revision: D21211981

fbshipit-source-id: 74e4b9e5d2f5d9409ce732935552a02bdde85b93

eden/scm/lib/revisionstore/src/contentstore.rs

@@ -431,6 +431,8 @@ mod tests {
     use util::path::create_dir;
 
     use crate::{
+        metadatastore::MetadataStore,
+        repack::{repack, RepackKind, RepackLocation},
         testutil::{make_config, make_lfs_config, FakeHgIdRemoteStore},
         types::ContentHash,
     };
@@ -784,6 +786,39 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn test_repack_one_datapack_lfs() -> Result<()> {
+        let cachedir = TempDir::new()?;
+        let localdir = TempDir::new()?;
+        let mut config = make_lfs_config(&cachedir);
+        config.set("lfs", "threshold", Some("10M"), &Default::default());
+
+        let k1 = key("a", "2");
+        let delta = Delta {
+            data: Bytes::from(&[1, 2, 3, 4, 5][..]),
+            base: None,
+            key: k1.clone(),
+        };
+
+        let store = Arc::new(ContentStore::new(&localdir, &config)?);
+        store.add(&delta, &Default::default())?;
+        store.flush()?;
+
+        let metadata = Arc::new(MetadataStore::new(&localdir, &config)?);
+
+        repack(
+            get_packs_path(&localdir, &None)?,
+            Some((store, metadata)),
+            RepackKind::Full,
+            RepackLocation::Local,
+        )?;
+
+        let store = Arc::new(ContentStore::new(&localdir, &config)?);
+        assert_eq!(store.get_delta(&k1)?, Some(delta));
+
+        Ok(())
+    }
+
     #[cfg(feature = "fb")]
     mod fb_tests {
         use super::*;

eden/scm/lib/revisionstore/src/lfs.rs

@@ -846,9 +846,9 @@ impl HgIdMutableDeltaStore for LfsMultiplexer {
     }
 
     fn flush(&self) -> Result<Option<PathBuf>> {
-        self.non_lfs.flush()?;
+        let ret = self.non_lfs.flush()?;
         self.lfs.flush()?;
-        Ok(None)
+        Ok(ret)
     }
 }
 

eden/scm/lib/revisionstore/src/multiplexstore.rs

@@ -61,11 +61,16 @@ impl<T: HgIdMutableDeltaStore> HgIdMutableDeltaStore for MultiplexDeltaStore<T>
     }
 
     fn flush(&self) -> Result<Option<PathBuf>> {
+        let mut ret = None;
         for store in self.stores.iter() {
-            store.flush()?;
+            let opt = store.flush()?;
+            // It's non sensical for the MultiplexStore to be built with multiple pack stores,
+            // therefore let's assert that only one store can ever return a PathBuf.
+            assert!(opt.is_none() || !ret.is_some());
+            ret = ret.or(opt);
         }
 
-        Ok(None)
+        Ok(ret)
     }
 }
 
@@ -127,11 +132,16 @@ impl<T: HgIdMutableHistoryStore> HgIdMutableHistoryStore for MultiplexHgIdHistor
     }
 
     fn flush(&self) -> Result<Option<PathBuf>> {
+        let mut ret = None;
         for store in self.stores.iter() {
-            store.flush()?;
+            let opt = store.flush()?;
+            // It's non sensical for the MultiplexStore to be built with multiple pack stores,
+            // therefore let's assert that only one store can ever return a PathBuf.
+            assert!(opt.is_none() || !ret.is_some());
+            ret = ret.or(opt);
         }
 
-        Ok(None)
+        Ok(ret)
     }
 }
 

eden/scm/lib/revisionstore/src/repack.rs

@@ -317,6 +317,8 @@ fn repack_datapack_to_contentstore(
     let new_pack = store.commit_pending(location)?;
 
     for path in repacked {
+        // TODO: This is a bit fragile as a bug in commit_pending not returning a path could lead
+        // to data loss. A better return type would avoid this.
         if Some(&path) != new_pack.as_ref() {
             match DataPack::new(&path) {
                 Ok(pack) => pack.delete()?,