fix scm-aware Watchman queries when filters are active

Summary:
# Context

We received several reports of scm-aware Watchman queries returning incorrect results when FilteredFS was being used and filters were actively enabled[1]. In these reports, users would expect that modified files between commits would be reported as changed, however Watchman (err, EdenFS in this case) reported these files as unmodified between commits. This was causing build tools to use stale file content.

After further investigation, I found that disabling filters (running `hg filteredfs disable`) caused Watchman to report correct results. Therefore, the bug only occurred when a non-null filter was enabled on FilteredFS. This significantly narrowed down the possible causes.

# Root Cause

The root cause was a bug in the ObjectID comparison logic for FilteredBackingStores. FilteredBackingStores use FilteredObjectIDs. When determining differences between these IDs, we must take into account three things:

1) Do the FilteredObjectIDs have the same type (Blob, Tree, or Unfiltered Tree)?
2) Do the Filters associated w/ each FilteredObjectID resolve to the exact same coverage (i.e. do they filter the same descendents)?
3) Do the underlying ObjectIDs associated w/ each FilteredObjectID refer to the same object(s).

We were successfully determining #1 and #2, but we failed to determine #3 correctly in one specific edge case. This was causing us to incorrectly calculate two FilteredObjectIDs as identical when they were actually different.

# This diff

This diff ensures that object equality (aka #3 from above) is checked in all cases. Therefore we don't hit a scenario where #1 and #2 are equal and #3 is ignored altogether.

Reviewed By: kmancini

Differential Revision: D55350885

fbshipit-source-id: bdafab99e56ddfa98446b4ba26dc0bde96121dad

eden/fs/store/FilteredBackingStore.cpp

@@ -136,7 +136,19 @@ ObjectComparison FilteredBackingStore::compareObjectsById(
         filteredOne.filter(),
         filteredTwo.filter());
     if (pathAffected.isReady()) {
-      return std::move(pathAffected).get();
+      auto filterComparison = std::move(pathAffected).get();
+
+      // If the filters are identical, we need to check whether the underlying
+      // Objects are identical. In other words, the filters being identical is
+      // not enough to confirm that the objects are identical.
+      if (filterComparison == ObjectComparison::Identical) {
+        return backingStore_->compareObjectsById(
+            filteredOne.object(), filteredTwo.object());
+      } else {
+        // If the filter coverage is different, the objects must be filtered
+        // differently (or we can't confirm they're filtered the same way).
+        return filterComparison;
+      }
     } else {
       // We can't immediately tell if the path is affected by the filter
       // change. Instead of chaining the future and queueing up a bunch of

eden/fs/store/test/FilteredBackingStoreTest.cpp

@@ -797,6 +797,12 @@ TEST_F(FakePrefixFilteredBackingStoreTest, testCompareSimilarTreeObjectsById) {
     return;
   }
 
+  // These two trees have different filters, but the filters evaluate to the
+  // same filtering results. These two trees are also different objects
+  // altogether (i.e. they have different underlying ObjectIDs).
+  //
+  // These two trees should resolve to different objects, but a previous bug in
+  // comparison logic caused them to evaluate as identical.
   auto substringFilter = std::make_unique<FakePrefixFilter>();
   auto treeFOID =
       FilteredObjectId{RelativePath{"bar"}, "foooo", makeTestHash("0000")};
@@ -820,8 +826,8 @@ TEST_F(FakePrefixFilteredBackingStoreTest, testCompareSimilarTreeObjectsById) {
   };
 
   // We expect a tree with the same filter coverage but different underlying
-  // objects to be identical (due to a bug).
-  EXPECT_EQ(
+  // objects to not be identical.
+  EXPECT_NE(
       filteredStore_->compareObjectsById(
           ObjectId{treeFOID.getValue()}, ObjectId{similarFOID.getValue()}),
       ObjectComparison::Identical);