remove use of getScmStatusBetweenRevisions in eden doctor

Summary: There is one instace of `getScmStatusBetweenRevisions` in use - it is used in the eden cli in a hacky way to check if a commit hash is valid. Since this is not used anywhere else in a meaningful way, this replaces that use case with a hg call and depreciates `getScmStatusBetweenRevisions`

Reviewed By: simpkins

Differential Revision: D18690026

fbshipit-source-id: 02bd2c20a0f631ec41116f9fd4e18d14369298ef

eden/cli/config.py

@@ -758,6 +758,9 @@ Do you want to run `eden mount %s` instead?"""
 
         return checkouts
 
+    def get_hg_repo(self, path: str) -> Optional[util.HgRepo]:
+        return util.get_hg_repo(path)
+
     def _get_directory_map(self) -> Dict[Path, str]:
         """
         Parse config.json which holds a mapping of mount paths to their

eden/cli/doctor/check_hg.py

@@ -16,7 +16,6 @@ import facebook.eden.ttypes as eden_ttypes
 from eden.cli import hg_util
 from eden.cli.config import EdenCheckout
 from eden.cli.doctor.problem import FixableProblem, ProblemTracker, UnexpectedCheckError
-from thrift.Thrift import TApplicationException
 
 
 class HgChecker:
@@ -202,18 +201,21 @@ class DirstateChecker(HgFileChecker):
         return binascii.hexlify(commit).decode("utf-8")
 
     def _is_commit_hash_valid(self, commit_hash: bytes) -> bool:
-        # The null commit ID is always valid
-        if commit_hash == self._null_commit_id:
-            return True
-
         try:
-            with self.checkout.instance.get_thrift_client() as client:
-                client.getScmStatusBetweenRevisions(
-                    bytes(self.checkout.path), commit_hash, commit_hash
+            repo = self.checkout.instance.get_hg_repo(str(self.checkout.path))
+            if repo is not None:
+                repo.get_commit_hash(
+                    self._commit_hex(commit_hash), stderr_output=subprocess.STDOUT
                 )
-            return True
-        except (TApplicationException, eden_ttypes.EdenError) as ex:
-            if "RepoLookupError: unknown revision" in str(ex):
+                return True
+            else:
+                # This case can happen if the repo is corrupted. In this case return
+                # true and assume that the commit was valid before the repo corrupted
+                # This will also be hit in the cli unit tests since the FakeEdenInstance
+                # does not set up a complete .hg directory.
+                return True
+        except subprocess.CalledProcessError as ex:
+            if b"unknown revision" in ex.output:
                 return False
             raise
 

eden/cli/doctor/test/doctor_test.py

@@ -16,6 +16,7 @@ from eden.cli.config import EdenCheckout, EdenInstance
 from eden.cli.doctor import check_hg, check_watchman
 from eden.cli.doctor.test.lib.fake_client import FakeClient, ResetParentsCommitsArgs
 from eden.cli.doctor.test.lib.fake_eden_instance import FakeEdenInstance
+from eden.cli.doctor.test.lib.fake_hg_repo import FakeHgRepo
 from eden.cli.doctor.test.lib.fake_mount_table import FakeMountTable
 from eden.cli.doctor.test.lib.testcase import DoctorTestBase
 from eden.cli.test.lib.output import TestOutput
@@ -609,7 +610,7 @@ Repairing hg directory contents for {checkout.path}...<green>fixed<reset>
         self.assert_dirstate_p0(checkout, snapshot_hex)
 
     def test_snapshot_and_dirstate_file_differ_and_snapshot_invalid(self):
-        def check_commit_validity(path: bytes, commit: str) -> bool:
+        def check_commit_validity(commit: str) -> bool:
             if commit == "12345678" * 5:
                 return False
             return True
@@ -649,7 +650,10 @@ Repairing hg directory contents for {checkout.path}...<green>fixed<reset>
     def test_snapshot_and_dirstate_file_differ_and_all_commit_hash_invalid(
         self, mock_get_tip_commit_hash
     ):
-        def check_commit_validity(path: bytes, commit: str) -> bool:
+        def check_commit_validity(commit: str) -> bool:
+            null_commit = "00000000" * 5
+            if commit == null_commit:
+                return True
             return False
 
         dirstate_hash_hex = "12000000" * 5
@@ -691,7 +695,7 @@ Repairing hg directory contents for {checkout.path}...<green>fixed<reset>
     def test_snapshot_and_dirstate_file_differ_and_all_parents_invalid(
         self, mock_get_tip_commit_hash
     ):
-        def check_commit_validity(path: bytes, commit: str) -> bool:
+        def check_commit_validity(commit: str) -> bool:
             return False
 
         dirstate_hash_hex = "12000000" * 5
@@ -732,7 +736,7 @@ Repairing hg directory contents for {checkout.path}...<green>fixed<reset>
         self.assert_dirstate_p0(checkout, valid_commit_hash)
 
     def test_snapshot_and_dirstate_file_differ_and_dirstate_commit_hash_invalid(self):
-        def check_commit_validity(path: bytes, commit: str) -> bool:
+        def check_commit_validity(commit: str) -> bool:
             if commit == "12000000" * 5:
                 return False
             return True
@@ -763,7 +767,7 @@ Repairing hg directory contents for {checkout.path}...<green>fixed<reset>
         dirstate_hash_hex: str,
         snapshot_hex: str,
         dirstate_parent2_hash_hex=None,
-        commit_checker: Optional[Callable[[bytes, str], bool]] = None,
+        commit_checker: Optional[Callable[[str], bool]] = None,
     ) -> Tuple[EdenCheckout, doctor.ProblemFixer, str]:
         instance = FakeEdenInstance(self.make_temporary_directory())
         if dirstate_parent2_hash_hex is None:
@@ -777,9 +781,10 @@ Repairing hg directory contents for {checkout.path}...<green>fixed<reset>
                 dirstate_parent=(dirstate_hash_hex, dirstate_parent2_hash_hex),
             )
 
-        if commit_checker:
-            client = typing.cast(FakeClient, checkout.instance.get_thrift_client())
-            client.commit_checker = commit_checker
+        hg_repo = checkout.instance.get_hg_repo(str(checkout.path))
+        if commit_checker and hg_repo is not None:
+            fake_hg_repo = typing.cast(FakeHgRepo, hg_repo)
+            fake_hg_repo.commit_checker = commit_checker
 
         fixer, out = self.create_fixer(dry_run=False)
         check_hg.check_hg(fixer, checkout)

eden/cli/doctor/test/lib/fake_client.py

@@ -18,8 +18,6 @@ class ResetParentsCommitsArgs(NamedTuple):
 
 
 class FakeClient:
-    commit_checker: Optional[Callable[[bytes, str], bool]] = None
-
     def __init__(self):
         self._mounts = []
         self.set_parents_calls: List[ResetParentsCommitsArgs] = []
@@ -50,40 +48,3 @@ class FakeClient:
                 mount=mountPoint, parent1=parents.parent1, parent2=parents.parent2
             )
         )
-
-    def getScmStatus(
-        self,
-        mountPoint: Optional[bytes] = None,
-        listIgnored: Optional[bool] = None,
-        commit: Optional[bytes] = None,
-    ) -> Optional[eden_ttypes.ScmStatus]:
-        assert mountPoint is not None
-        self._check_commit_valid(mountPoint, commit)
-        return None
-
-    def getScmStatusBetweenRevisions(
-        self,
-        mountPoint: Optional[bytes] = None,
-        oldHash: Optional[bytes] = None,
-        newHash: Optional[bytes] = None,
-    ) -> Optional[eden_ttypes.ScmStatus]:
-        assert mountPoint is not None
-        self._check_commit_valid(mountPoint, oldHash)
-        self._check_commit_valid(mountPoint, newHash)
-        return None
-
-    def _check_commit_valid(self, path: bytes, commit: Union[None, bytes, str]):
-        if self.commit_checker is None:
-            return
-
-        if commit is None:
-            return
-        if isinstance(commit, str):
-            commit_hex = commit
-        else:
-            commit_hex = binascii.hexlify(commit).decode("utf-8")
-
-        if not self.commit_checker(path, commit_hex):
-            raise eden_ttypes.EdenError(
-                message=f"RepoLookupError: unknown revision {commit_hex}"
-            )

eden/cli/doctor/test/lib/fake_eden_instance.py

@@ -20,6 +20,7 @@ from eden.cli.config import CheckoutConfig, EdenCheckout, EdenInstance, HealthSt
 from fb303_core.ttypes import fb303_status
 
 from .fake_client import FakeClient
+from .fake_hg_repo import FakeHgRepo
 from .fake_mount_table import FakeMountTable
 
 
@@ -30,7 +31,7 @@ class FakeCheckout(NamedTuple):
 
 
 class FakeEdenInstance:
-    default_commit_hash = "1" * 40
+    default_commit_hash: str = "1" * 40
 
     def __init__(
         self,
@@ -54,7 +55,7 @@ class FakeEdenInstance:
 
         # A map from mount path --> FakeCheckout
         self._checkouts_by_path: Dict[str, FakeCheckout] = {}
-
+        self._hg_repo_by_path: Dict[str, FakeHgRepo] = {}
         self.mount_table = FakeMountTable()
         self._next_dev_id = 10
 
@@ -161,7 +162,7 @@ class FakeEdenInstance:
         full_path: str,
         fake_checkout: FakeCheckout,
         dirstate_parent: Union[str, Tuple[str, str], None],
-    ):
+    ) -> None:
         hg_dir = Path(full_path) / ".hg"
         hg_dir.mkdir()
         dirstate_path = hg_dir / "dirstate"
@@ -191,6 +192,8 @@ class FakeEdenInstance:
         (hg_dir / "bookmarks").touch()
         (hg_dir / "branch").write_text("default\n")
 
+        self._hg_repo_by_path[full_path] = FakeHgRepo()
+
     def get_mount_paths(self) -> Iterable[str]:
         return self._checkouts_by_path.keys()
 
@@ -238,3 +241,8 @@ class FakeEdenInstance:
 
     def get_config_value(self, key: str, default: str) -> str:
         return self._config.get(key, default)
+
+    def get_hg_repo(self, path: str) -> Optional[FakeHgRepo]:
+        if path in self._hg_repo_by_path:
+            return self._hg_repo_by_path[path]
+        return None

eden/cli/doctor/test/lib/fake_hg_repo.py

@@ -0,0 +1,24 @@
+#!/usr/bin/env python3
+# Copyright (c) Facebook, Inc. and its affiliates.
+#
+# This software may be used and distributed according to the terms of the
+# GNU General Public License version 2.
+
+import subprocess
+from typing import Callable, Optional
+
+
+class FakeHgRepo:
+    commit_checker: Optional[Callable[[str], bool]] = None
+
+    def get_commit_hash(self, commit: str, stderr_output=None) -> str:
+        commit_checker = self.commit_checker
+
+        if not commit_checker or commit_checker(commit):
+            return commit
+
+        cmd = " ".join(["log", "-r", commit, "-T{node}"])
+        output = f"RepoLookupError: unknown revision {commit}"
+        raise subprocess.CalledProcessError(
+            returncode=255, cmd=cmd, output=str.encode(output)
+        )

eden/cli/util.py

@@ -295,15 +295,17 @@ class HgRepo(Repo):
     def __repr__(self) -> str:
         return f"HgRepo(source={self.source!r}, " f"working_dir={self.working_dir!r})"
 
-    def _run_hg(self, args: List[str]) -> bytes:
+    def _run_hg(self, args: List[str], stderr_output=None) -> bytes:
         cmd = [self._hg_binary] + args
-        out_bytes = subprocess.check_output(cmd, cwd=self.working_dir, env=self._env)
+        out_bytes = subprocess.check_output(
+            cmd, cwd=self.working_dir, env=self._env, stderr=stderr_output
+        )
         # pyre-fixme[22]: The cast is redundant.
         out = typing.cast(bytes, out_bytes)
         return out
 
-    def get_commit_hash(self, commit: str) -> str:
-        out = self._run_hg(["log", "-r", commit, "-T{node}"])
+    def get_commit_hash(self, commit: str, stderr_output=None) -> str:
+        out = self._run_hg(["log", "-r", commit, "-T{node}"], stderr_output)
         return out.strip().decode("utf-8")
 
     def cat_file(self, commit: str, path: str) -> bytes:
@@ -364,7 +366,7 @@ def _get_git_repo(path: str) -> Optional[GitRepo]:
     return None
 
 
-def _get_hg_repo(path: str) -> Optional[HgRepo]:
+def get_hg_repo(path: str) -> Optional[HgRepo]:
     """
     If path points to a mercurial repository, return a HgRepo object.
     Otherwise, if path is not a mercurial repository, return None.
@@ -401,7 +403,7 @@ def get_repo(path: str) -> Optional[Repo]:
         return None
 
     while True:
-        hg_repo = _get_hg_repo(path)
+        hg_repo = get_hg_repo(path)
         if hg_repo is not None:
             return hg_repo
         git_repo = _get_git_repo(path)

eden/fs/service/eden.thrift

@@ -874,6 +874,8 @@ service EdenService extends fb303_core.BaseService {
   ) throws (1: EdenError ex)
 
   /**
+   * DEPRECATED
+   *
    * Computes the status between two specified revisions.
    * This does not care about the state of the working copy.
    */

eden/integration/hg/doctor_test.py

@@ -0,0 +1,118 @@
+#!/usr/bin/env python3
+# Copyright (c) Facebook, Inc. and its affiliates.
+#
+# This software may be used and distributed according to the terms of the
+# GNU General Public License version 2.
+
+import subprocess
+from pathlib import Path
+
+from eden.integration.lib import hgrepo
+from facebook.eden.ttypes import WorkingDirectoryParents
+
+from .lib.hg_extension_test_base import EdenHgTestCase, hg_test
+
+
+@hg_test
+# pyre-fixme[13]: Attribute `backing_repo` is never initialized.
+# pyre-fixme[13]: Attribute `backing_repo_name` is never initialized.
+# pyre-fixme[13]: Attribute `config_variant_name` is never initialized.
+# pyre-fixme[13]: Attribute `repo` is never initialized.
+class DoctorTest(EdenHgTestCase):
+    commit1: str
+    commit2: str
+
+    def populate_backing_repo(self, repo: hgrepo.HgRepository) -> None:
+        repo.write_file("letters", "a\nb\nc\n")
+        repo.write_file("numbers", "1\n2\n3\n")
+        self.commit1 = repo.commit("Initial commit.")
+
+        repo.write_file("letters", "a\n")
+        repo.write_file("numbers", "1\n")
+        self.commit2 = repo.commit("New commit.")
+
+    def test_eden_doctor_fixes_valid_mismatched_parents(self) -> None:
+        # this specifically tests when EdenFS and Mercurial are out of sync,
+        # but and mercurial does know about EdenFS's WCP
+        mount_path = Path(self.mount)
+
+        # set eden to point at the first commit, while keeping mercurial at the
+        # second commit
+        parents = WorkingDirectoryParents(parent1=self.commit1.encode("utf-8"))
+        with self.eden.get_thrift_client() as client:
+            client.resetParentCommits(mountPoint=bytes(mount_path), parents=parents)
+
+        with self.assertRaises(hgrepo.HgError) as status_context:
+            self.repo.status()
+
+        self.assertIn(
+            b"requested parent commit is out-of-date", status_context.exception.stderr
+        )
+
+        # hg whereami reads eden's SNAPSHOT file
+        eden_parent = self.hg("whereami").strip("\n")
+        hg_parent = self.hg("log", "-r.", "-T{node}")
+
+        # make sure that eden and mercurial are out of sync
+        self.assertNotEqual(eden_parent, hg_parent)
+
+        cmd_result = self.eden.run_unchecked("doctor", "-n", stdout=subprocess.PIPE)
+        error_msg = (
+            "mercurial's parent commit is %s, but Eden's internal parent commit is %s"
+            % (self.commit2, self.commit1)
+        )
+        self.assertIn(error_msg.encode("utf-8"), cmd_result.stdout)
+
+        # run eden doctor and make sure eden and mercurial are in sync again
+        fixed_result = self.eden.run_unchecked("doctor", stdout=subprocess.PIPE)
+        self.assertIn(b"Successfully fixed 1 problem", fixed_result.stdout)
+
+        eden_parent_fixed = self.hg("whereami").strip("\n")
+        hg_parent_fixed = self.hg("log", "-r.", "-T{node}")
+        self.assertEqual(eden_parent_fixed, hg_parent_fixed)
+
+        # Since Eden's snapshot file pointed to a known commit, it should pick
+        # Eden's parent as the new parent
+        self.assertEqual(eden_parent, hg_parent_fixed)
+
+    def test_eden_doctor_fixes_invalid_mismatched_parents(self) -> None:
+        # this specifically tests when EdenFS and Mercurial are out of sync,
+        # but Mercurial does not know about Eden's WCP
+
+        mount_path = Path(self.mount)
+
+        corrupt_commit = b"9" * 40
+        parents = WorkingDirectoryParents(parent1=corrupt_commit)
+
+        # point eden to a random commit
+        with self.eden.get_thrift_client() as client:
+            client.resetParentCommits(mountPoint=bytes(mount_path), parents=parents)
+
+        with self.assertRaises(hgrepo.HgError) as status_context:
+            self.repo.status()
+
+        self.assertIn(
+            b"requested parent commit is out-of-date", status_context.exception.stderr
+        )
+
+        # hg whereami reads eden's SNAPSHOT file
+        eden_parent = self.hg("whereami").strip("\n")
+        hg_parent = self.hg("log", "-r.", "-T{node}")
+
+        # make sure that eden and mercurial are out of sync
+        self.assertNotEqual(eden_parent, hg_parent)
+
+        cmd_result = self.eden.run_unchecked("doctor", "-n", stdout=subprocess.PIPE)
+        self.assertIn(b"Eden's snapshot file points to a bad commit", cmd_result.stdout)
+
+        # run eden doctor and make sure eden and mercurial are in sync again
+        fixed_result = self.eden.run_unchecked("doctor", stdout=subprocess.PIPE)
+        self.assertIn(b"Successfully fixed 1 problem", fixed_result.stdout)
+
+        eden_parent_fixed = self.hg("whereami").strip("\n")
+        hg_parent_fixed = self.hg("log", "-r.", "-T{node}")
+        self.assertEqual(eden_parent_fixed, hg_parent_fixed)
+
+        # Since Eden's snapshot file pointed to a bad commit, it should pick
+        # mercurial's parent as the new parent
+        self.assertEqual(hg_parent, hg_parent_fixed)