HgDatapackStore: use C++ options to stop using legacy filters

Reviewed By: jdelliot

Differential Revision: D53840247

fbshipit-source-id: c93c3f13ce39e4c059eebb3cf113f97a4bae6d07

eden/fs/store/hg/HgDatapackStore.cpp

@@ -83,12 +83,21 @@ Tree::value_type fromRawTreeEntry(
   return {std::move(name), std::move(treeEntry)};
 }
 
+bool doFilteredPathsApply(
+    bool ignoreFilteredPathsConfig,
+    const std::unordered_set<RelativePath>& filteredPaths,
+    const RelativePath& path) {
+  return ignoreFilteredPathsConfig || filteredPaths.empty() ||
+      filteredPaths.count(path) == 0;
+}
+
 TreePtr fromRawTree(
     const sapling::Tree* tree,
     const ObjectId& edenTreeId,
     RelativePathPiece path,
     HgObjectIdFormat hgObjectIdFormat,
-    const std::unordered_set<RelativePath>& filteredPaths) {
+    const std::unordered_set<RelativePath>& filteredPaths,
+    bool ignoreFilteredPathsConfig) {
   Tree::container entries{kPathMapDefaultCaseSensitive};
 
   entries.reserve(tree->entries.size());
@@ -97,8 +106,8 @@ TreePtr fromRawTree(
       auto entry = fromRawTreeEntry(tree->entries[i], path, hgObjectIdFormat);
       // TODO(xavierd): In the case where this checks becomes too hot, we may
       // need to change to a Trie like datastructure for fast filtering.
-      if (filteredPaths.empty() ||
-          filteredPaths.count(path + entry.first) == 0) {
+      if (doFilteredPathsApply(
+              ignoreFilteredPathsConfig, filteredPaths, path + entry.first)) {
         entries.emplace(entry.first, std::move(entry.second));
       }
     } catch (const PathComponentContainsDirectorySeparator& ex) {
@@ -183,7 +192,8 @@ void HgDatapackStore::getTreeBatch(const ImportRequestsList& importRequests) {
                     treeRequest->hash,
                     treeRequest->proxyHash.path(),
                     hgObjectIdFormat,
-                    *filteredPaths)};
+                    *filteredPaths,
+                    cppOptions_->ignoreConfigFilter())};
               });
         }
 
@@ -227,7 +237,8 @@ folly::Try<TreePtr> HgDatapackStore::getTree(
         edenTreeId,
         path,
         std::move(hgObjectIdFormat),
-        std::move(*filteredPaths))};
+        std::move(*filteredPaths),
+        cppOptions_->ignoreConfigFilter())};
   } else {
     return GetTreeResult{tree.exception()};
   }
@@ -247,7 +258,8 @@ TreePtr HgDatapackStore::getTreeLocal(
         edenTreeId,
         proxyHash.path(),
         hgObjectIdFormat,
-        *filteredPaths);
+        *filteredPaths,
+        cppOptions_->ignoreConfigFilter());
   }
 
   return nullptr;

eden/fs/store/hg/test/HgDatapackStoreTest.cpp

@@ -69,7 +69,7 @@ std::vector<PathComponent> getTreeNames(
 }
 } // namespace
 
-struct HgDatapackStoreTest : TestRepo, ::testing::Test {
+struct HgDatapackStoreTestBase : TestRepo, ::testing::Test {
   EdenStatsPtr stats{makeRefPtr<EdenStats>()};
 
   HgDatapackStore::RustOptions options{computeTestRustOptions()};
@@ -90,6 +90,11 @@ struct HgDatapackStoreTest : TestRepo, ::testing::Test {
       std::make_shared<ReloadableConfig>(rawEdenConfig)};
   FaultInjector faultInjector{/*enabled=*/true};
 
+  std::shared_ptr<MemoryLocalStore> localStore{
+      std::make_shared<MemoryLocalStore>(stats.copy())};
+};
+
+struct HgDatapackStoreTest : HgDatapackStoreTestBase {
   HgDatapackStore datapackStore{
       repo.path(),
       options,
@@ -98,8 +103,17 @@ struct HgDatapackStoreTest : TestRepo, ::testing::Test {
       edenConfig,
       nullptr,
       &faultInjector};
-  std::shared_ptr<MemoryLocalStore> localStore{
-      std::make_shared<MemoryLocalStore>(stats.copy())};
+};
+
+struct HgDatapackStoreTestIgnoreConfig : HgDatapackStoreTestBase {
+  HgDatapackStore datapackStore{
+      repo.path(),
+      options,
+      std::make_unique<HgDatapackStore::CppOptions>(
+          /*ignoreFilteredPathsConfig=*/true),
+      edenConfig,
+      nullptr,
+      &faultInjector};
 };
 
 TEST_F(HgDatapackStoreTest, getTreeBatch) {
@@ -156,3 +170,39 @@ TEST_F(HgDatapackStoreTest, getTreeBatch) {
       getTreeNames(tree1),
       ::testing::ElementsAre(PathComponent{"foo"}, PathComponent{"src"}));
 }
+
+TEST_F(HgDatapackStoreTestIgnoreConfig, getTreeBatch) {
+  // force a reload
+  updateTestEdenConfig(
+      testConfigSource,
+      edenConfig,
+      {
+          {"hg:filtered-paths", "['foo']"},
+      });
+
+  auto tree1Hash = HgProxyHash::makeEmbeddedProxyHash1(
+      datapackStore.getManifestNode(ObjectId::fromHex(commit1.value())).value(),
+      RelativePathPiece{});
+
+  HgProxyHash proxyHash =
+      HgProxyHash::load(localStore.get(), tree1Hash, "getTree", *stats);
+
+  auto request = HgImportRequest::makeTreeImportRequest(
+      tree1Hash,
+      proxyHash,
+      ObjectFetchContext::getNullContext()->getPriority(),
+      ObjectFetchContext::getNullContext()->getCause(),
+      ObjectFetchContext::getNullContext()->getClientPid());
+
+  auto executor = std::make_shared<folly::CPUThreadPoolExecutor>(1);
+  auto tree1fut = via(executor.get(), [&]() {
+    this->datapackStore.getTreeBatch(std::vector{request});
+  });
+
+  std::move(tree1fut).get();
+  auto tree1 = request->getPromise<TreePtr>()->getFuture().get();
+
+  ASSERT_THAT(
+      getTreeNames(tree1),
+      ::testing::ElementsAre(PathComponent{"foo"}, PathComponent{"src"}));
+}

eden/integration/hg/legacy_filter_test.py

@@ -16,9 +16,6 @@ from .lib.hg_extension_test_base import EdenHgTestCase, hg_test
 @hg_test
 # pyre-ignore[13]: T62487924
 class FilterTest(EdenHgTestCase):
-    hidden_files: Set[str] = {"foo/foo.cpp", "foo/bar/baz/file", "hello"}
-    symlink_to_hidden: Set[str] = {"fooslink", "foofooslink"}
-
     def populate_backing_repo(self, repo: hgrepo.HgRepository) -> None:
         repo.write_file("foo/foo.cpp", "foo\n")
         repo.write_file("foo/bar/baz/file", "foo\n")
@@ -38,31 +35,71 @@ class FilterTest(EdenHgTestCase):
         )
         return result
 
+    def hidden_files(self) -> Set[str]:
+        if self.backing_store_type == "filteredhg":
+            return set()
+        else:
+            return {"foo/foo.cpp", "foo/bar/baz/file", "hello"}
+
+    def expected_toplevel_readdir_result(self) -> Set[str]:
+        if self.backing_store_type == "filteredhg":
+            return {
+                "bar",
+                "baz",
+                ".eden",
+                "foo",
+                "foofooslink",
+                "fooslink",
+                "hello",
+                ".hg",
+            }
+        else:
+            return {"bar", "baz", "foofooslink", "fooslink", ".eden", ".hg"}
+
+    def expected_baz_readdir_result(self) -> Set[str]:
+        if self.backing_store_type == "filteredhg":
+            return {"baz.cpp", "baz2.cpp"}
+        else:
+            return {"baz2.cpp"}
+
+    def symlink_to_hidden(self) -> Set[str]:
+        if self.backing_store_type == "filteredhg":
+            return set()
+        else:
+            return {"fooslink", "foofooslink"}
+
     def test_read_dir(self) -> None:
         listing = set(self.read_dir(""))
         if sys.platform == "win32":
-            self.assertEqual(listing, {"bar", "baz", ".eden", ".hg"})
-        else:
+            # On Windows we don't expect symlinked files to exist. Note: this
+            # could change if symlinks are rolled out to Windows in the future.
             self.assertEqual(
-                listing, {"bar", "baz", "foofooslink", "fooslink", ".eden", ".hg"}
+                listing,
+                set(
+                    filter(
+                        lambda e: "link" in e, self.expected_toplevel_readdir_result()
+                    )
+                ),
             )
+        else:
+            self.assertEqual(listing, self.expected_toplevel_readdir_result())
 
-        listing = self.read_dir("baz")
-        self.assertEqual(listing, ["baz2.cpp"])
+        listing = set(self.read_dir("baz"))
+        self.assertEqual(listing, self.expected_baz_readdir_result())
 
     def test_read_hidden_file(self) -> None:
-        for path in self.hidden_files:
+        for path in self.hidden_files():
             with self.assertRaisesRegex(IOError, ""):
                 self.read_file(path)
 
         if sys.platform != "win32":
-            for path in self.symlink_to_hidden:
+            for path in self.symlink_to_hidden():
                 with self.assertRaisesRegex(IOError, ""):
                     self.read_file(path)
 
     def test_get_sha1_thrift(self) -> None:
         with self.get_thrift_client_legacy() as client:
-            for path in self.hidden_files:
+            for path in self.hidden_files():
                 result = client.getSHA1(
                     self.mount_path_bytes, [path.encode()], sync=SyncBehavior()
                 )