From 2e8a3805bb016bcaa7ca422fffbb1da22dd00605 Mon Sep 17 00:00:00 2001 From: Gavin Mak Date: Wed, 7 Oct 2026 21:40:19 +0000 Subject: [PATCH] sync: don't run git ls-remote before fetchcmd on unpinned refs When a project's revision is an unpinned branch or tag and superprojects are not used (so self.revisionId is unset), _CustomFetch previously ran git ls-remote against the Git remote before invoking repo.fetchcmd just to pre-populate REPO_TREV. This added a per-project network round-trip before fetchcmd and forced branch resolution against the Git remote rather than letting fetchcmd resolve REPO_RREV from its own server (just as standard git fetch does in _RemoteFetch). Only populate REPO_TREV before fetchcmd when the target commit SHA is already known (via self.revisionId from superproject or a SHA-pinned revisionExpr). When REPO_RREV is an unpinned branch or tag, leave REPO_TREV empty, let fetchcmd resolve REPO_RREV and update the local tracking ref and FETCH_HEAD, and verify afterward that the tracking ref and FETCH_HEAD resolve to the same valid commit in the object store. Bug: 543101585 Change-Id: I482bf4c022d0e669e5620d4517581ac193983da0 Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/640423 Reviewed-by: Brian Gan Commit-Queue: Gavin Mak Tested-by: Gavin Mak --- docs/fetch-cmd.md | 32 ++++++------ project.py | 70 ++++++++++++-------------- tests/test_project.py | 111 ++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 156 insertions(+), 57 deletions(-) diff --git a/docs/fetch-cmd.md b/docs/fetch-cmd.md index b125ed344..2378833f4 100644 --- a/docs/fetch-cmd.md +++ b/docs/fetch-cmd.md @@ -28,7 +28,10 @@ Environment section in `repo help forall` or `subcmds/forall.py`. The following environment variable is specific to `repo.fetchcmd`: -* `REPO_TREV`: The target revision resolved to a full commit hash. +* `REPO_TREV`: The target commit hash when the revision is already pinned to + a commit (e.g. via a superproject or a commit SHA in the manifest), or + empty when `REPO_RREV` is an unpinned branch or tag to be resolved by the + fetch command. ## Contract @@ -37,22 +40,23 @@ The following environment variable is specific to `repo.fetchcmd`: After the fetch command exits with status 0, `repo` expects the following postconditions to be met: -1. `git cat-file -e REPO_TREV` succeeds (the commit must exist in the object - store). -2. The mapped local tracking ref (e.g. `refs/remotes/REPO_REMOTE/` - for a branch revision, or the tag ref itself for a tag) must point to - `REPO_TREV`. -3. `FETCH_HEAD` must point to `REPO_TREV`. -4. The commit graph from `REPO_TREV` must be reachable far enough to compute - merge bases with local branches. +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. ### Invariants -* The command should be idempotent; fetching the same `REPO_TREV` twice should - be a no-op. -* Only `FETCH_HEAD` and `refs/remotes/*` should be modified to preserve - `repo sync --network-only` semantics. `HEAD` and local branches must not be - touched by the fetch command. +* The command should be idempotent; repeating the same fetch should be a + no-op. +* Only `FETCH_HEAD`, `refs/remotes/*`, and `refs/tags/*` should be modified to + preserve `repo sync --network-only` semantics. `HEAD` and local branches + must not be touched by the fetch command. * Dirty worktree state must be preserved. * The command is **not** executed for `MetaProject`s (i.e. the internal `repo` repository itself at `.repo/repo` and the `manifests` repository at diff --git a/project.py b/project.py index 2c1958e7e..b97143139 100644 --- a/project.py +++ b/project.py @@ -3228,7 +3228,8 @@ class Project: """Fetch using a custom command specified by repo.fetchcmd. Populates the subshell with project-context environment variables, - including REPO_TREV (target revision resolved to a commit hash). + including REPO_TREV when the target revision is already resolved to a + commit hash. Expects the target commit to be reachable and tracking refs/FETCH_HEAD to be updated upon a successful 0 exit code. @@ -3236,27 +3237,20 @@ class Project: For a detailed contract, environment variables, and postconditions, see docs/fetch-cmd.md. """ - # Resolve REPO_TREV (target revision resolved to a full commit hash). + # Populate REPO_TREV when the target commit hash is already known + # (e.g. from a superproject or a SHA-pinned manifest revision). When the + # manifest names an unpinned branch or tag, leave REPO_TREV empty and + # let fetchcmd resolve REPO_RREV from its remote without a separate + # git ls-remote round-trip. repo_trev = None if self.revisionId and IsId(self.revisionId): repo_trev = self.revisionId - - if not repo_trev: - output = self._LsRemote(self.upstream or self.revisionExpr) - if output: - lines = output.splitlines() - if lines: - parts = lines[0].split() - if parts: - repo_trev = parts[0] - - if not repo_trev: - logger.error("error: Cannot resolve REPO_TREV for %s", self.name) - return False + elif self.revisionExpr and IsId(self.revisionExpr): + repo_trev = self.revisionExpr env = os.environ.copy() env.update(self.GetEnvVars()) - env["REPO_TREV"] = repo_trev + env["REPO_TREV"] = repo_trev or "" cmd_str = self.manifest.manifestProject.fetch_cmd if verbose: @@ -3288,23 +3282,22 @@ class Project: return False # Verify postconditions. - try: - self.bare_git.cat_file("-e", repo_trev) - except GitError: - logger.error( - "error: Postcondition failed: %s not found in object store", - repo_trev, - ) - return False - - try: - ref_name = self.GetRemote().ToLocal(self.revisionExpr) - except GitError as e: - logger.error("error: Failed to resolve tracking ref: %s", e) - return False - try: - resolved_ref = self.bare_git.rev_parse(ref_name) - if resolved_ref != repo_trev: + if not IsId(self.revisionExpr): + try: + ref_name = self.GetRemote().ToLocal(self.revisionExpr) + except GitError as e: + logger.error("error: Failed to resolve tracking ref: %s", e) + return False + try: + resolved_ref = self.bare_git.ResolveCommit(ref_name) + except GitError: + logger.error( + "error: Postcondition failed: %s does not resolve to a " + "commit in object store", + ref_name, + ) + return False + if repo_trev and resolved_ref != repo_trev: logger.error( "error: Postcondition failed: %s is %s, expected %s", ref_name, @@ -3312,12 +3305,10 @@ class Project: repo_trev, ) return False - except GitError: - logger.error("error: Postcondition failed: %s not found", ref_name) - return False + repo_trev = resolved_ref try: - fetch_head = self.bare_git.rev_parse("FETCH_HEAD") + fetch_head = self.bare_git.ResolveCommit("FETCH_HEAD") if fetch_head != repo_trev: logger.error( "error: Postcondition failed: FETCH_HEAD is %s, " @@ -3327,7 +3318,10 @@ class Project: ) return False except GitError: - logger.error("error: Postcondition failed: FETCH_HEAD not found") + logger.error( + "error: Postcondition failed: FETCH_HEAD does not resolve to " + "a commit in object store" + ) return False return True diff --git a/tests/test_project.py b/tests/test_project.py index 5903acabd..18dc47afb 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -3307,12 +3307,13 @@ class FetchCmdTests(unittest.TestCase): proj.GetRevisionId = mock.MagicMock(return_value="1234abcd") return proj - def test_fetch_cmd_execution(self): - """Test that fetch_cmd is executed with correct environment.""" + def test_fetch_cmd_execution_unpinned_branch(self) -> None: + """Test fetch_cmd with an unpinned branch does not call ls-remote.""" with utils_for_test.TempGitTree() as tempdir: proj = self._get_project(tempdir) - - proj.bare_git.rev_parse.return_value = "1234abcd" + proj.revisionId = None + proj.revisionExpr = "main" + proj.bare_git.ResolveCommit.return_value = "1234abcd" mock_remote = mock.MagicMock() mock_remote.ToLocal.return_value = "refs/remotes/origin/main" proj.GetRemote = mock.MagicMock(return_value=mock_remote) @@ -3323,15 +3324,115 @@ class FetchCmdTests(unittest.TestCase): self.assertTrue(res) mock_run.assert_called_once() + proj._LsRemote.assert_not_called() args, kwargs = mock_run.call_args self.assertEqual(args[0], "echo hi") self.assertEqual(kwargs["shell"], True) self.assertEqual(kwargs["cwd"], tempdir) - self.assertEqual(kwargs["env"]["REPO_TREV"], "1234abcd") + self.assertEqual(kwargs["env"]["REPO_TREV"], "") + self.assertEqual(kwargs["env"]["REPO_RREV"], "main") self.assertEqual( 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") + + def test_fetch_cmd_execution_pinned_revision_id(self) -> None: + """Test fetch_cmd populates REPO_TREV when revisionId is set.""" + with utils_for_test.TempGitTree() as tempdir: + sha = "0123456789abcdef0123456789abcdef01234567" + proj = self._get_project(tempdir) + proj.revisionId = sha + proj.revisionExpr = "main" + proj.bare_git.ResolveCommit.return_value = sha + mock_remote = mock.MagicMock() + mock_remote.ToLocal.return_value = "refs/remotes/origin/main" + 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="") + res = proj._CustomFetch() + + self.assertTrue(res) + proj._LsRemote.assert_not_called() + _, kwargs = mock_run.call_args + self.assertEqual(kwargs["env"]["REPO_TREV"], sha) + + def test_fetch_cmd_execution_pinned_revision_expr_sha(self) -> None: + """Test fetch_cmd populates REPO_TREV when revisionExpr is a SHA.""" + with utils_for_test.TempGitTree() as tempdir: + sha = "0123456789abcdef0123456789abcdef01234567" + proj = self._get_project(tempdir) + proj.revisionId = None + proj.revisionExpr = sha + proj.bare_git.ResolveCommit.return_value = sha + proj.GetRemote = mock.MagicMock() + + with mock.patch("subprocess.run") as mock_run: + mock_run.return_value = mock.MagicMock(returncode=0, stderr="") + res = proj._CustomFetch() + + self.assertTrue(res) + proj._LsRemote.assert_not_called() + 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") + + def test_fetch_cmd_postcondition_fails_when_fetch_head_mismatches_ref( + self, + ) -> None: + """Test _CustomFetch fails when FETCH_HEAD disagrees with the ref.""" + with utils_for_test.TempGitTree() as tempdir: + proj = self._get_project(tempdir) + proj.revisionId = None + proj.revisionExpr = "main" + proj.bare_git.ResolveCommit.side_effect = [ + "11111111" * 5, + "22222222" * 5, + ] + mock_remote = mock.MagicMock() + mock_remote.ToLocal.return_value = "refs/remotes/origin/main" + 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_postcondition_fails_when_ref_mismatches_pinned_trev( + self, + ) -> None: + """Test _CustomFetch fails when the ref disagrees with REPO_TREV.""" + with utils_for_test.TempGitTree() as tempdir: + proj = self._get_project(tempdir) + proj.revisionId = "11111111" * 5 + proj.revisionExpr = "main" + proj.bare_git.ResolveCommit.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) + + 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_fails_when_ref_missing(self) -> None: + """Test _CustomFetch fails when tracking ref or FETCH_HEAD fails.""" + with utils_for_test.TempGitTree() as tempdir: + proj = self._get_project(tempdir) + proj.revisionId = None + proj.revisionExpr = "main" + proj.bare_git.ResolveCommit.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) + + with mock.patch("subprocess.run") as mock_run: + mock_run.return_value = mock.MagicMock(returncode=0, stderr="") + self.assertFalse(proj._CustomFetch()) def test_sync_fetch_cmd_requires_use_local_gitdirs(self): """Test that fetch_cmd requires use_local_gitdirs."""