mirror of
https://gerrit.googlesource.com/git-repo
synced 2026-10-04 12:40:46 +00:00
project: read HEAD directly in-memory to avoid subprocesses
Benchmark: * repo upload frameworks/base: 2.84s -> 0.50s (-82.3%, 5.6x faster) * repo upload (3,045 projects): 7.66s -> 5.24s (-31.6%, 2.42s saved) From repo's inception through v2.56, GetHead() directly read the `.git/HEAD` file in Python. In commit52bab0ba("project: Use git rev-parse to read HEAD"), this was replaced with git subprocess calls on the premise that git provides a dedicated command. However, in large multi-project workspaces (such as Android with 3,000+ projects), spawning thousands of git processes introduced severe latency regressions during `repo upload` and `repo status`. Furthermore, switching to subprocesses broke detached HEADs and unborn branches (fixed in commits7f7d70efand8c3585f3by re-adding the v2.56 file-reading logic as an error recovery fallback). This patch restores fast in-memory reading as the primary path, while adding modern defensive safeguards: * Symbolic refs (`ref: refs/heads/...`): Strips whitespace and tabs and returns the ref directly in memory. * Detached HEAD: Validates 40-char SHA-1 and 64-char SHA-256 commit hashes via git_config.IsId(), normalizing to lowercase. * Symlinks: Detects filesystem symlinks via os.path.islink() and safely falls back to git symbolic-ref. * Fallback: Catches (OSError, AssertionError) and transparently falls back to native git commands for reftables, unexpected layouts, or filesystem errors. Unifies recovery fallback parsing with the fast path (CRLF/tabs, lowercase hashes, and consistent RelPath errors). In addition, this change substantially expands test coverage in tests/test_project.py, adding comprehensive unit tests for symbolic refs, whitespace/tabs, CRLF line endings, SHA-1, SHA-256, uppercase hash normalization, symlinks, corrupted worktrees, and fallback robustness. Test: ./run_tests tests/test_project.py Change-Id: Ib5c2530117c6939e4b9293feda81aa745c003c6a Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/623001 Commit-Queue: James Hawkins <jhawkins@google.com> Reviewed-by: Brian Gan <brgan@google.com> Tested-by: James Hawkins <jhawkins@google.com>
This commit is contained in:
1 parent
d7299422ae
commit
4fe87617ff
2 files changed
+281
-14
No files matched your search
@@ -409,6 +409,228 @@ class ProjectTests(unittest.TestCase):
|
||||
).strip()
|
||||
self.assertEqual(expected, fakeproj.work_git.GetHead())
|
||||
|
||||
def test_parse_head(self) -> None:
|
||||
"""Verify _ParseHead parses refs, hashes, whitespace, and invalid
|
||||
refs.
|
||||
"""
|
||||
with tempfile.TemporaryDirectory(prefix="repo-tests") as tempdir:
|
||||
fakeproj = FakeProject(tempdir)
|
||||
work_git = fakeproj.work_git
|
||||
|
||||
# Standard symbolic ref
|
||||
self.assertEqual(
|
||||
work_git._ParseHead("ref: refs/heads/main\n"),
|
||||
"refs/heads/main",
|
||||
)
|
||||
|
||||
# Tabs and extra whitespace
|
||||
self.assertEqual(
|
||||
work_git._ParseHead("ref:\t refs/heads/branch \r\n"),
|
||||
"refs/heads/branch",
|
||||
)
|
||||
|
||||
# Reftables placeholder should return None
|
||||
self.assertIsNone(work_git._ParseHead("ref: refs/heads/.invalid\n"))
|
||||
|
||||
# Empty or whitespace-only symbolic refs should return None
|
||||
self.assertIsNone(work_git._ParseHead("ref:\n"))
|
||||
self.assertIsNone(work_git._ParseHead("ref: \t \r\n"))
|
||||
|
||||
# 40-character SHA-1
|
||||
sha1 = "0123456789abcdef0123456789abcdef01234567"
|
||||
self.assertEqual(work_git._ParseHead(f"{sha1}\n"), sha1)
|
||||
|
||||
# Uppercase SHA-1 normalized to lowercase
|
||||
sha_upper = "4B825DC642CB6EB9A060E54BF8D69288FBEE4904"
|
||||
self.assertEqual(
|
||||
work_git._ParseHead(f"{sha_upper}\r\n"), sha_upper.lower()
|
||||
)
|
||||
|
||||
# 64-character SHA-256
|
||||
sha256 = "0123456789abcdef" * 4
|
||||
self.assertEqual(work_git._ParseHead(f"{sha256}\n"), sha256)
|
||||
|
||||
# 40-character string with invalid hex characters (e.g. 'g'-'z')
|
||||
invalid_sha = "0123456789abcdef0123456789abcdef0123456z"
|
||||
self.assertIsNone(work_git._ParseHead(f"{invalid_sha}\n"))
|
||||
|
||||
# Empty or unparseable lines
|
||||
self.assertIsNone(work_git._ParseHead(""))
|
||||
self.assertIsNone(work_git._ParseHead(" \n"))
|
||||
self.assertIsNone(work_git._ParseHead("corrupted-not-a-hash"))
|
||||
|
||||
def test_get_head_in_memory_fast_path(self) -> None:
|
||||
"""Verify GetHead reads HEAD in-memory without spawning subprocesses."""
|
||||
with tempfile.TemporaryDirectory(prefix="repo-tests") as tempdir:
|
||||
fakeproj = FakeProject(tempdir)
|
||||
os.makedirs(fakeproj.gitdir, exist_ok=True)
|
||||
head_file = os.path.join(fakeproj.gitdir, "HEAD")
|
||||
|
||||
# 1. Standard symbolic ref (on a branch)
|
||||
with open(head_file, "w", encoding="utf-8", newline="") as fp:
|
||||
fp.write("ref: refs/heads/feature-branch\n")
|
||||
|
||||
with mock.patch.object(
|
||||
fakeproj.work_git, "symbolic_ref"
|
||||
) as mock_sym, mock.patch.object(
|
||||
fakeproj.work_git, "rev_parse"
|
||||
) as mock_parse:
|
||||
self.assertEqual(
|
||||
fakeproj.work_git.GetHead(), "refs/heads/feature-branch"
|
||||
)
|
||||
mock_sym.assert_not_called()
|
||||
mock_parse.assert_not_called()
|
||||
|
||||
# 2. Whitespace, tabs, and CRLF handling
|
||||
with open(head_file, "w", encoding="utf-8", newline="") as fp:
|
||||
fp.write("ref:\t refs/heads/feature-branch \r\n")
|
||||
|
||||
with mock.patch.object(
|
||||
fakeproj.work_git, "symbolic_ref"
|
||||
) as mock_sym, mock.patch.object(
|
||||
fakeproj.work_git, "rev_parse"
|
||||
) as mock_parse:
|
||||
self.assertEqual(
|
||||
fakeproj.work_git.GetHead(), "refs/heads/feature-branch"
|
||||
)
|
||||
mock_sym.assert_not_called()
|
||||
mock_parse.assert_not_called()
|
||||
|
||||
# 3. Detached HEAD with 40-character SHA-1
|
||||
fake_sha1 = "0123456789abcdef0123456789abcdef01234567"
|
||||
with open(head_file, "w", encoding="utf-8", newline="") as fp:
|
||||
fp.write(f"{fake_sha1}\n")
|
||||
|
||||
with mock.patch.object(
|
||||
fakeproj.work_git, "symbolic_ref"
|
||||
) as mock_sym, mock.patch.object(
|
||||
fakeproj.work_git, "rev_parse"
|
||||
) as mock_parse:
|
||||
self.assertEqual(fakeproj.work_git.GetHead(), fake_sha1)
|
||||
mock_sym.assert_not_called()
|
||||
mock_parse.assert_not_called()
|
||||
|
||||
# 4. Detached HEAD with 64-character SHA-256
|
||||
fake_sha256 = "0123456789abcdef" * 4
|
||||
with open(head_file, "w", encoding="utf-8", newline="") as fp:
|
||||
fp.write(f"{fake_sha256}\n")
|
||||
|
||||
with mock.patch.object(
|
||||
fakeproj.work_git, "symbolic_ref"
|
||||
) as mock_sym, mock.patch.object(
|
||||
fakeproj.work_git, "rev_parse"
|
||||
) as mock_parse:
|
||||
self.assertEqual(fakeproj.work_git.GetHead(), fake_sha256)
|
||||
mock_sym.assert_not_called()
|
||||
mock_parse.assert_not_called()
|
||||
|
||||
# 5. Uppercase SHA normalized to lowercase
|
||||
fake_upper = fake_sha1.upper()
|
||||
with open(head_file, "w", encoding="utf-8", newline="") as fp:
|
||||
fp.write(f"{fake_upper}\n")
|
||||
|
||||
with mock.patch.object(
|
||||
fakeproj.work_git, "symbolic_ref"
|
||||
) as mock_sym, mock.patch.object(
|
||||
fakeproj.work_git, "rev_parse"
|
||||
) as mock_parse:
|
||||
self.assertEqual(fakeproj.work_git.GetHead(), fake_sha1)
|
||||
mock_sym.assert_not_called()
|
||||
mock_parse.assert_not_called()
|
||||
|
||||
def test_get_head_symlink_and_fallback(self) -> None:
|
||||
"""Verify GetHead handles symlinks and invalid files via fallback."""
|
||||
with tempfile.TemporaryDirectory(prefix="repo-tests") as tempdir:
|
||||
fakeproj = FakeProject(tempdir)
|
||||
os.makedirs(fakeproj.gitdir, exist_ok=True)
|
||||
head_file = os.path.join(fakeproj.gitdir, "HEAD")
|
||||
|
||||
# Symlink HEAD should fall back to symbolic_ref
|
||||
platform_utils.symlink("refs/heads/main", head_file)
|
||||
with mock.patch.object(
|
||||
fakeproj.work_git,
|
||||
"symbolic_ref",
|
||||
return_value="refs/heads/main",
|
||||
) as mock_sym:
|
||||
self.assertEqual(fakeproj.work_git.GetHead(), "refs/heads/main")
|
||||
mock_sym.assert_called_once()
|
||||
|
||||
def test_get_head_worktree_corrupted_fallback(self) -> None:
|
||||
"""Verify GetHead raises NoManifestException on corrupted worktrees."""
|
||||
with tempfile.TemporaryDirectory(prefix="repo-tests") as tempdir:
|
||||
dotgit = os.path.join(tempdir, ".git")
|
||||
with open(dotgit, "w", encoding="utf-8", newline="") as fp:
|
||||
fp.write("malformed without gitdir prefix\n")
|
||||
fakeproj = FakeProject(tempdir)
|
||||
with self.assertRaises(error.NoManifestException) as cm:
|
||||
fakeproj.work_git.GetHead()
|
||||
self.assertEqual(cm.exception.path, fakeproj.RelPath(local=False))
|
||||
|
||||
def test_get_head_fallback_robustness(self) -> None:
|
||||
"""Verify GetHead fallback handles CRLF, tabs, and lowercase hashes."""
|
||||
with tempfile.TemporaryDirectory(prefix="repo-tests") as tempdir:
|
||||
fakeproj = FakeProject(tempdir)
|
||||
os.makedirs(fakeproj.gitdir, exist_ok=True)
|
||||
head_file = os.path.join(fakeproj.gitdir, "HEAD")
|
||||
|
||||
# 1. Fallback strips tabs, extra whitespace, and CRLF
|
||||
with open(head_file, "w", encoding="utf-8", newline="") as fp:
|
||||
fp.write("ref:\t refs/heads/fallback-branch\r\n")
|
||||
|
||||
with mock.patch("platform_utils.islink", return_value=True):
|
||||
with mock.patch.object(
|
||||
fakeproj.work_git,
|
||||
"symbolic_ref",
|
||||
side_effect=error.GitError("sym error"),
|
||||
), mock.patch.object(
|
||||
fakeproj.work_git,
|
||||
"rev_parse",
|
||||
side_effect=error.GitError("parse error"),
|
||||
):
|
||||
self.assertEqual(
|
||||
fakeproj.work_git.GetHead(),
|
||||
"refs/heads/fallback-branch",
|
||||
)
|
||||
|
||||
# 2. Fallback normalizes uppercase hashes to lowercase
|
||||
sha_upper = "4B825DC642CB6EB9A060E54BF8D69288FBEE4904"
|
||||
with open(head_file, "w", encoding="utf-8", newline="") as fp:
|
||||
fp.write(f"{sha_upper}\r\n")
|
||||
|
||||
with mock.patch("platform_utils.islink", return_value=True):
|
||||
with mock.patch.object(
|
||||
fakeproj.work_git,
|
||||
"symbolic_ref",
|
||||
side_effect=error.GitError("sym error"),
|
||||
), mock.patch.object(
|
||||
fakeproj.work_git,
|
||||
"rev_parse",
|
||||
side_effect=error.GitError("parse error"),
|
||||
):
|
||||
self.assertEqual(
|
||||
fakeproj.work_git.GetHead(), sha_upper.lower()
|
||||
)
|
||||
|
||||
# 3. Fallback raises NoManifestException with RelPath on .invalid
|
||||
with open(head_file, "w", encoding="utf-8", newline="") as fp:
|
||||
fp.write("ref: refs/heads/.invalid\r\n")
|
||||
|
||||
with mock.patch("platform_utils.islink", return_value=True):
|
||||
with mock.patch.object(
|
||||
fakeproj.work_git,
|
||||
"symbolic_ref",
|
||||
side_effect=error.GitError("sym error"),
|
||||
), mock.patch.object(
|
||||
fakeproj.work_git,
|
||||
"rev_parse",
|
||||
side_effect=error.GitError("parse error"),
|
||||
):
|
||||
with self.assertRaises(error.NoManifestException) as cm:
|
||||
fakeproj.work_git.GetHead()
|
||||
self.assertEqual(
|
||||
cm.exception.path, fakeproj.RelPath(local=False)
|
||||
)
|
||||
|
||||
def _get_derived_subproject_url(self, submodule_url):
|
||||
with tempfile.TemporaryDirectory(prefix="repo-tests") as tempdir:
|
||||
|
||||
|
||||
Reference in new issue
Block a user