sync: verify fetchcmd postconditions without reading objects

After repo.fetchcmd exits, _CustomFetch verified the tracking ref and
FETCH_HEAD with ResolveCommit, i.e. `git rev-parse --verify
<ref>^{commit}`. Peeling to a commit makes Git look the object up: it
opens every pack index in objects/pack/ until it finds the commit, then
reads the commit from its packfile.

fetchcmd exists for object stores that are populated lazily, such as
ABFS, whose `abfs git fetch` registers packfiles as virtual files and
returns at once. There the peel is the first read of the packs the
command registered, and it blocks until they are downloaded. A project
whose revision did not change needs no objects to sync, yet paid that
download whenever fetchcmd registered new packs for it.

Compare the object IDs that the refs record instead (ResolveRef, which
runs the same rev-parse without the peel). Only when they differ peel
both sides to commits, so an annotated tag still matches the commit it
points to. A commit missing from the object store now fails the checkout
that reads it instead of the network half.

On a first sync the checkout reads the commit anyway, so there the
download only moves to the next command that needs the object.

Bug: 543101585
Change-Id: I51248ff1e9674b16e3ff8cee69572dbc8c7a8e99
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/643003
Commit-Queue: Gavin Mak <gavinmak@google.com>
Tested-by: Gavin Mak <gavinmak@google.com>
Reviewed-by: Brian Gan <brgan@google.com>

docs/fetch-cmd.md

@@ -42,13 +42,13 @@ postconditions to be met:
 
 1.  When the manifest revision names a branch or tag, the mapped local tracking
     ref (e.g. `refs/remotes/REPO_REMOTE/<branch>` for a branch, or
-    `refs/tags/<tag>` for a tag) must resolve to a valid commit in the object
-    store (`git rev-parse --verify <ref>^{commit}`), and must equal `REPO_TREV`
-    if `REPO_TREV` was non-empty.
-2.  `FETCH_HEAD` must resolve to the same target commit in the object store
-    (`git rev-parse --verify FETCH_HEAD^{commit}`).
-3.  The commit graph from the target commit must be reachable far enough to
-    compute merge bases with local branches.
+    `refs/tags/<tag>` for a tag) must name the target commit, and must equal
+    `REPO_TREV` if `REPO_TREV` was non-empty. A tag ref may name an annotated
+    tag of the target commit.
+2.  `FETCH_HEAD` must name the same target commit.
+3.  The target commit, and the commit graph from it far enough to compute
+    merge bases with local branches, must be readable from the object store.
+    They may be materialized lazily, when first read.
 
 ### Invariants
 
@@ -66,5 +66,10 @@ postconditions to be met:
 
 *   A non-zero exit status aborts the project's sync, and the command's stderr
     is surfaced to the user.
-*   `repo` verifies the tracking ref and target reachability after exit 0. Any
-    mismatch is treated as a failure.
+*   After exit 0, `repo` checks postconditions 1 and 2 by comparing the object
+    IDs that the refs record (`git rev-parse --verify <ref>`), so it does not
+    read the object store during the network half of the sync. It peels the
+    IDs to commits only when they differ, as for an annotated tag. Any mismatch
+    is treated as a failure.
+*   A target commit missing from the object store fails the checkout that reads
+    it.

project.py

@@ -3444,7 +3444,10 @@ class Project:
             finally:
                 self._RefreshConfigIfModified(config_stat)
 
-        # Verify postconditions.
+        # Verify postconditions against the object IDs that the refs record.
+        # Peeling them would read the object store, and on one that is
+        # populated lazily that downloads the objects fetchcmd just registered
+        # before anything needs them. The checkout reads them when it does.
         if not git_config.IsId(self.revisionExpr):
             try:
                 ref_name = self.GetRemote().ToLocal(self.revisionExpr)
@@ -3452,15 +3455,13 @@ class Project:
                 logger.error("error: Failed to resolve tracking ref: %s", e)
                 return False
             try:
-                resolved_ref = self.bare_git.ResolveCommit(ref_name)
+                resolved_ref = self.bare_git.ResolveRef(ref_name)
             except error.GitError:
                 logger.error(
-                    "error: Postcondition failed: %s does not resolve to a "
-                    "commit in object store",
-                    ref_name,
+                    "error: Postcondition failed: %s not found", ref_name
                 )
                 return False
-            if repo_trev and resolved_ref != repo_trev:
+            if repo_trev and not self._IsSameCommit(resolved_ref, repo_trev):
                 logger.error(
                     "error: Postcondition failed: %s is %s, expected %s",
                     ref_name,
@@ -3468,27 +3469,40 @@ class Project:
                     repo_trev,
                 )
                 return False
-            repo_trev = resolved_ref
+            if not repo_trev:
+                repo_trev = resolved_ref
 
         try:
-            fetch_head = self.bare_git.ResolveCommit("FETCH_HEAD")
-            if fetch_head != repo_trev:
-                logger.error(
-                    "error: Postcondition failed: FETCH_HEAD is %s, "
-                    "expected %s",
-                    fetch_head,
-                    repo_trev,
-                )
-                return False
+            fetch_head = self.bare_git.ResolveRef("FETCH_HEAD")
         except error.GitError:
+            logger.error("error: Postcondition failed: FETCH_HEAD not found")
+            return False
+        if not self._IsSameCommit(fetch_head, repo_trev):
             logger.error(
-                "error: Postcondition failed: FETCH_HEAD does not resolve to "
-                "a commit in object store"
+                "error: Postcondition failed: FETCH_HEAD is %s, expected %s",
+                fetch_head,
+                repo_trev,
             )
             return False
 
         return True
 
+    def _IsSameCommit(self, oid: str, expected: str) -> bool:
+        """Whether object ID |oid| names the same commit as |expected|.
+
+        Equal IDs match without reading the object store. Different IDs
+        still match when both peel to the same commit, as when one of them
+        names an annotated tag.
+        """
+        if oid == expected:
+            return True
+        try:
+            return self.bare_git.ResolveCommit(
+                oid
+            ) == self.bare_git.ResolveCommit(expected)
+        except error.GitError:
+            return False
+
     def _GetUpstreamFallback(self) -> Optional[str]:
         """Resolve a fallback upstream branch when revisionExpr is a SHA-1.
 
@@ -5465,8 +5479,13 @@ class Project:
             except (OSError, AssertionError):
                 return True
 
-        def ResolveCommit(self, revision: str) -> str:
-            """Resolve |revision| to a commit without option ambiguity."""
+        def ResolveRef(self, revision: str) -> str:
+            """Resolve |revision| to the object ID it names, without peeling.
+
+            Unlike ResolveCommit, this neither peels |revision| nor checks
+            that its object exists, so a ref name or a full object ID resolves
+            without reading the object store.
+            """
             cmdv = ["--verify", "--quiet"]
             if git_command.git_require((2, 30, 0)):
                 cmdv.append("--end-of-options")
@@ -5475,9 +5494,13 @@ class Project:
                     f"invalid revision: {revision}",
                     project=self._project.name,
                 )
-            cmdv.append(f"{revision}^{{commit}}")
+            cmdv.append(revision)
             return self.rev_parse(*cmdv, log_as_error=False)
 
+        def ResolveCommit(self, revision: str) -> str:
+            """Resolve |revision| to a commit without option ambiguity."""
+            return self.ResolveRef(f"{revision}^{{commit}}")
+
         def SetHead(self, ref, message=None):
             cmdv = []
             if message is not None:

tests/test_project.py

@@ -529,6 +529,43 @@ class ProjectTests(unittest.TestCase):
             with self.assertRaises(error.GitError):
                 git.ResolveCommit("--not-a-revision")
 
+    def test_resolve_ref_does_not_peel(self) -> None:
+        """Ref resolution verifies the name without peeling it to a commit."""
+        proj = mock.MagicMock(name="project")
+        proj.name = "project"
+        git = project.Project._GitGetByExec(proj, bare=True, gitdir="gitdir")
+        command = mock.MagicMock()
+        command.stdout = "1" * 40 + "\n"
+        command.Wait.return_value = 0
+
+        with mock.patch.object(
+            git_command, "git_require", return_value=True
+        ), mock.patch.object(
+            git_command, "GitCommand", return_value=command
+        ) as cmd:
+            self.assertEqual("1" * 40, git.ResolveRef("FETCH_HEAD"))
+
+        cmd.assert_called_once_with(
+            proj,
+            [
+                "rev-parse",
+                "--verify",
+                "--quiet",
+                "--end-of-options",
+                "FETCH_HEAD",
+            ],
+            bare=True,
+            gitdir="gitdir",
+            capture_stdout=True,
+            capture_stderr=True,
+            verify_command=True,
+            log_as_error=False,
+        )
+
+        with mock.patch.object(git_command, "git_require", return_value=False):
+            with self.assertRaises(error.GitError):
+                git.ResolveRef("--not-a-revision")
+
     def test_get_branches_reuses_ref_snapshot_for_current_branch(self) -> None:
         """GetBranches derives the current branch from the loaded refs."""
         with utils_for_test.TempGitTree() as tempdir:
@@ -3438,6 +3475,8 @@ class FetchCmdTests(unittest.TestCase):
             tempdir, use_local_gitdirs=True, fetch_cmd="echo hi"
         )
         proj.GetRevisionId = mock.MagicMock(return_value="1234abcd")
+        # Every ID names a commit unless a test says otherwise.
+        proj.bare_git.ResolveCommit.side_effect = lambda rev: rev
         return proj
 
     def test_fetch_cmd_execution_unpinned_branch(self) -> None:
@@ -3446,7 +3485,7 @@ class FetchCmdTests(unittest.TestCase):
             proj = self._get_project(tempdir)
             proj.revisionId = None
             proj.revisionExpr = "main"
-            proj.bare_git.ResolveCommit.return_value = "1234abcd"
+            proj.bare_git.ResolveRef.return_value = "1234abcd"
             mock_remote = mock.MagicMock()
             mock_remote.ToLocal.return_value = "refs/remotes/origin/main"
             proj.GetRemote = mock.MagicMock(return_value=mock_remote)
@@ -3468,10 +3507,9 @@ class FetchCmdTests(unittest.TestCase):
                 kwargs["env"]["REPO_PROJECT_FETCH_URL"],
                 "http://example.com/repo",
             )
-            proj.bare_git.ResolveCommit.assert_any_call(
-                "refs/remotes/origin/main"
-            )
-            proj.bare_git.ResolveCommit.assert_any_call("FETCH_HEAD")
+            proj.bare_git.ResolveRef.assert_any_call("refs/remotes/origin/main")
+            proj.bare_git.ResolveRef.assert_any_call("FETCH_HEAD")
+            proj.bare_git.ResolveCommit.assert_not_called()
 
     def test_fetch_cmd_execution_pinned_revision_id(self) -> None:
         """Test fetch_cmd populates REPO_TREV when revisionId is set."""
@@ -3480,7 +3518,7 @@ class FetchCmdTests(unittest.TestCase):
             proj = self._get_project(tempdir)
             proj.revisionId = sha
             proj.revisionExpr = "main"
-            proj.bare_git.ResolveCommit.return_value = sha
+            proj.bare_git.ResolveRef.return_value = sha
             mock_remote = mock.MagicMock()
             mock_remote.ToLocal.return_value = "refs/remotes/origin/main"
             proj.GetRemote = mock.MagicMock(return_value=mock_remote)
@@ -3493,6 +3531,7 @@ class FetchCmdTests(unittest.TestCase):
             proj._LsRemote.assert_not_called()
             _, kwargs = mock_run.call_args
             self.assertEqual(kwargs["env"]["REPO_TREV"], sha)
+            proj.bare_git.ResolveCommit.assert_not_called()
 
     def test_fetch_cmd_execution_pinned_revision_expr_sha(self) -> None:
         """Test fetch_cmd populates REPO_TREV when revisionExpr is a SHA."""
@@ -3501,7 +3540,7 @@ class FetchCmdTests(unittest.TestCase):
             proj = self._get_project(tempdir)
             proj.revisionId = None
             proj.revisionExpr = sha
-            proj.bare_git.ResolveCommit.return_value = sha
+            proj.bare_git.ResolveRef.return_value = sha
             proj.GetRemote = mock.MagicMock()
 
             with mock.patch("subprocess.run") as mock_run:
@@ -3513,7 +3552,8 @@ class FetchCmdTests(unittest.TestCase):
             proj.GetRemote.assert_not_called()
             _, kwargs = mock_run.call_args
             self.assertEqual(kwargs["env"]["REPO_TREV"], sha)
-            proj.bare_git.ResolveCommit.assert_called_once_with("FETCH_HEAD")
+            proj.bare_git.ResolveRef.assert_called_once_with("FETCH_HEAD")
+            proj.bare_git.ResolveCommit.assert_not_called()
 
     def test_fetch_cmd_postcondition_fails_when_fetch_head_mismatches_ref(
         self,
@@ -3523,7 +3563,7 @@ class FetchCmdTests(unittest.TestCase):
             proj = self._get_project(tempdir)
             proj.revisionId = None
             proj.revisionExpr = "main"
-            proj.bare_git.ResolveCommit.side_effect = [
+            proj.bare_git.ResolveRef.side_effect = [
                 "11111111" * 5,
                 "22222222" * 5,
             ]
@@ -3543,7 +3583,7 @@ class FetchCmdTests(unittest.TestCase):
             proj = self._get_project(tempdir)
             proj.revisionId = "11111111" * 5
             proj.revisionExpr = "main"
-            proj.bare_git.ResolveCommit.return_value = "22222222" * 5
+            proj.bare_git.ResolveRef.return_value = "22222222" * 5
             mock_remote = mock.MagicMock()
             mock_remote.ToLocal.return_value = "refs/remotes/origin/main"
             proj.GetRemote = mock.MagicMock(return_value=mock_remote)
@@ -3558,7 +3598,7 @@ class FetchCmdTests(unittest.TestCase):
             proj = self._get_project(tempdir)
             proj.revisionId = None
             proj.revisionExpr = "main"
-            proj.bare_git.ResolveCommit.side_effect = error.GitError("missing")
+            proj.bare_git.ResolveRef.side_effect = error.GitError("missing")
             mock_remote = mock.MagicMock()
             mock_remote.ToLocal.return_value = "refs/remotes/origin/main"
             proj.GetRemote = mock.MagicMock(return_value=mock_remote)
@@ -3567,6 +3607,70 @@ class FetchCmdTests(unittest.TestCase):
                 mock_run.return_value = mock.MagicMock(returncode=0, stderr="")
                 self.assertFalse(proj._CustomFetch())
 
+    def test_fetch_cmd_postcondition_fails_when_fetch_head_missing(
+        self,
+    ) -> None:
+        """Test _CustomFetch fails when FETCH_HEAD does not resolve."""
+        with utils_for_test.TempGitTree() as tempdir:
+            sha = "0123456789abcdef0123456789abcdef01234567"
+            proj = self._get_project(tempdir)
+            proj.revisionId = sha
+            proj.revisionExpr = sha
+            proj.bare_git.ResolveRef.side_effect = error.GitError("missing")
+
+            with mock.patch("subprocess.run") as mock_run:
+                mock_run.return_value = mock.MagicMock(returncode=0, stderr="")
+                self.assertFalse(proj._CustomFetch())
+
+    def test_fetch_cmd_postcondition_peels_annotated_tag(self) -> None:
+        """Test an annotated tag matches REPO_TREV once peeled."""
+        with utils_for_test.TempGitTree() as tempdir:
+            commit = "11111111" * 5
+            tag = "aaaaaaaa" * 5
+            proj = self._get_project(tempdir)
+            proj.revisionId = commit
+            proj.revisionExpr = "refs/tags/v1.0"
+            proj.bare_git.ResolveRef.return_value = tag
+            proj.bare_git.ResolveCommit.side_effect = {
+                tag: commit,
+                commit: commit,
+            }.get
+            mock_remote = mock.MagicMock()
+            mock_remote.ToLocal.return_value = "refs/tags/v1.0"
+            proj.GetRemote = mock.MagicMock(return_value=mock_remote)
+
+            with mock.patch("subprocess.run") as mock_run:
+                mock_run.return_value = mock.MagicMock(returncode=0, stderr="")
+                self.assertTrue(proj._CustomFetch())
+
+            proj.bare_git.ResolveRef.assert_any_call("refs/tags/v1.0")
+            proj.bare_git.ResolveRef.assert_any_call("FETCH_HEAD")
+            proj.bare_git.ResolveCommit.assert_any_call(tag)
+
+    def test_fetch_cmd_postcondition_fails_when_tag_peels_elsewhere(
+        self,
+    ) -> None:
+        """Test an annotated tag of another commit fails the postcondition."""
+        with utils_for_test.TempGitTree() as tempdir:
+            commit = "11111111" * 5
+            other = "22222222" * 5
+            tag = "aaaaaaaa" * 5
+            proj = self._get_project(tempdir)
+            proj.revisionId = commit
+            proj.revisionExpr = "refs/tags/v1.0"
+            proj.bare_git.ResolveRef.return_value = tag
+            proj.bare_git.ResolveCommit.side_effect = {
+                tag: other,
+                commit: commit,
+            }.get
+            mock_remote = mock.MagicMock()
+            mock_remote.ToLocal.return_value = "refs/tags/v1.0"
+            proj.GetRemote = mock.MagicMock(return_value=mock_remote)
+
+            with mock.patch("subprocess.run") as mock_run:
+                mock_run.return_value = mock.MagicMock(returncode=0, stderr="")
+                self.assertFalse(proj._CustomFetch())
+
     def test_fetch_cmd_refreshes_config_cache_when_modified(self) -> None:
         """Test _CustomFetch refreshes GitConfig and remotes on config edit."""
         with utils_for_test.TempGitTree() as tempdir:
@@ -3589,7 +3693,7 @@ class FetchCmdTests(unittest.TestCase):
             )
             proj.revisionId = sha
             proj.revisionExpr = "main"
-            proj.bare_git.ResolveCommit.return_value = sha
+            proj.bare_git.ResolveRef.return_value = sha
 
             def _edit_config(*_args: Any, **_kwargs: Any) -> mock.MagicMock:
                 with open(proj.config.file, "w", encoding="utf-8") as fp:
@@ -3681,7 +3785,7 @@ class FetchCmdTests(unittest.TestCase):
             proj = self._get_project(tempdir)
             proj.revisionId = None
             proj.revisionExpr = "main"
-            proj.bare_git.ResolveCommit.return_value = sha
+            proj.bare_git.ResolveRef.return_value = sha
             mock_remote = mock.MagicMock()
             mock_remote.ToLocal.return_value = "refs/remotes/origin/main"
             proj.GetRemote = mock.MagicMock(return_value=mock_remote)
@@ -3741,6 +3845,137 @@ class FetchCmdTests(unittest.TestCase):
             self.assertFalse(fakeproj.UseFetchCmd)
 
 
+class FetchCmdGitTests(unittest.TestCase):
+    """Tests running the fetchcmd contract against real Git repositories."""
+
+    @staticmethod
+    def _git(cwd: str, *args: str) -> str:
+        return subprocess.run(
+            ["git", "-C", cwd] + list(args),
+            check=True,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
+            universal_newlines=True,
+        ).stdout.rstrip("\n")
+
+    def _make_project(
+        self,
+        topdir: str,
+        fetch_cmd: str,
+        revision_expr: str,
+        revision_id: Optional[str],
+    ) -> project.Project:
+        """Return a project under |topdir| that fetches with |fetch_cmd|."""
+        worktree = os.path.join(topdir, "proj")
+        os.mkdir(worktree)
+        self._git(worktree, "init", "-q")
+
+        manifest = mock.MagicMock()
+        manifest.manifestProject.use_local_gitdirs = True
+        manifest.manifestProject.fetch_cmd = fetch_cmd
+        manifest.UseLocalGitDirs = True
+        manifest.IsMirror = False
+        manifest.is_multimanifest = False
+        manifest.topdir = topdir
+        manifest.path_prefix = ""
+        manifest.globalConfig = None
+
+        remote_spec = mock.MagicMock()
+        remote_spec.name = "origin"
+        remote_spec.url = os.path.join(topdir, "remote")
+
+        return project.Project(
+            manifest=manifest,
+            name="proj",
+            remote=remote_spec,
+            gitdir=os.path.join(worktree, ".git"),
+            objdir=os.path.join(worktree, ".git"),
+            worktree=worktree,
+            relpath="proj",
+            revisionExpr=revision_expr,
+            revisionId=revision_id,
+        )
+
+    def test_postconditions_do_not_read_the_object_store(self) -> None:
+        """Test fetchcmd may leave the target commit to materialize lazily."""
+        with tempfile.TemporaryDirectory(prefix="repo-tests") as topdir:
+            # The commit is not in the object store yet, as on a filesystem
+            # that downloads objects when they are first read.
+            commit = "1234abcd" * 5
+            proj = self._make_project(
+                topdir,
+                f"echo {commit} > $REPO_PATH/.git/FETCH_HEAD",
+                revision_expr=commit,
+                revision_id=commit,
+            )
+
+            self.assertTrue(proj._CustomFetch())
+
+    def test_postconditions_peel_an_annotated_tag(self) -> None:
+        """Test a fetched annotated tag satisfies a pinned REPO_TREV."""
+        with tempfile.TemporaryDirectory(prefix="repo-tests") as topdir:
+            remote = os.path.join(topdir, "remote")
+            os.mkdir(remote)
+            self._git(remote, "init", "-q")
+            with open(os.path.join(remote, "a.txt"), "w") as fp:
+                fp.write("a\n")
+            self._git(remote, "add", "a.txt")
+            ident = [
+                "-c",
+                "user.name=Test",
+                "-c",
+                "user.email=test@example.com",
+            ]
+            self._git(remote, *ident, "commit", "-q", "-m", "one")
+            self._git(remote, *ident, "tag", "-a", "-m", "release", "v1.0")
+            commit = self._git(remote, "rev-parse", "HEAD")
+            proj = self._make_project(
+                topdir,
+                f"git -C $REPO_PATH fetch -q {remote} "
+                "+refs/tags/v1.0:refs/tags/v1.0",
+                revision_expr="refs/tags/v1.0",
+                revision_id=commit,
+            )
+
+            self.assertTrue(proj._CustomFetch())
+            # The ref and FETCH_HEAD name the tag object, not the commit.
+            tag = self._git(proj.worktree, "rev-parse", "refs/tags/v1.0")
+            self.assertNotEqual(tag, commit)
+            self.assertEqual(
+                self._git(proj.worktree, "rev-parse", "FETCH_HEAD"), tag
+            )
+
+    def test_postconditions_reject_a_tag_of_another_commit(self) -> None:
+        """Test an annotated tag of another commit fails a pinned REPO_TREV."""
+        with tempfile.TemporaryDirectory(prefix="repo-tests") as topdir:
+            remote = os.path.join(topdir, "remote")
+            os.mkdir(remote)
+            self._git(remote, "init", "-q")
+            ident = [
+                "-c",
+                "user.name=Test",
+                "-c",
+                "user.email=test@example.com",
+            ]
+            self._git(
+                remote, *ident, "commit", "-q", "--allow-empty", "-m", "1"
+            )
+            pinned = self._git(remote, "rev-parse", "HEAD")
+            self._git(
+                remote, *ident, "commit", "-q", "--allow-empty", "-m", "2"
+            )
+            self._git(remote, *ident, "tag", "-a", "-m", "release", "v1.0")
+            proj = self._make_project(
+                topdir,
+                f"git -C $REPO_PATH fetch -q {remote} "
+                "+refs/tags/v1.0:refs/tags/v1.0",
+                revision_expr="refs/tags/v1.0",
+                revision_id=pinned,
+            )
+
+            self.assertFalse(proj._CustomFetch())
+
+
 class ReprojectCmdTests(unittest.TestCase):
     """Tests for the repo.reprojectcmd feature."""