[scm] Refactor git config state to be fully mock-able.

Previously little bits of the scm.GIT were mocked (like SetConfig,
and GetConfig, but not the other config related methods).

This changes things so that the git config state is a class whose
logic is shared between prod and test, with a 'real' and 'test'
implementation which know how to load and save configuration at
a low level.

R=yiwzhang

Change-Id: I1dc11b550908862607ea539de7fa60fbce30e700
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5660891
Commit-Queue: Yiwei Zhang <yiwzhang@google.com>
Reviewed-by: Allen Li <ayatane@chromium.org>
Auto-Submit: Robbie Iannucci <iannucci@chromium.org>
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>

scm.py

@@ -3,12 +3,16 @@
 # found in the LICENSE file.
 """SCM-specific utility classes."""
 
-from collections import defaultdict
+import abc
 import os
 import pathlib
 import platform
 import re
-from typing import Mapping, List
+import threading
+
+from collections import defaultdict
+from typing import Iterable, Literal, Dict, List, Optional, Sequence
+from typing import Tuple, Mapping
 
 import gclient_utils
 import git_common
@@ -41,6 +45,349 @@ def determine_scm(root):
         return 'diff'
 
 
+GitConfigScope = Literal['system', 'local', 'worktree']
+GitScopeOrder: List[GitConfigScope] = ['system', 'local', 'worktree']
+GitFlatConfigData = Mapping[str, Sequence[str]]
+
+
+class GitConfigStateBase(metaclass=abc.ABCMeta):
+    """GitConfigStateBase is the abstract base class for implementations of
+    CachedGitConfigState.
+
+    This is meant to model the manipulation of some underlying config data.
+
+    In GitConfigStateReal, this is modeled using `git config` commands in
+    a specific git repo.
+
+    In GitConfigStateTest, this is modeled using a set of GitConfigScope-indexed
+    dictionaries.
+    """
+
+    @abc.abstractmethod
+    def load_config(self) -> GitFlatConfigData:
+        """When invoked, this should return the full state of the configuration
+        observable.
+
+        The caller must not mutate the returned value.
+        """
+
+    @abc.abstractmethod
+    def set_config(self, key: str, value: str, *, append: bool,
+                   scope: GitConfigScope):
+        """When invoked, this should set `key` to a singluar `value` in the git
+        scope `scope` in this state's underlying data.
+
+        If `append` is True, this should add an additional value to the existing
+        `key`, if any.
+        """
+
+    @abc.abstractmethod
+    def set_config_multi(self, key: str, value: str, *, append: bool,
+                         value_pattern: Optional[str], scope: GitConfigScope):
+        """When invoked, this should replace all existing values of `key` with
+        `value` in the git scope `scope` in this state's underlying data.
+
+        If `value_pattern` is supplied, only existing values matching this
+        pattern will be replaced.
+
+        TODO: Make value_pattern an re.Pattern. This wasn't done at the time of
+        this refactor to keep the refactor small.
+        """
+
+    @abc.abstractmethod
+    def unset_config(self, key: str, *, scope: GitConfigScope):
+        """When invoked, remove a singlar value from `key` in this state's underlying data.
+        """
+
+    @abc.abstractmethod
+    def unset_config_multi(self, key: str, *, value_pattern: Optional[str],
+                           scope: GitConfigScope):
+        """When invoked, remove all values from `key` in this state's underlying data.
+
+        If `value_pattern` is supplied, only values matching this pattern will
+        be removed.
+
+        TODO: Make value_pattern an re.Pattern. This wasn't done at the time of
+        this refactor to keep the refactor small.
+        """
+
+
+class CachedGitConfigState(object):
+    """This represents the observable git configuration state for a given
+    repository (whose top-level path is `root`).
+
+    This maintains an in-memory cache of the entire, flattened, observable
+    configuration state according to the GitConfigStateBase implementation.
+
+    All SetConfig operations which actually change the underlying data will
+    clear the internal cache. All read operations will either use the internal
+    cache, or repopulate it from the GitConfigStateBase implementation
+    on-demand.
+
+    This design assumes no other processes are mutating git config state, which
+    is typically true for git_cl and other short-lived programs in depot_tools
+    which use scm.py.
+    """
+
+    def __init__(self, impl: GitConfigStateBase):
+        """Initializes a git config cache against the given underlying
+        GitConfigStateBase (either GitConfigStateReal or GitConfigStateTest).
+        """
+        self._impl: GitConfigStateBase = impl
+
+        # Actual cached configuration from the point of view of this root.
+        self._config: Optional[GitFlatConfigData] = None
+
+    def _maybe_load_config(self) -> GitFlatConfigData:
+        if self._config is None:
+            self._config = self._impl.load_config()
+        return self._config
+
+    def clear_cache(self):
+        self._config = None
+
+    def GetConfig(self,
+                  key: str,
+                  default: Optional[str] = None) -> Optional[str]:
+        """Lazily loads all configration observable for this CachedGitConfigState,
+        then returns the last value for `key` as a string.
+
+        If `key` is missing, returns default.
+        """
+        values = self._maybe_load_config().get(key, None)
+        if not values:
+            return default
+
+        return values[-1]
+
+    def GetConfigBool(self, key: str) -> bool:
+        """Returns the booleanized value of `key`.
+
+        This follows `git config` semantics (i.e. it normalizes the string value
+        of the config value to "true" - all other string values return False).
+        """
+        return self.GetConfig(key) == 'true'
+
+    def GetConfigList(self, key: str) -> List[str]:
+        """Returns all values of `key` as a list of strings."""
+        return self._maybe_load_config().get(key, [])
+
+    def YieldConfigRegexp(self, pattern: str) -> Iterable[Tuple[str, str]]:
+        """Yields (key, value) pairs for any config keys matching `pattern`.
+
+        This use re.match, so `pattern` needs to be for the entire config key.
+        """
+        p = re.compile(pattern)
+        for name, values in sorted(self._maybe_load_config().items()):
+            if p.match(name):
+                for value in values:
+                    yield name, value
+
+    def SetConfig(self,
+                  key,
+                  value=None,
+                  *,
+                  append: bool = False,
+                  missing_ok: bool = True,
+                  modify_all: bool = False,
+                  scope: GitConfigScope = 'local',
+                  value_pattern: Optional[str] = None):
+        """Sets or unsets one or more config values.
+
+        Args:
+            cwd: path to set `git config` for.
+            key: The specific config key to affect.
+            value: The value to set. If this is None, `key` will be unset.
+            append: If True and `value` is not None, this will append
+                the value instead of replacing an existing one.
+            missing_ok: If `value` is None (i.e. this is an unset operation),
+                ignore retcode=5 from `git config` (meaning that the value is
+                not present). If `value` is not None, then this option has no
+                effect.
+            modify_all: If True, this will change a set operation to
+                `--replace-all`, and will change an unset operation to
+                `--unset-all`.
+            scope: By default this is the local scope, but could be `system`,
+                `global`, or `worktree`, depending on which config scope you
+                want to affect.
+            value_pattern: For use with `modify_all=True`, allows
+                further filtering of the set or unset operation based on
+                the currently configured value. Ignored for
+                `modify_all=False`.
+        """
+        if value is None:
+            if key not in self._maybe_load_config():
+                if missing_ok:
+                    # Implements early return - underlying state doesn't need to
+                    # be told to remove already-missing keys.
+                    #
+                    # For GitConfigStateReal this avoids lots of unnecessary
+                    # subprocess invocations to unset keys which don't exist.
+                    return
+
+                raise ValueError(
+                    f'CachedGitConfigState: Cannot unset missing key {key!r}'
+                    ' with missing_ok=False.')
+
+            if modify_all:
+                self._impl.unset_config_multi(key,
+                                              value_pattern=value_pattern,
+                                              scope=scope)
+            else:
+                self._impl.unset_config(key, scope=scope)
+        else:
+            if modify_all:
+                self._impl.set_config_multi(key,
+                                            value,
+                                            append=append,
+                                            value_pattern=value_pattern,
+                                            scope=scope)
+            else:
+                self._impl.set_config(key, value, append=append, scope=scope)
+
+        # Once the underlying storage has set the value, we clear our cache so
+        # the next getter will reload it.
+        self.clear_cache()
+
+
+class GitConfigStateReal(GitConfigStateBase):
+    """GitConfigStateReal implements CachedGitConfigState by actually interacting with
+    the git configuration files on disk via GIT.Capture.
+    """
+
+    def __init__(self, root: pathlib.Path):
+        super().__init__()
+        self.root = root
+
+    def load_config(self) -> GitFlatConfigData:
+        try:
+            rawConfig = GIT.Capture(['config', '--list', '-z'],
+                                    cwd=self.root,
+                                    strip_out=False)
+        except subprocess2.CalledProcessError:
+            return {}
+
+        cfg: Dict[str, List[str]] = defaultdict(list)
+
+        # Splitting by '\x00' gets an additional empty string at the end.
+        for line in rawConfig.split('\x00')[:-1]:
+            key, value = map(str.strip, line.split('\n', 1))
+            cfg[key].append(value)
+
+        return cfg
+
+    def set_config(self, key: str, value: str, *, append: bool,
+                   scope: GitConfigScope):
+        args = ['config', f'--{scope}', key, value]
+        if append:
+            args.append('--add')
+        GIT.Capture(args, cwd=self.root)
+
+    def set_config_multi(self, key: str, value: str, *, append: bool,
+                         value_pattern: Optional[str], scope: GitConfigScope):
+        args = ['config', f'--{scope}', '--replace-all', key, value]
+        if value_pattern is not None:
+            args.append(value_pattern)
+        if append:
+            args.append('--add')
+        GIT.Capture(args, cwd=self.root)
+
+    def unset_config(self, key: str, *, scope: GitConfigScope):
+        GIT.Capture(['config', f'--{scope}', '--unset', key], cwd=self.root)
+
+    def unset_config_multi(self, key: str, *, value_pattern: Optional[str],
+                           scope: GitConfigScope):
+        args = ['config', f'--{scope}', '--unset-all', key]
+        if value_pattern is not None:
+            args.append(value_pattern)
+        GIT.Capture(args, cwd=self.root)
+
+
+class GitConfigStateTest(GitConfigStateBase):
+    """A fake implementation of GitConfigStateBase for testing."""
+
+    def __init__(self,
+                 initial_state: Optional[Dict[GitConfigScope,
+                                              GitFlatConfigData]] = None):
+        self.state: Dict[GitConfigScope, Dict[str, List[str]]] = {}
+        if initial_state is not None:
+            # We want to copy initial_state to make it mutable inside our class.
+            for scope, data in initial_state.items():
+                self.state[scope] = {k: list(v) for k, v in data.items()}
+        super().__init__()
+
+    def _get_scope(self, scope: GitConfigScope) -> Dict[str, List[str]]:
+        ret = self.state.get(scope, None)
+        if ret is None:
+            ret = {}
+            self.state[scope] = ret
+        return ret
+
+    def load_config(self) -> GitFlatConfigData:
+        ret = {}
+        for scope in GitScopeOrder:
+            for key, value in self._get_scope(scope).items():
+                curvals = ret.get(key, None)
+                if curvals is None:
+                    curvals = []
+                    ret[key] = curvals
+                curvals.extend(value)
+        return ret
+
+    def set_config(self, key: str, value: str, *, append: bool,
+                   scope: GitConfigScope):
+        cfg = self._get_scope(scope)
+        cur = cfg.get(key)
+        if cur is None or len(cur) == 1:
+            if append:
+                cfg[key] = (cur or []) + [value]
+            else:
+                cfg[key] = [value]
+            return
+        raise ValueError(f'GitConfigStateTest: Cannot set key {key} '
+                         f'- current value {cur!r} is multiple.')
+
+    def set_config_multi(self, key: str, value: str, *, append: bool,
+                         value_pattern: Optional[str], scope: GitConfigScope):
+        cfg = self._get_scope(scope)
+        cur = cfg.get(key)
+        if value_pattern is None or cur is None:
+            if append:
+                cfg[key] = (cur or []) + [value]
+            else:
+                cfg[key] = [value]
+            return
+
+        pat = re.compile(value_pattern)
+        newval = [v for v in cur if pat.match(v)]
+        newval.append(value)
+        cfg[key] = newval
+
+    def unset_config(self, key: str, *, scope: GitConfigScope):
+        cfg = self._get_scope(scope)
+        cur = cfg.get(key)
+        if cur is None or len(cur) == 1:
+            del cfg[key]
+            return
+        raise ValueError(f'GitConfigStateTest: Cannot unset key {key} '
+                         f'- current value {cur!r} is multiple.')
+
+    def unset_config_multi(self, key: str, *, value_pattern: Optional[str],
+                           scope: GitConfigScope):
+        cfg = self._get_scope(scope)
+        if value_pattern is None:
+            del cfg[key]
+            return
+
+        cur = cfg.get(key)
+        if cur is None:
+            del cfg[key]
+            return
+
+        pat = re.compile(value_pattern)
+        cfg[key] = [v for v in cur if not pat.match(v)]
+
+
 class GIT(object):
     current_version = None
     rev_parse_cache = {}
@@ -49,44 +396,35 @@ class GIT(object):
     # This cache speeds up all `git config ...` operations by only running a
     # single subcommand, which can greatly accelerate things like
     # git-map-branches.
-    _CONFIG_CACHE: Mapping[str, Mapping[str, List[str]]] = {}
-
-    @staticmethod
-    def _load_config(cwd: str) -> Mapping[str, List[str]]:
-        """Loads git config for the given cwd.
-
-        The calls to this method are cached in-memory for performance. The
-        config is only reloaded on cache misses.
+    _CONFIG_CACHE: Dict[pathlib.Path, Optional[CachedGitConfigState]] = {}
+    _CONFIG_CACHE_LOCK = threading.Lock()
 
-        Args:
-            cwd: path to fetch `git config` for.
+    @classmethod
+    def drop_config_cache(cls):
+        """Completely purges all cached git config data.
 
-        Returns:
-            A dict mapping git config keys to a list of its values.
+        This should always be safe to call (it will be lazily repopulated), but
+        really is only meant to be called from tests.
         """
-        if cwd not in GIT._CONFIG_CACHE:
-            try:
-                rawConfig = GIT.Capture(['config', '--list', '-z'],
-                                        cwd=cwd,
-                                        strip_out=False)
-            except subprocess2.CalledProcessError:
-                return {}
-
-            cfg = defaultdict(list)
-
-            # Splitting by '\x00' gets an additional empty string at the end.
-            for line in rawConfig.split('\x00')[:-1]:
-                key, value = map(str.strip, line.split('\n', 1))
-                cfg[key].append(value)
-
-            GIT._CONFIG_CACHE[cwd] = cfg
-
-        return GIT._CONFIG_CACHE[cwd]
+        with cls._CONFIG_CACHE_LOCK:
+            cls._CONFIG_CACHE = {}
 
     @staticmethod
-    def _clear_config(cwd: str) -> None:
-        GIT._CONFIG_CACHE.pop(cwd, None)
+    def _new_config_state(root: pathlib.Path) -> GitConfigStateBase:
+        """_new_config_state is mocked in tests/scm_mock to return
+        a GitConfigStateTest."""
+        return GitConfigStateReal(root)
 
+    @classmethod
+    def _get_config_state(cls, cwd: str) -> CachedGitConfigState:
+        key = pathlib.Path(cwd).absolute()
+        with cls._CONFIG_CACHE_LOCK:
+            cur = GIT._CONFIG_CACHE.get(key, None)
+            if cur is not None:
+                return cur
+            ret = CachedGitConfigState(cls._new_config_state(key))
+            cls._CONFIG_CACHE[key] = ret
+            return ret
 
     @staticmethod
     def ApplyEnvVars(kwargs):
@@ -152,29 +490,34 @@ class GIT(object):
         return results
 
     @staticmethod
-    def GetConfig(cwd, key, default=None):
-        values = GIT._load_config(cwd).get(key, None)
-        if not values:
-            return default
+    def GetConfig(cwd: str,
+                  key: str,
+                  default: Optional[str] = None) -> Optional[str]:
+        """Lazily loads all configration observable for this CachedGitConfigState,
+        then returns the last value for `key` as a string.
 
-        return values[-1]
+        If `key` is missing, returns default.
+        """
+        return GIT._get_config_state(cwd).GetConfig(key, default)
 
     @staticmethod
-    def GetConfigBool(cwd, key) -> bool:
-        return GIT.GetConfig(cwd, key) == 'true'
+    def GetConfigBool(cwd: str, key: str) -> bool:
+        """Returns the booleanized value of `key`.
+
+        This follows `git config` semantics (i.e. it normalizes the string value
+        of the config value to "true" - all other string values return False).
+        """
+        return GIT._get_config_state(cwd).GetConfigBool(key)
 
     @staticmethod
-    def GetConfigList(cwd, key):
-        return GIT._load_config(cwd).get(key, [])
+    def GetConfigList(cwd: str, key: str) -> List[str]:
+        """Returns all values of `key` as a list of strings."""
+        return GIT._get_config_state(cwd).GetConfigList(key)
 
     @staticmethod
-    def YieldConfigRegexp(cwd, pattern):
+    def YieldConfigRegexp(cwd: str, pattern: str) -> Iterable[Tuple[str, str]]:
         """Yields (key, value) pairs for any config keys matching `pattern`."""
-        p = re.compile(pattern)
-        for name, values in GIT._load_config(cwd).items():
-            if p.match(name):
-                for value in values:
-                    yield name, value
+        yield from GIT._get_config_state(cwd).YieldConfigRegexp(pattern)
 
     @staticmethod
     def GetBranchConfig(cwd, branch, key, default=None):
@@ -183,17 +526,15 @@ class GIT(object):
         return GIT.GetConfig(cwd, key, default)
 
     @staticmethod
-    def SetConfig(
-        cwd,
-        key,
-        value=None,
-        *,
-        append=False,
-        missing_ok=True,
-        modify_all=False,
-        scope='local',
-        value_pattern=None,
-    ):
+    def SetConfig(cwd: str,
+                  key: str,
+                  value: Optional[str] = None,
+                  *,
+                  append: bool = False,
+                  missing_ok: bool = True,
+                  modify_all: bool = False,
+                  scope: GitConfigScope = 'local',
+                  value_pattern: Optional[str] = None):
         """Sets or unsets one or more config values.
 
         Args:
@@ -217,26 +558,13 @@ class GIT(object):
                 the currently configured value. Ignored for
                 `modify_all=False`.
         """
-        GIT._clear_config(cwd)
-
-        args = ['config', f'--{scope}']
-        if value == None:
-            args.extend(['--unset' + ('-all' if modify_all else ''), key])
-        else:
-            if modify_all:
-                args.append('--replace-all')
-            if append:
-                args.append('--add')
-            args.extend([key, value])
-
-        if modify_all and value_pattern:
-            args.append(value_pattern)
-
-        accepted_retcodes = [0]
-        if value is None and missing_ok:
-            accepted_retcodes = [0, 5]
-
-        GIT.Capture(args, cwd=cwd, accepted_retcodes=accepted_retcodes)
+        GIT._get_config_state(cwd).SetConfig(key,
+                                             value,
+                                             append=append,
+                                             missing_ok=missing_ok,
+                                             modify_all=modify_all,
+                                             scope=scope,
+                                             value_pattern=value_pattern)
 
     @staticmethod
     def SetBranchConfig(cwd, branch, key, value=None):

tests/gerrit_util_test.py

@@ -21,9 +21,11 @@ import httplib2
 
 sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 
+import scm_mock
+
 import gerrit_util
-import git_common
 import metrics
+import scm
 import subprocess2
 
 RUN_SUBPROC_TESTS = 'RUN_SUBPROC_TESTS' in os.environ
@@ -104,6 +106,7 @@ class CookiesAuthenticatorTest(unittest.TestCase):
                    return_value=self._GITCOOKIES).start()
         mock.patch('os.getenv', return_value={}).start()
         mock.patch('os.environ', {'HOME': '$HOME'}).start()
+        mock.patch('os.getcwd', return_value='/fame/cwd').start()
         mock.patch('os.path.exists', return_value=True).start()
         mock.patch(
             'git_common.run',
@@ -111,6 +114,8 @@ class CookiesAuthenticatorTest(unittest.TestCase):
                 subprocess2.CalledProcessError(1, ['cmd'], 'cwd', 'out', 'err')
             ],
         ).start()
+        scm_mock.GIT(self)
+
         self.addCleanup(mock.patch.stopall)
         self.maxDiff = None
 
@@ -144,16 +149,10 @@ class CookiesAuthenticatorTest(unittest.TestCase):
             os.path.expanduser(os.path.join('~', '.gitcookies')),
             gerrit_util.CookiesAuthenticator().get_gitcookies_path())
 
-        git_common.run.side_effect = ['http.cookiefile\nhttp.cookiefile\x00']
+        scm.GIT.SetConfig(os.getcwd(), 'http.cookiefile', '/some/path')
         self.assertEqual(
-            'http.cookiefile',
+            '/some/path',
             gerrit_util.CookiesAuthenticator().get_gitcookies_path())
-        git_common.run.assert_called_with('config',
-                                          '--list',
-                                          '-z',
-                                          autostrip=False,
-                                          cwd=os.getcwd(),
-                                          env=mock.ANY)
 
         os.getenv.return_value = 'git-cookies-path'
         self.assertEqual(
@@ -717,9 +716,17 @@ class ShouldUseSSOTest(unittest.TestCase):
     def setUp(self) -> None:
         self.newauth = mock.patch('newauth.Enabled', return_value=True)
         self.newauth.start()
+
+        self.cwd = mock.patch('os.getcwd', return_value='/fake/cwd')
+        self.cwd.start()
+
         self.sso = mock.patch('gerrit_util.ssoHelper.find_cmd',
                               return_value='/fake/git-remote-sso')
         self.sso.start()
+
+        scm_mock.GIT(self)
+
+        self.addCleanup(mock.patch.stopall)
         return super().setUp()
 
     def tearDown(self) -> None:

tests/git_cl_test.py

@@ -17,10 +17,13 @@ import shutil
 import sys
 import tempfile
 import unittest
+
 from unittest import mock
 
 sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 
+import scm_mock
+
 import metrics
 import metrics_utils
 # We have to disable monitoring before importing git_cl.
@@ -84,33 +87,6 @@ class ChangelistMock(object):
         return ('origin', 'refs/remotes/origin/main')
 
 
-class GitMocks(object):
-    def __init__(self, config=None, branchref=None):
-        self.branchref = branchref or 'refs/heads/main'
-        self.config = config or {}
-
-    def GetBranchRef(self, _root):
-        return self.branchref
-
-    def NewBranch(self, branchref):
-        self.branchref = branchref
-
-    def GetConfig(self, root, key, default=None):
-        if root != '':
-            key = '%s:%s' % (root, key)
-        return self.config.get(key, default)
-
-    def SetConfig(self, root, key, value=None):
-        if root != '':
-            key = '%s:%s' % (root, key)
-        if value:
-            self.config[key] = value
-            return
-        if key not in self.config:
-            raise CERR1
-        del self.config[key]
-
-
 class WatchlistsMock(object):
     def __init__(self, _):
         pass
@@ -654,14 +630,9 @@ class TestGitCl(unittest.TestCase):
                    lambda *a: self._mocked_call('ValidAccounts', *a)).start()
         mock.patch('sys.exit', side_effect=SystemExitMock).start()
         mock.patch('git_cl.Settings.GetRoot', return_value='').start()
-        self.mockGit = GitMocks()
-        mock.patch('scm.GIT.GetBranchRef', self.mockGit.GetBranchRef).start()
-        mock.patch('scm.GIT.GetConfig', self.mockGit.GetConfig).start()
+        scm_mock.GIT(self)
         mock.patch('scm.GIT.ResolveCommit', return_value='hash').start()
         mock.patch('scm.GIT.IsValidRevision', return_value=True).start()
-        mock.patch('scm.GIT.SetConfig', self.mockGit.SetConfig).start()
-        mock.patch('git_new_branch.create_new_branch',
-                   self.mockGit.NewBranch).start()
         mock.patch('scm.GIT.FetchUpstreamTuple',
                    return_value=('origin', 'refs/heads/main')).start()
         mock.patch('scm.GIT.CaptureStatus',
@@ -864,7 +835,8 @@ class TestGitCl(unittest.TestCase):
         calls = []
 
         if squash_mode in ('override_squash', 'override_nosquash'):
-            self.mockGit.config['gerrit.override-squash-uploads'] = (
+            scm.GIT.SetConfig(
+                '', 'gerrit.override-squash-uploads',
                 'true' if squash_mode == 'override_squash' else 'false')
 
         if not git_footers.get_footer_change_id(description) and not squash:
@@ -1244,12 +1216,12 @@ class TestGitCl(unittest.TestCase):
             'gclient_utils.AskForData',
             lambda prompt: self._mocked_call('ask_for_data', prompt)).start()
 
-        self.mockGit.config['gerrit.host'] = 'true'
-        self.mockGit.config['branch.main.gerritissue'] = (str(issue)
-                                                          if issue else None)
-        self.mockGit.config['remote.origin.url'] = (
-            'https://%s.googlesource.com/my/repo' % short_hostname)
-        self.mockGit.config['user.email'] = 'owner@example.com'
+        scm.GIT.SetConfig('', 'gerrit.host', 'true')
+        scm.GIT.SetConfig('', 'branch.main.gerritissue',
+                          (str(issue) if issue else None))
+        scm.GIT.SetConfig('', 'remote.origin.url',
+                          f'https://{short_hostname}.googlesource.com/my/repo')
+        scm.GIT.SetConfig('', 'user.email', 'owner@example.com')
 
         self.calls = []
         if squash_mode == "override_nosquash":
@@ -1421,8 +1393,8 @@ class TestGitCl(unittest.TestCase):
         mockUploadAllPrecheck.return_value = (cls, True)
 
         upstream_gerrit_commit = 'upstream-commit'
-        self.mockGit.config[
-            'branch.upstream-branch.gerritsquashhash'] = upstream_gerrit_commit
+        scm.GIT.SetConfig('', 'branch.upstream-branch.gerritsquashhash',
+                          upstream_gerrit_commit)
 
         reviewers = []
         ccs = []
@@ -1730,13 +1702,13 @@ class TestGitCl(unittest.TestCase):
         # (so no LAST_UPLOAD_HASH_CONIFG_KEY)
 
         # Case 4: upstream2's last_upload is behind upstream3's base_commit
-        self.mockGit.config['branch.upstream2.%s' %
-                            git_cl.LAST_UPLOAD_HASH_CONFIG_KEY] = 'commit2.3'
+        key = f'branch.upstream2.{git_cl.LAST_UPLOAD_HASH_CONFIG_KEY}'
+        scm.GIT.SetConfig('', key, 'commit2.3')
         mockIsAncestor.side_effect = [True]
 
         # Case 3: upstream1's last_upload matches upstream2's base_commit
-        self.mockGit.config['branch.upstream1.%s' %
-                            git_cl.LAST_UPLOAD_HASH_CONFIG_KEY] = 'commit1.5'
+        key = f'branch.upstream1.{git_cl.LAST_UPLOAD_HASH_CONFIG_KEY}'
+        scm.GIT.SetConfig('', key, 'commit1.5')
 
         cls, cherry_pick = git_cl._UploadAllPrecheck(options, orig_args)
         self.assertFalse(cherry_pick)
@@ -1858,8 +1830,8 @@ class TestGitCl(unittest.TestCase):
         mockGitGetBranchConfigValue.return_value = None
 
         # Case 5: current's base_commit is behind upstream3's last_upload.
-        self.mockGit.config['branch.upstream3.%s' %
-                            git_cl.LAST_UPLOAD_HASH_CONFIG_KEY] = 'commit3.7'
+        key = f'branch.upstream3.{git_cl.LAST_UPLOAD_HASH_CONFIG_KEY}'
+        scm.GIT.SetConfig('', key, 'commit3.7')
         mockIsAncestor.side_effect = [False, True]
         with self.assertRaises(SystemExitMock):
             options = optparse.Values()
@@ -1904,8 +1876,8 @@ class TestGitCl(unittest.TestCase):
         mockIsAncestor.return_value = True
 
         # Give upstream3 a last upload hash
-        self.mockGit.config['branch.upstream3.%s' %
-                            git_cl.LAST_UPLOAD_HASH_CONFIG_KEY] = 'commit3.4'
+        key = f'branch.upstream3.{git_cl.LAST_UPLOAD_HASH_CONFIG_KEY}'
+        scm.GIT.SetConfig('', key, 'commit3.4')
 
         # end commits
         mockRunGit.return_value = 'commit4'
@@ -2391,8 +2363,8 @@ class TestGitCl(unittest.TestCase):
 
     def _patch_common(self, git_short_host='chromium'):
         mock.patch('scm.GIT.ResolveCommit', return_value='deadbeef').start()
-        self.mockGit.config['remote.origin.url'] = (
-            'https://%s.googlesource.com/my/repo' % git_short_host)
+        scm.GIT.SetConfig('', 'remote.origin.url',
+                          f'https://{git_short_host}.googlesource.com/my/repo')
         gerrit_util.GetChangeDetail.return_value = {
             'current_revision': '7777777777',
             'revisions': {
@@ -2505,8 +2477,8 @@ class TestGitCl(unittest.TestCase):
                 side_effect=gerrit_util.GerritError(404, ''))
     @mock.patch('sys.stderr', io.StringIO())
     def test_patch_gerrit_not_exists(self, *_mocks):
-        self.mockGit.config['remote.origin.url'] = (
-            'https://chromium.googlesource.com/my/repo')
+        scm.GIT.SetConfig('', 'remote.origin.url',
+                          'https://chromium.googlesource.com/my/repo')
         with self.assertRaises(SystemExitMock):
             self.assertEqual(1, git_cl.main(['patch', '123456']))
         self.assertEqual(
@@ -2550,8 +2522,8 @@ class TestGitCl(unittest.TestCase):
         mock.patch(
             'git_cl.gerrit_util.CookiesAuthenticator',
             CookiesAuthenticatorMockFactory(hosts_with_creds=auth)).start()
-        self.mockGit.config['remote.origin.url'] = (
-            'https://chromium.googlesource.com/my/repo')
+        scm.GIT.SetConfig('', 'remote.origin.url',
+                          'https://chromium.googlesource.com/my/repo')
         cl = git_cl.Changelist()
         cl.branch = 'main'
         cl.branchref = 'refs/heads/main'
@@ -2594,12 +2566,12 @@ class TestGitCl(unittest.TestCase):
         self.assertIsNone(cl.EnsureAuthenticated(force=False))
 
     def test_gerrit_ensure_authenticated_skipped(self):
-        self.mockGit.config['gerrit.skip-ensure-authenticated'] = 'true'
+        scm.GIT.SetConfig('', 'gerrit.skip-ensure-authenticated', 'true')
         cl = self._test_gerrit_ensure_authenticated_common(auth={})
         self.assertIsNone(cl.EnsureAuthenticated(force=False))
 
     def test_gerrit_ensure_authenticated_sso(self):
-        self.mockGit.config['remote.origin.url'] = 'sso://repo'
+        scm.GIT.SetConfig('', 'remote.origin.url', 'sso://repo')
 
         mock.patch(
             'git_cl.gerrit_util.CookiesAuthenticator',
@@ -2629,7 +2601,7 @@ class TestGitCl(unittest.TestCase):
         self.assertTrue('Bearer' in conn.req_headers['Authorization'])
 
     def test_gerrit_ensure_authenticated_non_https_sso(self):
-        self.mockGit.config['remote.origin.url'] = 'custom-scheme://repo'
+        scm.GIT.SetConfig('', 'remote.origin.url', 'custom-scheme://repo')
         self.calls = [
             (('logging.warning',
               'Ignoring branch %(branch)s with non-https remote '
@@ -2650,8 +2622,8 @@ class TestGitCl(unittest.TestCase):
         self.assertIsNone(cl.EnsureAuthenticated(force=False))
 
     def test_gerrit_ensure_authenticated_non_url(self):
-        self.mockGit.config['remote.origin.url'] = (
-            'git@somehost.example:foo/bar.git')
+        scm.GIT.SetConfig('', 'remote.origin.url',
+                          'git@somehost.example:foo/bar.git')
         self.calls = [
             (('logging.error',
               'Remote "%(remote)s" for branch "%(branch)s" points to "%(url)s", '
@@ -2673,11 +2645,11 @@ class TestGitCl(unittest.TestCase):
         self.assertIsNone(cl.EnsureAuthenticated(force=False))
 
     def _cmd_set_commit_gerrit_common(self, vote, notify=None):
-        self.mockGit.config['branch.main.gerritissue'] = '123'
-        self.mockGit.config['branch.main.gerritserver'] = (
-            'https://chromium-review.googlesource.com')
-        self.mockGit.config['remote.origin.url'] = (
-            'https://chromium.googlesource.com/infra/infra')
+        scm.GIT.SetConfig('', 'branch.main.gerritissue', '123')
+        scm.GIT.SetConfig('', 'branch.main.gerritserver',
+                          'https://chromium-review.googlesource.com')
+        scm.GIT.SetConfig('', 'remote.origin.url',
+                          'https://chromium.googlesource.com/infra/infra')
         self.calls = [
             (('SetReview', 'chromium-review.googlesource.com',
               'infra%2Finfra~123', None, {
@@ -2732,8 +2704,8 @@ class TestGitCl(unittest.TestCase):
         self.assertEqual(git_cl.main(['set-close', '--issue', '1']), 0)
 
     def test_description(self):
-        self.mockGit.config['remote.origin.url'] = (
-            'https://chromium.googlesource.com/my/repo')
+        scm.GIT.SetConfig('', 'remote.origin.url',
+                          'https://chromium.googlesource.com/my/repo')
         gerrit_util.GetChangeDetail.return_value = {
             'current_revision': 'sha1',
             'revisions': {
@@ -2783,7 +2755,7 @@ class TestGitCl(unittest.TestCase):
                    UpdateDescription).start()
         mock.patch('git_cl.gclient_utils.RunEditor', RunEditor).start()
 
-        self.mockGit.config['branch.main.gerritissue'] = '123'
+        scm.GIT.SetConfig('', 'branch.main.gerritissue', '123')
         self.assertEqual(0, git_cl.main(['description']))
 
     def test_description_does_not_append_bug_line_if_fixed_is_present(self):
@@ -2803,7 +2775,7 @@ class TestGitCl(unittest.TestCase):
                    lambda *args: current_desc).start()
         mock.patch('git_cl.gclient_utils.RunEditor', RunEditor).start()
 
-        self.mockGit.config['branch.main.gerritissue'] = '123'
+        scm.GIT.SetConfig('', 'branch.main.gerritissue', '123')
         self.assertEqual(0, git_cl.main(['description']))
 
     def test_description_set_stdin(self):
@@ -2950,20 +2922,20 @@ class TestGitCl(unittest.TestCase):
                             'archived/{issue}-{branch}']))
 
     def test_cmd_issue_erase_existing(self):
-        self.mockGit.config['branch.main.gerritissue'] = '123'
-        self.mockGit.config['branch.main.gerritserver'] = (
-            'https://chromium-review.googlesource.com')
+        scm.GIT.SetConfig('', 'branch.main.gerritissue', '123')
+        scm.GIT.SetConfig('', 'branch.main.gerritserver',
+                          'https://chromium-review.googlesource.com')
         self.calls = [
             ((['git', 'log', '-1', '--format=%B'], ), 'This is a description'),
         ]
         self.assertEqual(0, git_cl.main(['issue', '0']))
-        self.assertNotIn('branch.main.gerritissue', self.mockGit.config)
-        self.assertNotIn('branch.main.gerritserver', self.mockGit.config)
+        self.assertIsNone(scm.GIT.GetConfig('root', 'branch.main.gerritissue'))
+        self.assertIsNone(scm.GIT.GetConfig('root', 'branch.main.gerritserver'))
 
     def test_cmd_issue_erase_existing_with_change_id(self):
-        self.mockGit.config['branch.main.gerritissue'] = '123'
-        self.mockGit.config['branch.main.gerritserver'] = (
-            'https://chromium-review.googlesource.com')
+        scm.GIT.SetConfig('', 'branch.main.gerritissue', '123')
+        scm.GIT.SetConfig('', 'branch.main.gerritserver',
+                          'https://chromium-review.googlesource.com')
         mock.patch(
             'git_cl.Changelist.FetchDescription',
             lambda _: 'This is a description\n\nChange-Id: Ideadbeef').start()
@@ -2974,15 +2946,15 @@ class TestGitCl(unittest.TestCase):
                'This is a description\n'], ), ''),
         ]
         self.assertEqual(0, git_cl.main(['issue', '0']))
-        self.assertNotIn('branch.main.gerritissue', self.mockGit.config)
-        self.assertNotIn('branch.main.gerritserver', self.mockGit.config)
+        self.assertIsNone(scm.GIT.GetConfig('root', 'branch.main.gerritissue'))
+        self.assertIsNone(scm.GIT.GetConfig('root', 'branch.main.gerritserver'))
 
     def test_cmd_issue_json(self):
-        self.mockGit.config['branch.main.gerritissue'] = '123'
-        self.mockGit.config['branch.main.gerritserver'] = (
-            'https://chromium-review.googlesource.com')
-        self.mockGit.config['remote.origin.url'] = (
-            'https://chromium.googlesource.com/chromium/src')
+        scm.GIT.SetConfig('', 'branch.main.gerritissue', '123')
+        scm.GIT.SetConfig('', 'branch.main.gerritserver',
+                          'https://chromium-review.googlesource.com')
+        scm.GIT.SetConfig('', 'remote.origin.url',
+                          'https://chromium.googlesource.com/chromium/src')
         self.calls = [(
             (
                 'write_json',
@@ -3046,9 +3018,9 @@ class TestGitCl(unittest.TestCase):
         cl._GerritCommitMsgHookCheck(offer_removal=True)
 
     def test_GerritCmdLand(self):
-        self.mockGit.config['branch.main.gerritsquashhash'] = 'deadbeaf'
-        self.mockGit.config['branch.main.gerritserver'] = (
-            'chromium-review.googlesource.com')
+        scm.GIT.SetConfig('', 'branch.main.gerritsquashhash', 'deadbeaf')
+        scm.GIT.SetConfig('', 'branch.main.gerritserver',
+                          'chromium-review.googlesource.com')
         self.calls += [
             ((['git', 'diff', 'deadbeaf'], ), ''),  # No diff.
         ]
@@ -3222,9 +3194,9 @@ class TestGitCl(unittest.TestCase):
                       sys.stdout.getvalue())
 
     def test_git_cl_comment_add_gerrit(self):
-        self.mockGit.branchref = None
-        self.mockGit.config['remote.origin.url'] = (
-            'https://chromium.googlesource.com/infra/infra')
+        git_new_branch.create_new_branch(None)  # hits mock from scm_mock.GIT.
+        scm.GIT.SetConfig('', 'remote.origin.url',
+                          'https://chromium.googlesource.com/infra/infra')
         self.calls = [
             (('SetReview', 'chromium-review.googlesource.com',
               'infra%2Finfra~10', 'msg', None, None, None), None),
@@ -3233,8 +3205,8 @@ class TestGitCl(unittest.TestCase):
 
     @mock.patch('git_cl.Changelist.GetBranch', return_value='foo')
     def test_git_cl_comments_fetch_gerrit(self, *_mocks):
-        self.mockGit.config['remote.origin.url'] = (
-            'https://chromium.googlesource.com/infra/infra')
+        scm.GIT.SetConfig('', 'remote.origin.url',
+                          'https://chromium.googlesource.com/infra/infra')
         gerrit_util.GetChangeDetail.return_value = {
             'owner': {
                 'email': 'owner@example.com'
@@ -3388,8 +3360,8 @@ class TestGitCl(unittest.TestCase):
         # git cl comments also fetches robot comments (which are considered a
         # type of autogenerated comment), and unlike other types of comments,
         # only robot comments from the latest patchset are shown.
-        self.mockGit.config['remote.origin.url'] = (
-            'https://x.googlesource.com/infra/infra')
+        scm.GIT.SetConfig('', 'remote.origin.url',
+                          'https://x.googlesource.com/infra/infra')
         gerrit_util.GetChangeDetail.return_value = {
             'owner': {
                 'email': 'owner@example.com'
@@ -3505,8 +3477,8 @@ class TestGitCl(unittest.TestCase):
         mock.patch('os.path.isdir', selective_os_path_isdir_mock).start()
 
         url = 'https://chromium.googlesource.com/my/repo'
-        self.mockGit.config['remote.origin.url'] = ('/cache/this-dir-exists')
-        self.mockGit.config['/cache/this-dir-exists:remote.origin.url'] = (url)
+        scm.GIT.SetConfig('', 'remote.origin.url', '/cache/this-dir-exists')
+        scm.GIT.SetConfig('/cache/this-dir-exists', 'remote.origin.url', url)
         self.calls = [
             (('os.path.isdir', '/cache/this-dir-exists'), True),
         ]
@@ -3526,8 +3498,8 @@ class TestGitCl(unittest.TestCase):
         mock.patch('logging.error',
                    lambda *a: self._mocked_call('logging.error', *a)).start()
 
-        self.mockGit.config['remote.origin.url'] = (
-            '/cache/this-dir-doesnt-exist')
+        scm.GIT.SetConfig('', 'remote.origin.url',
+                          '/cache/this-dir-doesnt-exist')
         self.calls = [
             (('os.path.isdir', '/cache/this-dir-doesnt-exist'), False),
             (('logging.error',
@@ -3553,7 +3525,7 @@ class TestGitCl(unittest.TestCase):
         mock.patch('logging.error',
                    lambda *a: self._mocked_call('logging.error', *a)).start()
 
-        self.mockGit.config['remote.origin.url'] = ('/cache/this-dir-exists')
+        scm.GIT.SetConfig('', 'remote.origin.url', '/cache/this-dir-exists')
         self.calls = [
             (('os.path.isdir', '/cache/this-dir-exists'), True),
             (('logging.error',
@@ -3570,8 +3542,8 @@ class TestGitCl(unittest.TestCase):
         self.assertIsNone(cl.GetRemoteUrl())
 
     def test_gerrit_change_identifier_with_project(self):
-        self.mockGit.config['remote.origin.url'] = (
-            'https://chromium.googlesource.com/a/my/repo.git/')
+        scm.GIT.SetConfig('', 'remote.origin.url',
+                          'https://chromium.googlesource.com/a/my/repo.git/')
         cl = git_cl.Changelist(issue=123456)
         self.assertEqual(cl._GerritChangeIdentifier(), 'my%2Frepo~123456')
 
@@ -3641,11 +3613,10 @@ class ChangelistTest(unittest.TestCase):
                    return_value='project').start()
         mock.patch('sys.exit', side_effect=SystemExitMock).start()
 
+        scm_mock.GIT(self)
+
         self.addCleanup(mock.patch.stopall)
         self.temp_count = 0
-        self.mockGit = GitMocks()
-        mock.patch('scm.GIT.GetConfig', self.mockGit.GetConfig).start()
-        mock.patch('scm.GIT.SetConfig', self.mockGit.SetConfig).start()
         gerrit_util._Authenticator._resolved = None
 
     def testRunHook(self):
@@ -4238,10 +4209,10 @@ class ChangelistTest(unittest.TestCase):
         cl.PostUploadUpdates(options, new_upload, '12345')
         mockSetPatchset.assert_called_once_with(3)
         self.assertEqual(
-            self.mockGit.config['root:branch.current-branch.gerritsquashhash'],
+            scm.GIT.GetConfig('root', 'branch.current-branch.gerritsquashhash'),
             new_upload.commit_to_push)
         self.assertEqual(
-            self.mockGit.config['root:branch.current-branch.last-upload-hash'],
+            scm.GIT.GetConfig('root', 'branch.current-branch.last-upload-hash'),
             new_upload.new_last_uploaded_commit)
 
         mockAddReviewers.assert_called_once_with('chromium',

tests/git_common_test.py

@@ -458,7 +458,7 @@ class GitMutableFunctionsTest(git_test_utils.GitRepoReadWriteTestBase,
                          [])
 
         self.repo.git('config', '--add', 'happy.derpies', 'cat')
-        self.gc.scm.GIT._clear_config(self.repo.repo_path)
+        self.gc.scm.GIT.drop_config_cache()
         self.assertEqual(
             self.repo.run(self.gc.get_config_list, 'happy.derpies'),
             ['food', 'cat'])
@@ -482,19 +482,19 @@ class GitMutableFunctionsTest(git_test_utils.GitRepoReadWriteTestBase,
 
         self.repo.git('config', 'depot-tools.upstream', 'catfood')
 
-        self.gc.scm.GIT._clear_config(self.repo.repo_path)
+        self.gc.scm.GIT.drop_config_cache()
         self.assertEqual('catfood', self.repo.run(self.gc.root))
 
         self.repo.git('config', '--add', 'core.fsmonitor', 'true')
-        self.gc.scm.GIT._clear_config(self.repo.repo_path)
+        self.gc.scm.GIT.drop_config_cache()
         self.assertEqual(True, self.repo.run(self.gc.is_fsmonitor_enabled))
 
         self.repo.git('config', '--add', 'core.fsmonitor', 't')
-        self.gc.scm.GIT._clear_config(self.repo.repo_path)
+        self.gc.scm.GIT.drop_config_cache()
         self.assertEqual(False, self.repo.run(self.gc.is_fsmonitor_enabled))
 
         self.repo.git('config', '--add', 'core.fsmonitor', 'false')
-        self.gc.scm.GIT._clear_config(self.repo.repo_path)
+        self.gc.scm.GIT.drop_config_cache()
         self.assertEqual(False, self.repo.run(self.gc.is_fsmonitor_enabled))
 
     def testRoot(self):
@@ -659,7 +659,7 @@ class GitMutableStructuredTest(git_test_utils.GitRepoReadWriteTestBase,
             self.repo.run(self.gc.get_or_create_merge_base, 'branch_L',
                           'branch_K'))
 
-        self.gc.scm.GIT._clear_config(self.repo.repo_path)
+        self.gc.scm.GIT.drop_config_cache()
         self.assertEqual(
             self.repo['B'],
             self.repo.run(self.gc.get_config, 'branch.branch_K.base'))
@@ -680,7 +680,7 @@ class GitMutableStructuredTest(git_test_utils.GitRepoReadWriteTestBase,
         self.repo.run(self.gc.manual_merge_base, 'branch_K', self.repo['I'],
                       'branch_G')
 
-        self.gc.scm.GIT._clear_config(self.repo.repo_path)
+        self.gc.scm.GIT.drop_config_cache()
         self.assertEqual(
             self.repo['I'],
             self.repo.run(self.gc.get_or_create_merge_base, 'branch_K',

tests/scm_mock.py

@@ -0,0 +1,54 @@
+# Copyright (c) 2024 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import os
+import sys
+
+from typing import Dict, List, Optional
+from unittest import mock
+import unittest
+
+# This is to be able to import scm from the root of the repo.
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+import scm
+
+
+def GIT(test: unittest.TestCase,
+        config: Optional[Dict[str, List[str]]] = None,
+        branchref: Optional[str] = None):
+    """Installs fakes/mocks for scm.GIT so that:
+
+      * Initial git config (local scope) is set to `config`.
+      * GetBranch will just return a fake branchname starting with the value of
+        branchref.
+      * git_new_branch.create_new_branch will be mocked to update the value
+        returned by GetBranch.
+
+    NOTE: The dependency on git_new_branch.create_new_branch seems pretty
+    circular - this functionality should probably move to scm.GIT?
+    """
+    _branchref = [branchref or 'refs/heads/main']
+
+    initial_state = {}
+    if config is not None:
+        initial_state['local'] = config
+
+    def _newBranch(branchref):
+        _branchref[0] = branchref
+
+    patches: List[mock._patch] = [
+        mock.patch(
+            'scm.GIT._new_config_state',
+            side_effect=lambda root: scm.GitConfigStateTest(initial_state)),
+        mock.patch('scm.GIT.GetBranchRef',
+                   side_effect=lambda _root: _branchref[0]),
+        mock.patch('git_new_branch.create_new_branch', side_effect=_newBranch)
+    ]
+
+    for p in patches:
+        p.start()
+        test.addCleanup(p.stop)
+
+    test.addCleanup(scm.GIT.drop_config_cache)

tests/scm_unittest.py

@@ -206,7 +206,9 @@ class RealGitTest(fake_repos.FakeReposTestBase):
         self.assertEqual('', scm.GIT.GetConfig(self.cwd, key))
         self.assertEqual('', scm.GIT.GetConfig(self.cwd, key, 'default-value'))
 
-        scm.GIT._clear_config(self.cwd)
+        # Clear the cache because we externally manipulate the git config with
+        # the subprocess call.
+        scm.GIT.drop_config_cache()
         subprocess.run(['git', 'config', key, 'line 1\nline 2\nline 3'],
                        cwd=self.cwd)
         self.assertEqual('line 1\nline 2\nline 3',