revisionstore: better handle empty packfile flush

Summary:
On flush, if no data was added to the mutable packfile, Ok(None) should be
returned. This fixes a bug in the PackStore where an attempt was made to open
the newly created packfile, while none existed.

Reviewed By: quark-zju

Differential Revision: D17861327

fbshipit-source-id: 70f3f49f60de5883ca85787c1bb00c16c74e8f2f

lib/asyncrevisionstore/src/asyncmutabledatapack.rs

@@ -72,7 +72,7 @@ mod tests {
         let work = mutabledatapack.and_then(move |datapack| datapack.close());
         let mut runtime = Runtime::new().unwrap();
 
-        let datapackbase = runtime.block_on(work).unwrap().unwrap();
-        assert_eq!(datapackbase, PathBuf::new());
+        let datapackbase = runtime.block_on(work).unwrap();
+        assert_eq!(datapackbase, None);
     }
 }

lib/asyncrevisionstore/src/asyncmutablehistorypack.rs

@@ -45,8 +45,8 @@ mod tests {
         let work = mutablehistorypack.and_then(move |historypack| historypack.close());
         let mut runtime = Runtime::new().unwrap();
 
-        let historypackpath = runtime.block_on(work).unwrap().unwrap();
-        assert_eq!(historypackpath, PathBuf::new());
+        let historypackpath = runtime.block_on(work).unwrap();
+        assert_eq!(historypackpath, None);
     }
 
     #[test]

lib/revisionstore/src/mutabledatapack.rs

@@ -173,8 +173,7 @@ impl MutableDeltaStore for MutableDataPack {
         let new_inner = MutableDataPackInner::new(&guard.dir, DataPackVersion::One)?;
         let old_inner = replace(&mut *guard, new_inner);
 
-        let path = old_inner.close_pack()?;
-        Ok(Some(path))
+        old_inner.close_pack()
     }
 }
 
@@ -442,7 +441,7 @@ mod tests {
         let tempdir = tempdir().unwrap();
 
         let mutdatapack = MutableDataPack::new(tempdir.path(), DataPackVersion::One).unwrap();
-        mutdatapack.flush().unwrap();
+        assert_eq!(mutdatapack.flush().unwrap(), None);
         drop(mutdatapack);
         assert_eq!(fs::read_dir(tempdir.path()).unwrap().count(), 0);
     }

lib/revisionstore/src/mutablehistorypack.rs

@@ -148,8 +148,7 @@ impl MutableHistoryStore for MutableHistoryPack {
         let new_inner = MutableHistoryPackInner::new(&guard.dir, HistoryPackVersion::One)?;
         let old_inner = replace(&mut *guard, new_inner);
 
-        let path = old_inner.close_pack()?;
-        Ok(Some(path))
+        old_inner.close_pack()
     }
 }
 
@@ -438,7 +437,7 @@ mod tests {
         let tempdir = tempdir().unwrap();
         let muthistorypack =
             MutableHistoryPack::new(tempdir.path(), HistoryPackVersion::One).unwrap();
-        muthistorypack.flush().unwrap();
+        assert_eq!(muthistorypack.flush().unwrap(), None);
         drop(muthistorypack);
         assert_eq!(fs::read_dir(tempdir.path()).unwrap().count(), 0);
     }

lib/revisionstore/src/mutablepack.rs

@@ -50,7 +50,7 @@ pub trait MutablePack {
 
     /// Close the packfile, returning the path of the final immutable pack on disk. The
     /// `MutablePack` is no longer usable after being closed.
-    fn close_pack(self) -> Fallible<PathBuf>
+    fn close_pack(self) -> Fallible<Option<PathBuf>>
     where
         Self: Sized,
     {
@@ -61,7 +61,7 @@ pub trait MutablePack {
         let (packfile, indexfile, base_filepath) = match self.build_files() {
             Err(err) => {
                 if err.downcast_ref::<EmptyMutablePack>().is_some() {
-                    return Ok(PathBuf::new());
+                    return Ok(None);
                 } else {
                     return Err(err);
                 }
@@ -81,6 +81,6 @@ pub trait MutablePack {
         persist(packfile, packfile_path)?;
         persist(indexfile, indexfile_path)?;
 
-        Ok(base_filepath)
+        Ok(Some(base_filepath))
     }
 }

lib/revisionstore/src/packstore.rs

@@ -873,4 +873,20 @@ mod tests {
         }
         Ok(())
     }
+
+    #[test]
+    fn test_datapack_flush_empty() -> Fallible<()> {
+        let tempdir = TempDir::new()?;
+        let packstore = MutableDataPackStore::new(&tempdir, CorruptionPolicy::REMOVE)?;
+        packstore.flush()?;
+        Ok(())
+    }
+
+    #[test]
+    fn test_histpack_flush_empty() -> Fallible<()> {
+        let tempdir = TempDir::new()?;
+        let packstore = MutableHistoryPackStore::new(&tempdir, CorruptionPolicy::REMOVE)?;
+        packstore.flush()?;
+        Ok(())
+    }
 }

lib/revisionstore/src/repack.rs

@@ -93,7 +93,7 @@ fn repack_packs<'a, T: MutablePack, U: LocalStore + Repackable + ToKeys>(
         return Err(RepackFailure::Total(errors).into());
     }
 
-    let new_pack_path = mut_pack.close_pack()?;
+    let new_pack_path = mut_pack.close_pack()?.unwrap();
     let new_pack = U::from_path(&new_pack_path)?;
 
     let mut successfully_repacked = 0;