[scm.py] Fix error with missing_ok in GitConfig.

Previously there was an error where you could have:

  global: {"key": ["something"]}
  local: {}

And called SetConfig("key", None, missing_ok=False) - this would end
up raising CalledProcessError(returncode=5) because the caching
layer would see that "key" was not missing, then call into the
storage layer which would do `git config --local ...` which was
missing "key" (and therefore fail).

R=ayatane, yiwzhang

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

scm.py

@@ -97,23 +97,38 @@ class GitConfigStateBase(metaclass=abc.ABCMeta):
         """
 
     @abc.abstractmethod
-    def unset_config(self, key: str, *, scope: GitConfigScope):
+    def unset_config(self, key: str, *, scope: GitConfigScope,
+                     missing_ok: bool):
         """When invoked, remove a singlar value from `key` in this state's underlying data.
+
+        If missing_ok is False and `key` is not present in the given scope, this
+        must raise GitConfigUnsetMissingValue with `key`.
         """
 
     @abc.abstractmethod
     def unset_config_multi(self, key: str, *, value_pattern: Optional[str],
-                           scope: GitConfigScope):
+                           scope: GitConfigScope, missing_ok: bool):
         """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.
 
+        If missing_ok is False and `key` is not present in the given scope, this
+        must raise GitConfigUnsetMissingValue with `key`.
+
         TODO: Make value_pattern an re.Pattern. This wasn't done at the time of
         this refactor to keep the refactor small.
         """
 
 
+class GitConfigUnsetMissingValue(ValueError):
+
+    def __init__(self, key: str, scope: str) -> None:
+        super().__init__(
+            f'Cannot unset missing key {key!r} in scope {scope!r} with missing_ok=False.'
+        )
+
+
 class CachedGitConfigState(object):
     """This represents the observable git configuration state for a given
     repository (whose top-level path is `root`).
@@ -210,7 +225,8 @@ class CachedGitConfigState(object):
             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.
+                effect. If this is false and the key is missing, this will raise
+                GitConfigUnsetMissingValue.
             modify_all: If True, this will change a set operation to
                 `--replace-all`, and will change an unset operation to
                 `--unset-all`.
@@ -223,25 +239,13 @@ class CachedGitConfigState(object):
                 `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)
+                                              scope=scope,
+                                              missing_ok=missing_ok)
             else:
-                self._impl.unset_config(key, scope=scope)
+                self._impl.unset_config(key, scope=scope, missing_ok=missing_ok)
         else:
             if modify_all:
                 self._impl.set_config_multi(key,
@@ -299,15 +303,32 @@ class GitConfigStateReal(GitConfigStateBase):
             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(self, key: str, *, scope: GitConfigScope,
+                     missing_ok: bool):
+        accepted_retcodes = (0, 5) if missing_ok else (0, )
+        try:
+            GIT.Capture(['config', f'--{scope}', '--unset', key],
+                        cwd=self.root,
+                        accepted_retcodes=accepted_retcodes)
+        except subprocess2.CalledProcessError as cpe:
+            if cpe.returncode == 5:
+                raise GitConfigUnsetMissingValue(key, scope)
+            raise
 
     def unset_config_multi(self, key: str, *, value_pattern: Optional[str],
-                           scope: GitConfigScope):
+                           scope: GitConfigScope, missing_ok: bool):
+        accepted_retcodes = (0, 5) if missing_ok else (0, )
         args = ['config', f'--{scope}', '--unset-all', key]
         if value_pattern is not None:
             args.append(value_pattern)
-        GIT.Capture(args, cwd=self.root)
+        try:
+            GIT.Capture(args,
+                        cwd=self.root,
+                        accepted_retcodes=accepted_retcodes)
+        except subprocess2.CalledProcessError as cpe:
+            if cpe.returncode == 5:
+                raise GitConfigUnsetMissingValue(key, scope)
+            raise
 
 
 class GitConfigStateTest(GitConfigStateBase):
@@ -370,23 +391,33 @@ class GitConfigStateTest(GitConfigStateBase):
         newval.append(value)
         cfg[key] = newval
 
-    def unset_config(self, key: str, *, scope: GitConfigScope):
+    def unset_config(self, key: str, *, scope: GitConfigScope,
+                     missing_ok: bool):
         cfg = self._get_scope(scope)
         cur = cfg.get(key)
-        if cur is None or len(cur) == 1:
+        if cur is None:
+            if missing_ok:
+                return
+            raise GitConfigUnsetMissingValue(key, scope)
+        if 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):
+                           scope: GitConfigScope, missing_ok: bool):
         cfg = self._get_scope(scope)
+        cur = cfg.get(key)
+        if cur is None:
+            if not missing_ok:
+                raise GitConfigUnsetMissingValue(key, scope)
+            return
+
         if value_pattern is None:
             del cfg[key]
             return
 
-        cur = cfg.get(key)
         if cur is None:
             del cfg[key]
             return

testing_support/fake_repos.py

@@ -170,6 +170,22 @@ class FakeReposBase(object):
                 'git', 'init', '-b', DEFAULT_BRANCH, '-q',
                 join(self.git_base, repo)
             ])
+            subprocess2.check_call([
+                'git',
+                '-C',
+                join(self.git_base, repo),
+                'config',
+                'user.name',
+                'Hina Hoshino',
+            ])
+            subprocess2.check_call([
+                'git',
+                '-C',
+                join(self.git_base, repo),
+                'config',
+                'user.email',
+                'testing@example.com',
+            ])
             self.git_hashes[repo] = [(None, None)]
         self.populateGit()
         self.initialized = True

tests/scm_unittest.py

@@ -221,6 +221,27 @@ class RealGitTest(fake_repos.FakeReposTestBase):
         self.assertEqual('default-value',
                          scm.GIT.GetConfig(self.cwd, key, 'default-value'))
 
+        # Test missing_ok
+        key = 'scm.missing-key'
+        with self.assertRaises(scm.GitConfigUnsetMissingValue):
+            scm.GIT.SetConfig(self.cwd, key, None, missing_ok=False)
+        with self.assertRaises(scm.GitConfigUnsetMissingValue):
+            scm.GIT.SetConfig(self.cwd,
+                              key,
+                              None,
+                              modify_all=True,
+                              missing_ok=False)
+        with self.assertRaises(scm.GitConfigUnsetMissingValue):
+            scm.GIT.SetConfig(self.cwd,
+                              key,
+                              None,
+                              value_pattern='some_value',
+                              missing_ok=False)
+
+        scm.GIT.SetConfig(self.cwd, key, None)
+        scm.GIT.SetConfig(self.cwd, key, None, modify_all=True)
+        scm.GIT.SetConfig(self.cwd, key, None, value_pattern='some_value')
+
     def testGetSetConfigBool(self):
         key = 'scm.test-key'
         self.assertFalse(scm.GIT.GetConfigBool(self.cwd, key))