From b16e5f49d6f95a2be02ab9c8e43c2d13ca6f4033 Mon Sep 17 00:00:00 2001 From: hungweichen Date: Sat, 24 Oct 2026 18:54:00 +0000 Subject: [PATCH] git_refs, project: skip redundant git calls on unborn HEAD and warm refs Read plain files-backend unborn HEAD symrefs directly before spawning git symbolic-ref, update already-loaded bare_ref snapshots in _InitAnyMRef when no external ref changes occurred, and reuse warm bare_ref snapshots in GetRevisionId for non-tag refs. Also skip redundant rev-parse and empty-index ls-files subprocesses in _Reproject when HEAD is guaranteed to be an unborn local ref. Bug: 564817989 Change-Id: I0da10cefb14753ce6e1e22e192d652790d59a5b0 Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/649605 Tested-by: Gavin Mak Reviewed-by: Gavin Mak Commit-Queue: Gavin Mak --- git_refs.py | 139 ++++++++++++++++++++++++++++++---- project.py | 109 +++++++++++++++++++++----- tests/test_git_refs.py | 162 +++++++++++++++++++++++++++++++++++++++ tests/test_project.py | 168 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 542 insertions(+), 36 deletions(-) diff --git a/git_refs.py b/git_refs.py index 4575f9740..70b20139d 100644 --- a/git_refs.py +++ b/git_refs.py @@ -27,6 +27,8 @@ R_PUB = "refs/published/" R_WORKTREE = "refs/worktree/" R_WORKTREE_M = R_WORKTREE + "m/" R_M = "refs/remotes/m/" +# Maximum symbolic-ref indirection depth (matches Git SYMREF_MAXDEPTH). +_SYMREF_MAXDEPTH = 5 class GitRefs: @@ -76,6 +78,101 @@ class GitRefs: except KeyError: return "" + def UpdateSymref(self, name: str, target: str) -> None: + """Update a symbolic reference in an already-loaded ref snapshot. + + Args: + name: Full symbolic reference name (e.g. "refs/remotes/m/main"). + target: Target reference name that the symref points to. + """ + if self._phyref is None: + return + if self._HasExternalRefChanges(name): + self._phyref = None + return + + self._symref[name] = target + self._ResolveSymrefs() + + parts = name.strip("/").split("/") + for idx in range(1, len(parts)): + anc = os.path.join(*parts[:idx]) + self._TrackMtime(anc) + self._TrackMtime(os.path.join(*parts)) + + def _HasExternalRefChanges(self, updated_symref: str) -> bool: + """Check whether refs outside |updated_symref| changed on disk. + + Args: + updated_symref: Symbolic ref path just updated on disk. + + Returns: + True if reftable is in use or any unrelated ref path changed. + """ + if os.path.exists(os.path.join(self._gitdir, "reftable")): + return True + if "packed-refs" not in self._mtime and os.path.exists( + os.path.join(self._gitdir, "packed-refs") + ): + return True + + parts = updated_symref.strip("/").split("/") + expected_child = { + "/".join(parts[:idx]): parts[idx] for idx in range(1, len(parts)) + } + + for tracked, mtime in self._mtime.items(): + norm = tracked.replace(os.sep, "/") + if norm == updated_symref: + continue + full_path = os.path.join(self._gitdir, tracked) + try: + curr_mtime = os.path.getmtime(full_path) + except OSError: + return True + if norm in expected_child: + if curr_mtime != mtime: + try: + children = platform_utils.listdir(full_path) + except OSError: + return True + next_comp = expected_child[norm] + for child in children: + if child == next_comp: + continue + if os.path.join(tracked, child) not in self._mtime: + return True + elif curr_mtime != mtime: + return True + + for anc, next_comp in expected_child.items(): + anc_os = anc.replace("/", os.sep) + if anc_os not in self._mtime: + anc_full = os.path.join(self._gitdir, anc_os) + try: + if platform_utils.listdir(anc_full) != [next_comp]: + return True + except OSError: + return True + return False + + def _ResolveSymrefs(self) -> None: + """Resolve all symbolic references into _phyref up to max depth.""" + for name in self._symref: + self._phyref.pop(name, None) + + scan = self._symref + attempts = 0 + while scan and attempts < _SYMREF_MAXDEPTH: + scan_next = {} + for name, dest in scan.items(): + if dest in self._phyref: + self._phyref[name] = self._phyref[dest] + else: + scan_next[name] = dest + scan = scan_next + attempts += 1 + def _EnsureLoaded(self): if self._phyref is None or self._NeedUpdate(): self._LoadAll() @@ -105,23 +202,17 @@ class GitRefs: # --include-root-refs does not report an unborn HEAD. self._ReadSymbolicRef(HEAD) - scan = self._symref - attempts = 0 - while scan and attempts < 5: - scan_next = {} - for name, dest in scan.items(): - if dest in self._phyref: - self._phyref[name] = self._phyref[dest] - else: - scan_next[name] = dest - scan = scan_next - attempts += 1 + self._ResolveSymrefs() + self._RefreshMtimes() - self._TrackMtime(HEAD) - self._TrackMtime("config") - self._TrackMtime("packed-refs") - self._TrackTreeMtimes("refs") - self._TrackTreeMtimes("reftable") + def _RefreshMtimes(self) -> None: + """Record current modification times for ref storage paths.""" + self._mtime = {} + self._TrackMtime(HEAD) + self._TrackMtime("config") + self._TrackMtime("packed-refs") + self._TrackTreeMtimes("refs") + self._TrackTreeMtimes("reftable") @staticmethod def _IsNullRef(ref_id: str) -> bool: @@ -165,6 +256,22 @@ class GitRefs: def _ReadSymbolicRef(self, name: str) -> None: """Read a symbolic reference.""" + if name == HEAD: + path = os.path.join(self._gitdir, HEAD) + try: + if not platform_utils.islink(path): + with open( + path, "r", encoding="utf-8", errors="replace" + ) as fd: + line = fd.readline().strip() + if line.startswith("ref:"): + ref = line[4:].strip() + if ref and ref != R_HEADS + ".invalid": + self._symref[name] = ref + return + except OSError: + pass + p = git_command.GitCommand( None, ["symbolic-ref", "-q", name], diff --git a/project.py b/project.py index 39d341405..5e0671ec3 100644 --- a/project.py +++ b/project.py @@ -2133,11 +2133,12 @@ class Project: # Tags may be annotated, in which case all_refs holds the tag object # SHA rather than the peeled commit SHA. Fall through to ResolveCommit # so GetRevisionId always returns a commit SHA. - if ( - all_refs is not None - and not rev.startswith(git_refs.R_TAGS) - and rev in all_refs - ): + is_tag = rev.startswith( + git_refs.R_TAGS + ) or self.revisionExpr.startswith(git_refs.R_TAGS) + if all_refs is None and not is_tag and self.bare_ref.is_loaded: + all_refs = self.bare_ref.all + if all_refs is not None and not is_tag and rev in all_refs: return all_refs[rev] try: @@ -4186,12 +4187,17 @@ class Project: project=self.name, ) - try: - head_tree = self.work_git.rev_parse( - "-q", "--verify", "HEAD^{tree}", log_as_error=False - ) - except error.GitError: + use_exec_work_git = isinstance(self.work_git, self._GitGetByExec) + unborn_head = use_exec_work_git and self.work_git.IsUnbornHead() + if unborn_head: head_tree = None + else: + try: + head_tree = self.work_git.rev_parse( + "-q", "--verify", "HEAD^{tree}", log_as_error=False + ) + except error.GitError: + head_tree = None if head_tree: p = git_command.GitCommand( @@ -4207,6 +4213,8 @@ class Project: project=self.name, ) staged = p.stdout.split("\0")[:-1] + elif use_exec_work_git and not self.work_git.HasIndexFile(): + staged = [] else: p = git_command.GitCommand( self, @@ -4230,12 +4238,15 @@ class Project: ) head = self.work_git.GetHead() - try: - head_oid = self.work_git.rev_parse( - "-q", "--verify", "HEAD", log_as_error=False - ) - except error.GitError: + if unborn_head: head_oid = None + else: + try: + head_oid = self.work_git.rev_parse( + "-q", "--verify", "HEAD", log_as_error=False + ) + except error.GitError: + head_oid = None env = os.environ.copy() env.update(self.GetEnvVars()) @@ -4276,12 +4287,15 @@ class Project: raise LocalSyncFail(msg, project=self.name) new_head = self.work_git.GetHead() - try: - new_head_oid = self.work_git.rev_parse( - "-q", "--verify", "HEAD", log_as_error=False - ) - except error.GitError: + if use_exec_work_git and self.work_git.IsUnbornHead(head=new_head): new_head_oid = None + else: + try: + new_head_oid = self.work_git.rev_parse( + "-q", "--verify", "HEAD", log_as_error=False + ) + except error.GitError: + new_head_oid = None if new_head != head or new_head_oid != head_oid: from_desc = ( @@ -4626,6 +4640,8 @@ class Project: active_git.UpdateRef(ref, dst, message=msg, detach=True) else: active_git.symbolic_ref("-m", msg, ref, dst) + if active_git is self.bare_git: + self.bare_ref.UpdateSymref(ref, dst) def _CheckDirReference(self, srcdir, destdir): # Git worktrees don't use symlinks to share at all. @@ -5262,6 +5278,59 @@ class Project: ) return ref + def IsUnbornHead(self, head: Optional[str] = None) -> bool: + """Check whether HEAD is guaranteed to be an unborn local ref. + + Args: + head: Optional parsed HEAD symref target; if None, read from + the worktree's HEAD file on disk. + + Returns: + True when the repository uses plain loose files and HEAD + points to a missing local ref; False otherwise. + """ + if "GIT_DIR" in os.environ or "GIT_COMMON_DIR" in os.environ: + return False + try: + dotgit = self.GetDotgitPath() + head_path = os.path.join(dotgit, git_refs.HEAD) + if not os.path.isdir(dotgit) or platform_utils.islink( + head_path + ): + return False + if head is None: + with open( + head_path, encoding="utf-8", errors="replace" + ) as fd: + head = self._ParseHead(fd.readline()) + except (OSError, AssertionError): + return False + if not head or not head.startswith("refs/") or ".." in head: + return False + if any(ch in head for ch in "~^:?*[\\@{ "): + return False + return not any( + os.path.lexists(os.path.join(dotgit, rel)) + for rel in ("packed-refs", "reftable", "commondir", head) + ) + + def HasIndexFile(self) -> bool: + """Check whether the worktree may have a Git index file. + + Returns: + False only when the worktree's .git/index file is confirmed + absent; True otherwise (including custom env overrides). + """ + if "GIT_INDEX_FILE" in os.environ or "GIT_DIR" in os.environ: + return True + try: + dotgit = self.GetDotgitPath() + return not os.path.isdir(dotgit) or os.path.lexists( + os.path.join(dotgit, "index") + ) + except (OSError, AssertionError): + return True + def ResolveCommit(self, revision: str) -> str: """Resolve |revision| to a commit without option ambiguity.""" cmdv = ["--verify", "--quiet"] diff --git a/tests/test_git_refs.py b/tests/test_git_refs.py index 108e855eb..965556bd0 100644 --- a/tests/test_git_refs.py +++ b/tests/test_git_refs.py @@ -244,3 +244,165 @@ def test_updates_when_storage_backend_toggles(tmp_path): _run(repo, "refs", "migrate", "--ref-format=files") _run(repo, "branch", "files-branch") assert refs.get("refs/heads/files-branch") == head + + +@pytest.mark.parametrize("reftable", [False, True]) +def test_unborn_head_resolution( + tmp_path: Path, reftable: bool, monkeypatch: pytest.MonkeyPatch +) -> None: + """Unborn HEAD reads files backend directly and uses git on reftable.""" + if reftable and not utils_for_test.supports_reftable(): + pytest.skip("reftable not supported") + + repo = os.path.join(tmp_path, "unborn") + ref_format = "reftable" if reftable else "files" + utils_for_test.init_git_tree(repo, ref_format=ref_format) + _run(repo, "symbolic-ref", "HEAD", "refs/heads/unborn-main") + + refs = git_refs.GitRefs(os.path.join(repo, ".git")) + seen_symbolic_ref = [] + + class SpyGitCommand(git_command.GitCommand): + def __init__( + self, + proj: Any, + cmdv: List[str], + *args: Any, + **kwargs: Any, + ) -> None: + if cmdv and cmdv[0] == "symbolic-ref": + seen_symbolic_ref.append(cmdv) + super().__init__(proj, cmdv, *args, **kwargs) + + monkeypatch.setattr(git_command, "GitCommand", SpyGitCommand) + assert refs.symref("HEAD") == "refs/heads/unborn-main" + if not reftable: + assert seen_symbolic_ref == [] + + +@pytest.mark.parametrize("reftable", [False, True]) +def test_update_symref_keeps_snapshot_warm( + tmp_path: Path, reftable: bool +) -> None: + """UpdateSymref stays warm on files backend and reloads on reftable.""" + if reftable and not utils_for_test.supports_reftable(): + pytest.skip("reftable not supported") + + repo = _init_repo(tmp_path, reftable=reftable) + branch = _run(repo, "symbolic-ref", "--short", "HEAD") + head = _run(repo, "rev-parse", "HEAD") + refs = git_refs.GitRefs(os.path.join(repo, ".git")) + assert refs.get(f"refs/heads/{branch}") == head + + _run(repo, "symbolic-ref", "refs/remotes/m/main", f"refs/heads/{branch}") + refs.UpdateSymref("refs/remotes/m/main", f"refs/heads/{branch}") + + if reftable: + assert not refs.is_loaded + assert refs.symref("refs/remotes/m/main") == f"refs/heads/{branch}" + assert refs.get("refs/remotes/m/main") == head + else: + with mock.patch.object( + refs, + "_LoadAll", + side_effect=AssertionError("expected warm GitRefs snapshot"), + ): + assert refs.symref("refs/remotes/m/main") == f"refs/heads/{branch}" + assert refs.get("refs/remotes/m/main") == head + + +def test_update_symref_invalidates_when_external_ref_changed( + tmp_path: Path, +) -> None: + """External ref writes before UpdateSymref invalidate instead of masking.""" + repo = _init_repo(tmp_path, reftable=False) + branch = _run(repo, "symbolic-ref", "--short", "HEAD") + head = _run(repo, "rev-parse", "HEAD") + refs = git_refs.GitRefs(os.path.join(repo, ".git")) + assert refs.get(f"refs/heads/{branch}") == head + + # External branch write happens before symbolic-ref + UpdateSymref. + _run(repo, "branch", "external-topic") + _run(repo, "symbolic-ref", "refs/remotes/m/main", f"refs/heads/{branch}") + refs.UpdateSymref("refs/remotes/m/main", f"refs/heads/{branch}") + + assert not refs.is_loaded + assert refs.get("refs/heads/external-topic") == head + assert refs.get("refs/remotes/m/main") == head + + # External sibling ref in the same directory is also detected. + _run(repo, "update-ref", "refs/remotes/m/sibling", head) + _run( + repo, + "symbolic-ref", + "refs/remotes/m/main", + "refs/heads/external-topic", + ) + refs.UpdateSymref("refs/remotes/m/main", "refs/heads/external-topic") + + assert not refs.is_loaded + assert refs.get("refs/remotes/m/sibling") == head + + +def test_update_symref_self_or_cyclic_clears_phyref(tmp_path: Path) -> None: + """Self or cyclic symrefs clear any prior physical SHA from _phyref.""" + repo = _init_repo(tmp_path, reftable=False) + head = _run(repo, "rev-parse", "HEAD") + gitdir = os.path.join(repo, ".git") + _run(repo, "update-ref", "refs/remotes/m/main", head) + _run(repo, "update-ref", "refs/remotes/m/other", head) + + refs = git_refs.GitRefs(gitdir) + assert refs.get("refs/remotes/m/main") == head + assert refs.get("refs/remotes/m/other") == head + + _run(repo, "symbolic-ref", "refs/remotes/m/main", "refs/remotes/m/main") + refs.UpdateSymref("refs/remotes/m/main", "refs/remotes/m/main") + assert refs.get("refs/remotes/m/main") == "" + + _run(repo, "symbolic-ref", "refs/remotes/m/main", "refs/remotes/m/other") + refs.UpdateSymref("refs/remotes/m/main", "refs/remotes/m/other") + assert refs.get("refs/remotes/m/main") == head + + _run(repo, "symbolic-ref", "refs/remotes/m/other", "refs/remotes/m/main") + refs.UpdateSymref("refs/remotes/m/other", "refs/remotes/m/main") + assert refs.get("refs/remotes/m/main") == "" + assert refs.get("refs/remotes/m/other") == "" + + +def test_update_symref_propagates_chained_symrefs(tmp_path: Path) -> None: + """Re-pointing an intermediate symref updates chained symrefs cleanly.""" + repo = _init_repo(tmp_path, reftable=False) + branch = _run(repo, "symbolic-ref", "--short", "HEAD") + c1 = _run(repo, "rev-parse", "HEAD") + + Path(os.path.join(repo, "a")).write_text("2", encoding="utf-8") + _run(repo, "add", "a") + _run(repo, "commit", "-q", "-m", "second") + c2 = _run(repo, "rev-parse", "HEAD") + _run(repo, "update-ref", "refs/heads/first", c1) + + # Point HEAD at unborn refs/remotes/m/main before warming GitRefs. + _run(repo, "symbolic-ref", "HEAD", "refs/remotes/m/main") + refs = git_refs.GitRefs(os.path.join(repo, ".git")) + assert refs.symref("HEAD") == "refs/remotes/m/main" + assert refs.get("HEAD") == "" + + _run(repo, "symbolic-ref", "refs/remotes/m/main", "refs/heads/first") + refs.UpdateSymref("refs/remotes/m/main", "refs/heads/first") + _run(repo, "symbolic-ref", "refs/remotes/m/alias", "refs/remotes/m/main") + refs.UpdateSymref("refs/remotes/m/alias", "refs/remotes/m/main") + assert refs.get("HEAD") == c1 + assert refs.get("refs/remotes/m/alias") == c1 + + _run(repo, "symbolic-ref", "refs/remotes/m/main", f"refs/heads/{branch}") + refs.UpdateSymref("refs/remotes/m/main", f"refs/heads/{branch}") + + with mock.patch.object( + refs, + "_LoadAll", + side_effect=AssertionError("expected warm GitRefs snapshot"), + ): + assert refs.get("refs/remotes/m/main") == c2 + assert refs.get("refs/remotes/m/alias") == c2 + assert refs.get("HEAD") == c2 diff --git a/tests/test_project.py b/tests/test_project.py index a4779a1ba..414d94205 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -3845,6 +3845,16 @@ class ReprojectCmdTests(unittest.TestCase): proj.work_git.rev_parse.side_effect = error.GitError("unborn") mock_run = self._reproject(proj) mock_run.assert_called_once() + self.assertEqual( + [ + mock.call( + "-q", "--verify", "HEAD^{tree}", log_as_error=False + ), + mock.call("-q", "--verify", "HEAD", log_as_error=False), + mock.call("-q", "--verify", "HEAD", log_as_error=False), + ], + proj.work_git.rev_parse.call_args_list, + ) def test_reproject_rejects_an_operation_in_progress(self) -> None: """Test an unfinished rebase fails the sync before the command runs.""" @@ -4298,6 +4308,113 @@ class ReprojectCmdTests(unittest.TestCase): self.assertTrue(proj.config.GetBoolean("custom.reprojected")) + def test_reproject_missing_head_tree_keeps_symmetric_head_oid( + self, + ) -> None: + """Born HEAD with missing HEAD^{tree} still compares HEAD cleanly.""" + with utils_for_test.TempGitTree() as tempdir: + proj = self._get_project(tempdir) + proj.work_git.GetHead.return_value = "refs/heads/main" + + def _rev_parse(*args: str, **_kwargs: Any) -> str: + if "HEAD^{tree}" in args: + raise error.GitError("missing tree object") + return self.HEAD_ID + + proj.work_git.rev_parse.side_effect = _rev_parse + mock_run = self._reproject(proj) + mock_run.assert_called_once() + + def test_get_revision_id_reuses_loaded_bare_ref(self) -> None: + """Test GetRevisionId reuses warm bare_ref and peels tag revisions.""" + with utils_for_test.TempGitTree() as tempdir: + subprocess.run( + [ + "git", + "-C", + tempdir, + "commit", + "--allow-empty", + "-q", + "-m", + "init", + ], + check=True, + ) + commit_sha = subprocess.check_output( + ["git", "-C", tempdir, "rev-parse", "HEAD"], text=True + ).strip() + subprocess.run( + [ + "git", + "-C", + tempdir, + "update-ref", + "refs/remotes/origin/main", + commit_sha, + ], + check=True, + ) + subprocess.run( + [ + "git", + "-C", + tempdir, + "tag", + "-a", + "-m", + "tag v1.0", + "v1.0", + commit_sha, + ], + check=True, + ) + tag_sha = subprocess.check_output( + ["git", "-C", tempdir, "rev-parse", "refs/tags/v1.0"], text=True + ).strip() + subprocess.run( + [ + "git", + "-C", + tempdir, + "update-ref", + "refs/remotes/origin/tags/v1.0", + tag_sha, + ], + check=True, + ) + + proj = _create_mock_project(tempdir, use_local_gitdirs=True) + proj.revisionId = None + mock_remote = mock.MagicMock() + proj.GetRemote = mock.MagicMock(return_value=mock_remote) + proj.bare_git.ResolveCommit.return_value = commit_sha + + # Warm bare_ref via public API. + self.assertEqual( + proj.bare_ref.all["refs/remotes/origin/main"], commit_sha + ) + + proj.revisionExpr = "main" + mock_remote.ToLocal.return_value = "refs/remotes/origin/main" + self.assertEqual(proj.GetRevisionId(), commit_sha) + proj.bare_git.ResolveCommit.assert_not_called() + + proj.revisionExpr = "refs/tags/v1.0" + mock_remote.ToLocal.return_value = "refs/tags/v1.0" + self.assertEqual(proj.GetRevisionId(), commit_sha) + proj.bare_git.ResolveCommit.assert_called_once_with( + "refs/tags/v1.0" + ) + + # Custom refspec mapping refs/tags/* elsewhere still peels tags. + proj.bare_git.ResolveCommit.reset_mock() + mock_remote.ToLocal.return_value = "refs/remotes/origin/tags/v1.0" + self.assertEqual(proj.GetRevisionId(), commit_sha) + proj.bare_git.ResolveCommit.assert_called_once_with( + "refs/remotes/origin/tags/v1.0" + ) + class ReprojectCmdGitTests(unittest.TestCase): """Tests running the reprojectcmd contract against a real Git checkout.""" @@ -4609,6 +4726,57 @@ class ReprojectCmdGitTests(unittest.TestCase): self.assertTrue(clean, errors) self.assertFalse(os.path.exists(marker)) + def test_sync_local_half_rejects_staged_changes_on_unborn_head( + self, + ) -> None: + """Test real staged file on unborn HEAD is rejected before reproject.""" + with tempfile.TemporaryDirectory() as topdir: + proj, _ = self._make_client(topdir, self.READ_TREE) + staged_path = os.path.join(proj.worktree, "staged.txt") + with open(staged_path, "w", encoding="utf-8") as fp: + fp.write("staged\n") + self._git(proj.worktree, "add", "staged.txt") + + clean, errors = self._sync(proj) + self.assertFalse(clean) + self.assertTrue( + any("staged.txt" in str(err) for err in errors), errors + ) + + def test_reproject_unborn_empty_skips_redundant_git_checks(self) -> None: + """Clean unborn files-backend checkout runs only post-check diff.""" + with tempfile.TemporaryDirectory() as topdir, mock.patch.dict( + os.environ, {"GIT_DEFAULT_REF_FORMAT": "files"} + ): + proj, revid = self._make_client(topdir, self.READ_TREE) + seen_cmds: List[List[str]] = [] + + class SpyGitCommand(git_command.GitCommand): + def __init__( + self, + proj_arg: Any, + cmdv: List[str], + *args: Any, + **kwargs: Any, + ) -> None: + seen_cmds.append(list(cmdv)) + super().__init__(proj_arg, cmdv, *args, **kwargs) + + with mock.patch.object(git_command, "GitCommand", SpyGitCommand): + proj._Reproject(revid) + + self.assertEqual( + [ + [ + "diff-index", + "--cached", + "--name-only", + f"{revid}^{{tree}}", + ] + ], + seen_cmds, + ) + class DownloadPatchSetTests(unittest.TestCase): """Tests for Project.DownloadPatchSet."""