From f01eb8774f2464531e82f347eb7ce363662fa3c2 Mon Sep 17 00:00:00 2001 From: Hung-Wei Chen Date: Thu, 12 Nov 2026 21:56:18 +0000 Subject: [PATCH] git_config, project: coalesce SetString writes in BatchWrites This change attempts to skip sequential git config calls in order to further speed up git-repo's performance for VFS integration. On git-main we currently call git config 26K+ times, causing roughly 5~6 seconds of latency (see b/543101585#comment23's TRACE_FILE for data). Given the expected time of VFS integration is targeting 30s, 6 seconds makes a huge difference. During an initial `repo sync`, initializing each project (`_InitGitDir` and `_InitRemote`) invokes `SetString` multiple times per project, and `SyncAnalysisState` writes `repo.syncstate.*` keys once per sync. Because the `git config` CLI has no batch-write mode to set multiple keys in a single invocation, each `SetString` call previously spawned a separate `git config` subprocess and rewrote `.repo_config.json` per key (see b/543101585#comment23 and b/543101585#comment25 for trace profiles and benchmark data). Bug: 543101585 Bug: 564817989 Change-Id: Ia1d4613266fb6b7742e8db637039e72ed26a13a2 Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/657083 Tested-by: Gavin Mak Commit-Queue: Gavin Mak Reviewed-by: Brian Gan --- git_config.py | 378 +++++++++++++++++++++++++++++++++------ project.py | 103 ++++++----- tests/test_git_config.py | 230 +++++++++++++++++++++++- tests/test_project.py | 9 +- 4 files changed, 617 insertions(+), 103 deletions(-) diff --git a/git_config.py b/git_config.py index c2da8bbfd..deabab5ec 100644 --- a/git_config.py +++ b/git_config.py @@ -24,7 +24,7 @@ import ssl import subprocess import sys import tempfile -from typing import Optional, Tuple, Union +from typing import Any, Dict, Iterator, List, Optional, Tuple, Union import urllib.error import urllib.request @@ -40,6 +40,8 @@ import repo_trace SYNC_STATE_PREFIX = "repo.syncstate." ID_RE = re.compile(r"^(?:[0-9a-f]{40}|[0-9a-f]{64})$") +_INI_SECTION_RE = re.compile(r"^[A-Za-z0-9-]+$") +_INI_KEY_RE = re.compile(r"^[A-Za-z][A-Za-z0-9-]*$") REVIEW_CACHE = {} @@ -47,6 +49,85 @@ REVIEW_CACHE = {} _JSON_META_KEY = "__repo_meta__" +def _SplitIniName(name: str) -> Optional[Tuple[str, str, str]]: + """Split a config key into INI components when plain-append safe. + + Args: + name: Dotted git config key name (e.g. "remote.origin.url"). + + Returns: + A (section, subsection, var) tuple if the key can be written as a + plain INI entry, or None if it requires git config CLI handling. + """ + parts = name.split(".") + if len(parts) < 2: + return None + section = parts[0] + var = parts[-1] + subsection = ".".join(parts[1:-1]) if len(parts) > 2 else "" + if not _INI_SECTION_RE.match(section) or not _INI_KEY_RE.match(var): + return None + if any( + ord(ch) < 0x20 or ord(ch) > 0x7E or ch in ('"', "\\") + for ch in subsection + ): + return None + return (section, subsection, var) + + +def _IsPlainIniValue(val: Any) -> bool: + """Check whether a config value needs no INI quoting or escaping. + + Args: + val: Candidate configuration value passed to SetString. + + Returns: + True if val is a string that can be written verbatim without quotes + or backslash escapes; False otherwise. + """ + if not isinstance(val, str): + return False + if val and (val[0] in (" ", "\t") or val[-1] in (" ", "\t")): + return False + return not any( + ord(ch) < 0x20 or ord(ch) > 0x7E or ch in ('"', "\\", ";", "#") + for ch in val + ) + + +def _FormatIniBatch(ops: List[Tuple[str, List[str]]], existing: bytes) -> bytes: + """Format queued key/value pairs as UTF-8 INI bytes to append. + + Args: + ops: Ordered list of (name, values) pairs to append. + existing: Existing raw bytes of the target git config file. + + Returns: + UTF-8 encoded INI stanza bytes ready to append after existing. + """ + lines: List[str] = [] + if existing and not existing.endswith(b"\n"): + lines.append("\n") + last_header = None + for name, vals in ops: + parsed = _SplitIniName(name) + assert parsed is not None + section, subsection, var = parsed + header_key = (section.lower(), subsection) + if header_key != last_header: + if subsection: + lines.append(f'[{section} "{subsection}"]\n') + else: + lines.append(f"[{section}]\n") + last_header = header_key + for val in vals: + if val == "": + lines.append(f"\t{var} =\n") + else: + lines.append(f"\t{var} = {val}\n") + return "".join(lines).encode("utf-8") + + def IsChange(rev): return rev.startswith(git_refs.R_CHANGES) @@ -168,6 +249,9 @@ class GitConfig: self._cache_dict = None self._cache_fingerprint = None self._section_dict = None + self._batch_depth = 0 + self._batch_ops: Dict[str, Tuple[str, List[str]]] = {} + self._batch_json_dirty = False self._remotes = {} self._branches = {} @@ -190,9 +274,36 @@ class GitConfig: with contextlib.suppress(OSError): platform_utils.remove(self._json, missing_ok=True) + @contextlib.contextmanager + def BatchWrites(self) -> Iterator[None]: + """Coalesce compatible SetString writes under Git's config.lock. + + Yields: + None while SetString writes are queued for batch flushing on exit. + """ + self._batch_depth += 1 + try: + yield + except BaseException: + self._batch_depth -= 1 + if self._batch_depth == 0: + try: + self._FlushBatch(save_json=True) + except Exception: + self._batch_ops.clear() + self._batch_json_dirty = False + self.ClearCache() + self._RemoveJson() + raise + else: + self._batch_depth -= 1 + if self._batch_depth == 0: + self._FlushBatch(save_json=True) + def Reload(self) -> None: """Clear cached state and reload configuration from disk.""" self.ClearCache() + self._batch_json_dirty = False self._RemoveJson() _ = self._cache @@ -314,6 +425,7 @@ class GitConfig: if ( self._cache_dict is not None + and not self._batch_ops and self._ReadFileFingerprint() != self._cache_fingerprint ): self.ClearCache() @@ -333,7 +445,15 @@ class GitConfig: if value is None: if old: - self._do("--unset-all", name, allow_missing=True) + if key in self._batch_ops: + del self._batch_ops[key] + else: + if self._batch_ops and ( + key == "include.path" or key.startswith("includeif.") + ): + self._FlushBatch(save_json=False) + self._do("--unset-all", name, allow_missing=True) + self._cache_fingerprint = self._ReadFileFingerprint() del self._cache[key] self._SyncCacheAfterSet(key) @@ -345,22 +465,171 @@ class GitConfig: self.SetString(name, value[0]) elif old != value: - self._do("--replace-all", name, value[0]) - try: - for i in range(1, len(value)): - self._do("--add", name, value[i]) - except BaseException: - self.ClearCache() - self._RemoveJson() - raise - self._cache[key] = list(value) + vals = list(value) + if self._CanQueueInBatch(key, name, vals, old): + self._cache[key] = vals + self._batch_ops[key] = (name, vals) + else: + if self._batch_ops: + self._batch_ops.pop(key, None) + self._FlushBatch(save_json=False) + self._do("--replace-all", name, vals[0]) + try: + for i in range(1, len(vals)): + self._do("--add", name, vals[i]) + except BaseException: + self.ClearCache() + self._RemoveJson() + raise + self._cache[key] = vals + self._cache_fingerprint = self._ReadFileFingerprint() self._SyncCacheAfterSet(key) elif len(old) != 1 or old[0] != value: - self._do("--replace-all", name, value) - self._cache[key] = [value] + vals = [value] + if self._CanQueueInBatch(key, name, vals, old): + self._cache[key] = vals + self._batch_ops[key] = (name, vals) + else: + if self._batch_ops: + self._batch_ops.pop(key, None) + self._FlushBatch(save_json=False) + self._do("--replace-all", name, value) + self._cache[key] = vals + self._cache_fingerprint = self._ReadFileFingerprint() self._SyncCacheAfterSet(key) + def _HasIncludes(self) -> bool: + """Return True if cached config contains include or includeIf keys.""" + return self._cache_dict is not None and ( + "include.path" in self._cache_dict + or any(k.startswith("includeif.") for k in self._cache_dict) + ) + + def _CanQueueInBatch( + self, + key: str, + name: str, + vals: List[Any], + old: List[Optional[str]], + ) -> bool: + """Check whether a SetString write can be queued for lockfile append. + + Args: + key: Lowercased canonical key used in self._cache. + name: Original dotted key name preserving subsection case. + vals: New value list requested by SetString. + old: Existing cached values for key before this write. + + Returns: + True if the write can be appended at batch flush time; False if it + must be flushed immediately through git config. + """ + if self._batch_depth <= 0 or self.file == self._SYSTEM_CONFIG: + return False + if ( + key == "include.path" + or key.startswith("includeif.") + or self._HasIncludes() + ): + return False + if old and key not in self._batch_ops: + return False + return _SplitIniName(name) is not None and all( + _IsPlainIniValue(v) for v in vals + ) + + def _FlushBatch(self, save_json: bool = True) -> None: + """Flush queued SetString writes via config.lock or git config CLI. + + Args: + save_json: Whether to persist a dirty .repo_config.json cache + after flushing queued config updates. + """ + ops = list(self._batch_ops.values()) + self._batch_ops.clear() + if ops and not self._TryWriteBatchLocked(ops): + self._ApplyBatchViaGit(ops) + if save_json and self._batch_json_dirty: + self._batch_json_dirty = False + if self._cache_dict is not None: + self._SaveJson(self._cache_dict) + + def _TryWriteBatchLocked(self, ops: List[Tuple[str, List[str]]]) -> bool: + """Atomically append queued entries under .lock if unmodified. + + Args: + ops: Ordered list of (name, values) pairs to append. + + Returns: + True if the lockfile was written and renamed into place; False if + the lock was busy, the file changed externally, or an I/O error + occurred. + """ + if self.file == self._SYSTEM_CONFIG or os.path.islink(self.file): + return False + lock_path = f"{self.file}.lock" + try: + fd = os.open(lock_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o666) + except OSError: + return False + + replaced = False + try: + existing = b"" + curr_fp = None + if os.path.exists(self.file): + with open(self.file, "rb") as src_fp: + existing = src_fp.read() + st = os.fstat(src_fp.fileno()) + curr_fp = ( + st.st_mtime_ns, + st.st_ctime_ns, + st.st_size, + st.st_ino, + hashlib.sha1(existing).hexdigest(), + ) + if hasattr(os, "fchmod") and not platform_utils.isWindows(): + os.fchmod(fd, st.st_mode & 0o777) + if curr_fp != self._cache_fingerprint: + return False + + payload = _FormatIniBatch(ops, existing) + with os.fdopen(fd, "wb") as lock_fp: + fd = -1 + if existing: + lock_fp.write(existing) + lock_fp.write(payload) + os.replace(lock_path, self.file) + replaced = True + self._cache_fingerprint = self._ReadFileFingerprint() + return True + except OSError: + return False + finally: + if fd >= 0: + os.close(fd) + if not replaced: + platform_utils.remove(lock_path, missing_ok=True) + + def _ApplyBatchViaGit(self, ops: List[Tuple[str, List[str]]]) -> None: + """Apply queued batch operations via the git config CLI fallback. + + Args: + ops: Ordered list of (name, values) pairs to write via git config. + """ + try: + for name, vals in ops: + self._do("--replace-all", name, vals[0]) + for extra in vals[1:]: + self._do("--add", name, extra) + except BaseException: + self._batch_json_dirty = False + self.ClearCache() + self._RemoveJson() + raise + self.Reload() + def _SyncCacheAfterSet(self, key: str) -> None: """Keep the in-memory section index and JSON cache coherent. @@ -374,17 +643,16 @@ class GitConfig: elif key.startswith("branch."): self._branches = {} - has_includes = self._cache_dict is not None and ( - "include.path" in self._cache_dict - or any(k.startswith("includeif.") for k in self._cache_dict) - ) if ( key == "include.path" or key.startswith("includeif.") - or has_includes + or self._HasIncludes() ): self.ClearCache() self._RemoveJson() + self._batch_json_dirty = False + elif self._batch_depth > 0: + self._batch_json_dirty = True else: self._SaveJson(self._cache) @@ -476,7 +744,10 @@ class GitConfig: d = self._ReadJson() if d is None: d = self._ReadGit() - self._SaveJson(d) + if self._batch_depth > 0: + self._batch_json_dirty = True + else: + self._SaveJson(d) return d def _ReadFileFingerprint(self) -> Optional[Tuple[int, int, int, int, str]]: @@ -916,17 +1187,18 @@ class Remote: def Save(self): """Save this remote to the configuration.""" - self._Set("url", self.url) - # projectname is initialized for projects listed in the manifest, but - # not for others (e.g. the manifest project). This class is used for - # all of them. - if self.pushUrl is not None and self.projectname is not None: - self._Set("pushurl", self.pushUrl + "/" + self.projectname) - else: - self._Set("pushurl", self.pushUrl) - self._Set("review", self.review) - self._Set("projectname", self.projectname) - self._Set("fetch", list(map(str, self.fetch))) + with self._config.BatchWrites(): + self._Set("url", self.url) + # projectname is initialized for projects listed in the manifest, + # but not for others (e.g. the manifest project). This class is + # used for all of them. + if self.pushUrl is not None and self.projectname is not None: + self._Set("pushurl", self.pushUrl + "/" + self.projectname) + else: + self._Set("pushurl", self.pushUrl) + self._Set("review", self.review) + self._Set("projectname", self.projectname) + self._Set("fetch", list(map(str, self.fetch))) def _Set(self, key, value): key = f"remote.{self.name}.{key}" @@ -960,11 +1232,12 @@ class Branch: def Save(self): """Save this branch back into the configuration.""" - if self.remote: - self._Set("remote", self.remote.name) - else: - self._Set("remote", None) - self._Set("merge", self.merge) + with self._config.BatchWrites(): + if self.remote: + self._Set("remote", self.remote.name) + else: + self._Set("remote", None) + self._Set("merge", self.merge) def _Set(self, key, value): key = f"branch.{self.name}.{key}" @@ -1001,23 +1274,24 @@ class SyncAnalysisState: """ self._config = config now = datetime.datetime.now(datetime.timezone.utc) - self._Set("main.synctime", now.isoformat(timespec="microseconds")) - self._Set("main.version", "1") - self._Set("sys.argv", sys.argv) - for key, value in superproject_logging_data.items(): - self._Set(f"superproject.{key}", value) - for key, value in options.__dict__.items(): - self._Set(f"options.{key}", value) - config_items = config.DumpConfigDict().items() - EXTRACT_NAMESPACES = {"repo", "branch", "remote"} - self._SetDictionary( - { - k: v - for k, v in config_items - if not k.startswith(SYNC_STATE_PREFIX) - and k.split(".", 1)[0] in EXTRACT_NAMESPACES - } - ) + with self._config.BatchWrites(): + self._Set("main.synctime", now.isoformat(timespec="microseconds")) + self._Set("main.version", "1") + self._Set("sys.argv", sys.argv) + for key, value in superproject_logging_data.items(): + self._Set(f"superproject.{key}", value) + for key, value in options.__dict__.items(): + self._Set(f"options.{key}", value) + config_items = config.DumpConfigDict().items() + EXTRACT_NAMESPACES = {"repo", "branch", "remote"} + self._SetDictionary( + { + k: v + for k, v in config_items + if not k.startswith(SYNC_STATE_PREFIX) + and k.split(".", 1)[0] in EXTRACT_NAMESPACES + } + ) def _SetDictionary(self, data): """Save all key/value pairs of |data| dictionary. diff --git a/project.py b/project.py index ea3304c50..308d0f8ad 100644 --- a/project.py +++ b/project.py @@ -1926,24 +1926,26 @@ class Project: if is_new is None: is_new = not self.Exists - if is_new: - self._InitGitDir(force_sync=force_sync, quiet=quiet) - else: - try: - # At this point, it's possible that gitdir points to an old - # objdir (e.g. name changed, but objdir exists). Check - # references to ensure that's not the case. See - # https://issues.gerritcodereview.com/40013418 for more - # details. - self._CheckDirReference(self.objdir, self.gitdir) + with self.config.BatchWrites(): + if is_new: + self._InitGitDir(force_sync=force_sync, quiet=quiet) + else: + try: + # At this point, it's possible that gitdir points to an old + # objdir (e.g. name changed, but objdir exists). Check + # references to ensure that's not the case. See + # https://issues.gerritcodereview.com/40013418 for more + # details. + self._CheckDirReference(self.objdir, self.gitdir) - self._UpdateHooks(quiet=quiet) - except error.GitError as e: - if not force_sync: - raise e - # Let _InitGitDir fix the issue, force_sync is always True here. - self._InitGitDir(force_sync=True, quiet=quiet) - self._InitRemote() + self._UpdateHooks(quiet=quiet) + except error.GitError as e: + if not force_sync: + raise e + # Let _InitGitDir fix the issue, force_sync is always True + # here. + self._InitGitDir(force_sync=True, quiet=quiet) + self._InitRemote() if self.UseAlternates: # If gitdir/objects is a symlink, migrate it from the old layout. @@ -4496,23 +4498,27 @@ class Project: ) m = self.manifest.manifestProject.config - for key in ["user.name", "user.email"]: - if m.Has(key, include_defaults=False): - curr_config.SetString(key, m.GetString(key)) - if not self.manifest.EnableGitLfs: - curr_config.SetString( - "filter.lfs.smudge", "git-lfs smudge --skip -- %f" + with curr_config.BatchWrites(): + for key in ["user.name", "user.email"]: + if m.Has(key, include_defaults=False): + curr_config.SetString(key, m.GetString(key)) + if not self.manifest.EnableGitLfs: + curr_config.SetString( + "filter.lfs.smudge", + "git-lfs smudge --skip -- %f", + ) + curr_config.SetString( + "filter.lfs.process", + "git-lfs filter-process --skip", + ) + curr_config.SetBoolean( + "core.bare", True if self.manifest.IsMirror else None ) - curr_config.SetString( - "filter.lfs.process", "git-lfs filter-process --skip" - ) - curr_config.SetBoolean( - "core.bare", True if self.manifest.IsMirror else None - ) if tmp_gitdir: platform_utils.rename(tmp_gitdir, self.gitdir) tmp_gitdir = None + self.config.ClearCache() if not init_obj_dir and self.objdir != self.gitdir: # The project might be shared (obj_dir already initialized), but # such information is not available here. Instead of passing it, @@ -4614,27 +4620,28 @@ class Project: raise def _InitRemote(self): - if self.remote.url: - remote = self.GetRemote() - remote.url = self.remote.url - remote.pushUrl = self.remote.pushUrl - remote.review = self.remote.review - remote.projectname = self.name + with self.config.BatchWrites(): + if self.remote.url: + remote = self.GetRemote() + remote.url = self.remote.url + remote.pushUrl = self.remote.pushUrl + remote.review = self.remote.review + remote.projectname = self.name - if self.worktree: - remote.ResetFetch(mirror=False) - else: - remote.ResetFetch(mirror=True) - remote.Save() + if self.worktree: + remote.ResetFetch(mirror=False) + else: + remote.ResetFetch(mirror=True) + remote.Save() - # Disable auto-gc for depth=1 to prevent hangs during lazy fetches - # inside git checkout for partial clones. - effective_depth = ( - self.clone_depth or self.manifest.manifestProject.depth - ) - if effective_depth == 1: - self.config.SetBoolean("maintenance.auto", False) - self.config.SetInt("gc.auto", 0) + # Disable auto-gc for depth=1 to prevent hangs during lazy fetches + # inside git checkout for partial clones. + effective_depth = ( + self.clone_depth or self.manifest.manifestProject.depth + ) + if effective_depth == 1: + self.config.SetBoolean("maintenance.auto", False) + self.config.SetInt("gc.auto", 0) def _InitMRef(self): """Initialize the pseudo m/ ref.""" diff --git a/tests/test_git_config.py b/tests/test_git_config.py index 8587a6533..4cbf776e5 100644 --- a/tests/test_git_config.py +++ b/tests/test_git_config.py @@ -17,7 +17,8 @@ import json import os from pathlib import Path -from typing import Any, Optional +import types +from typing import Any, List, Optional, Tuple import pytest import utils_for_test @@ -563,3 +564,230 @@ def test_set_string_after_external_edit_preserves_external_keys( assert reader2.GetString("other.key") == "ext2" assert reader2.GetString("user.name") == "Alice" assert reader2.GetString("user.email") == "alice@example.com" + + +def test_batch_writes_coalesces_new_keys_without_git_replace( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Test BatchWrites writes new sections atomically with one JSON save.""" + cfg_path = tmp_path / "config" + cfg_path.write_text("[core]\n\trepositoryformatversion = 0\n") + config = git_config.GitConfig(str(cfg_path)) + + orig_do = config._do + seen_do = [] + save_count = 0 + orig_save = config._SaveJson + + def spy_do(*args: str) -> str: + seen_do.append(args) + return orig_do(*args) + + def spy_save(cache: Any) -> None: + nonlocal save_count + save_count += 1 + orig_save(cache) + + monkeypatch.setattr(config, "_do", spy_do) + monkeypatch.setattr(config, "_SaveJson", spy_save) + + with config.BatchWrites(): + config.SetString("filter.lfs.smudge", "git-lfs smudge --skip -- %f") + config.SetString("filter.lfs.process", "git-lfs filter-process --skip") + config.SetString("remote.goog.url", "https://example.com/platform/art") + config.SetString( + "remote.goog.fetch", + [ + "+refs/heads/*:refs/remotes/goog/*", + "+refs/tags/*:refs/tags/*", + ], + ) + config.SetBoolean("maintenance.auto", False) + + assert seen_do == [("--null", "--list")] + assert save_count == 1 + assert not (tmp_path / "config.lock").exists() + + reloaded = git_config.GitConfig(str(cfg_path)) + fresh = reloaded._ReadGit() + assert fresh["filter.lfs.smudge"] == ["git-lfs smudge --skip -- %f"] + assert fresh["filter.lfs.process"] == ["git-lfs filter-process --skip"] + assert fresh["remote.goog.url"] == ["https://example.com/platform/art"] + assert fresh["remote.goog.fetch"] == [ + "+refs/heads/*:refs/remotes/goog/*", + "+refs/tags/*:refs/tags/*", + ] + assert fresh["maintenance.auto"] == ["false"] + + +def test_batch_writes_falls_back_on_lock_or_external_edit( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Test BatchWrites preserves concurrent edits and respects config.lock.""" + # 1. External edit between cache load and flush falls back to git config CLI + # so neither the external key nor the batched key is lost. + cfg_path = tmp_path / "config" + cfg_path.write_text("[core]\n\tfilemode = true\n") + config = git_config.GitConfig(str(cfg_path)) + + with config.BatchWrites(): + config.SetString("remote.origin.url", "https://example.com/a") + with open(cfg_path, "a", encoding="utf-8") as fp: + fp.write("[external]\n\tadded = yes\n") + + reloaded = git_config.GitConfig(str(cfg_path)) + fresh = reloaded._ReadGit() + assert fresh["external.added"] == ["yes"] + assert fresh["remote.origin.url"] == ["https://example.com/a"] + assert config.GetString("external.added") == "yes" + + # 2. Pre-existing config.lock is never clobbered; BatchWrites delegates to + # _do instead. + locked_path = tmp_path / "locked_config" + locked_path.write_text("[core]\n\tfilemode = true\n") + lock_file = tmp_path / "locked_config.lock" + lock_file.write_text("held-by-other-process") + + locked_cfg = git_config.GitConfig(str(locked_path)) + _ = locked_cfg.GetBoolean("core.filemode") + do_calls = [] + monkeypatch.setattr( + locked_cfg, + "_do", + lambda *args: do_calls.append(args) or "core.filemode\ntrue\0", + ) + + with locked_cfg.BatchWrites(): + locked_cfg.SetString("remote.origin.url", "https://example.com/b") + + assert lock_file.read_text() == "held-by-other-process" + assert ("--replace-all", "remote.origin.url", "https://example.com/b") in ( + do_calls + ) + + +def test_batch_writes_direct_do_refreshes_fingerprint_and_branch_save_batches( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Direct _do in BatchWrites updates fingerprint; Save uses BatchWrites.""" + cfg_path = tmp_path / "config" + cfg_path.write_text("[core]\n\tfilemode = true\n") + config = git_config.GitConfig(str(cfg_path)) + _ = config.GetBoolean("core.filemode") + + orig_do = config._do + seen_do = [] + + def spy_do(*args: str, **kwargs: Any) -> str: + seen_do.append(args) + return orig_do(*args, **kwargs) + + monkeypatch.setattr(config, "_do", spy_do) + + # Updating an existing key runs _do() directly inside BatchWrites; the next + # SetString must not see a stale _cache_fingerprint or re-run --list. + with config.BatchWrites(): + config.SetString("core.filemode", "false") + config.SetString("new.section.key", "val") + + assert seen_do == [("--replace-all", "core.filemode", "false")] + assert config.GetString("core.filemode") == "false" + assert config.GetString("new.section.key") == "val" + + # Remote.Save() and Branch.Save() coalesce new keys via BatchWrites without + # spawning git config --replace-all subprocesses. + seen_do.clear() + remote = config.GetRemote("origin") + remote.url = "https://example.com/repo" + remote.ResetFetch() + remote.Save() + + branch = config.GetBranch("main") + branch.remote = remote + branch.merge = "refs/heads/main" + branch.Save() + + assert seen_do == [] + + # When include.path is present, _CanQueueInBatch returns False so writes go + # through git config immediately instead of queueing for lockfile append. + inc_file = tmp_path / "inc.gitconfig" + inc_file.write_text("[extra]\n\tflag = 1\n") + config.SetString("include.path", str(inc_file)) + _ = config.GetString("extra.flag") + seen_do.clear() + with config.BatchWrites(): + config.SetString("after.include", "yes") + assert ("--replace-all", "after.include", "yes") in seen_do + + +def test_batch_writes_failure_paths(tmp_path: Path) -> None: + """BatchWrites clears state on failure and preserves body errors.""" + # 1. Failed fallback flush clears poisoned in-memory cache and dirty flag. + fail_path = tmp_path / "fail_config" + fail_path.write_text("[core]\n\tfilemode = true\n") + lock_path = tmp_path / "fail_config.lock" + lock_path.write_text("locked") + fail_cfg = git_config.GitConfig(str(fail_path)) + assert fail_cfg.GetBoolean("core.filemode") is True + + with pytest.raises(error.GitError): + with fail_cfg.BatchWrites(): + fail_cfg.SetString("remote.origin.url", "https://bad.example.com") + + lock_path.unlink() + assert fail_cfg.GetString("remote.origin.url") is None + with fail_cfg.BatchWrites(): + pass + assert ( + git_config.GitConfig(str(fail_path)).GetString("remote.origin.url") + is None + ) + + # 2. If body raises and flush also fails, original body error propagates. + body_path = tmp_path / "body_config" + body_path.write_text("[core]\n\tfilemode = true\n") + body_lock = tmp_path / "body_config.lock" + body_lock.write_text("locked") + body_cfg = git_config.GitConfig(str(body_path)) + assert body_cfg.GetBoolean("core.filemode") is True + + with pytest.raises(RuntimeError, match="body boom"): + with body_cfg.BatchWrites(): + body_cfg.SetString("remote.origin.url", "https://bad.example.com") + raise RuntimeError("body boom") + + body_lock.unlink() + assert body_cfg.GetString("remote.origin.url") is None + + +def test_batch_writes_sync_analysis_state( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """UpdateSyncAnalysisState coalesces new syncstate keys without CLI.""" + state_cfg_path = tmp_path / "state_config" + state_cfg_path.write_text( + '[remote "origin"]\n\turl = https://example.com/repo\n', + encoding="utf-8", + ) + state_cfg = git_config.GitConfig(str(state_cfg_path)) + _ = state_cfg.GetString("remote.origin.url") + seen_do: List[Tuple[str, ...]] = [] + orig_do = state_cfg._do + monkeypatch.setattr( + state_cfg, + "_do", + lambda *args, **kw: seen_do.append(args) or orig_do(*args, **kw), + ) + + opts = types.SimpleNamespace(jobs=8, quiet=True) + state_cfg.UpdateSyncAnalysisState(opts, {"superproject": False}) + + assert seen_do == [] + fresh_state = git_config.GitConfig(str(state_cfg_path))._ReadGit() + assert fresh_state["repo.syncstate.main.version"] == ["1"] + assert fresh_state["repo.syncstate.options.jobs"] == ["8"] + assert fresh_state["repo.syncstate.options.quiet"] == ["true"] + assert fresh_state["repo.syncstate.remote.origin.url"] == [ + "https://example.com/repo" + ] diff --git a/tests/test_project.py b/tests/test_project.py index 82cef09d9..79307ab5f 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -5169,11 +5169,16 @@ class InitGitDirConfigTests(unittest.TestCase): super().__init__(proj_arg, cmdv, *args, **kwargs) with mock.patch.object(git_command, "GitCommand", SpyGitCommand): - proj._InitGitDir(quiet=True) - proj._InitRemote() + with proj.config.BatchWrites(): + proj._InitGitDir(quiet=True) + proj._InitRemote() init_cmds = [cmd for cmd in seen_cmds if cmd and cmd[0] == "init"] self.assertEqual([["init", "-q", "--template="]], init_cmds) + self.assertFalse( + any("--replace-all" in cmd for cmd in seen_cmds), + f"Expected BatchWrites to avoid --replace-all, got {seen_cmds}", + ) reloaded = git_config.GitConfig.ForRepository(gitdir=proj.gitdir) fresh = reloaded._ReadGit() self.assertEqual(