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."""