From 616396317c4f0ab0a1e71a2dd3ad49a43cd7d777 Mon Sep 17 00:00:00 2001 From: Gavin Mak Date: Mon, 12 Oct 2026 18:25:42 +0000 Subject: [PATCH] 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 ^{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 Tested-by: Gavin Mak Reviewed-by: Brian Gan --- docs/fetch-cmd.md | 23 ++-- project.py | 65 +++++++---- tests/test_project.py | 261 +++++++++++++++++++++++++++++++++++++++--- 3 files changed, 306 insertions(+), 43 deletions(-) diff --git a/docs/fetch-cmd.md b/docs/fetch-cmd.md index 2378833f4..1034cbb43 100644 --- a/docs/fetch-cmd.md +++ b/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/` for a branch, or - `refs/tags/` for a tag) must resolve to a valid commit in the object - store (`git rev-parse --verify ^{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/` 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 `), 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. diff --git a/project.py b/project.py index 4d0bd3d41..ffa56a7c7 100644 --- a/project.py +++ b/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: diff --git a/tests/test_project.py b/tests/test_project.py index 3d848838c..60d01ff39 100644 --- a/tests/test_project.py +++ b/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."""