git cl: cleanup branch config get/set/unset.

Previously there was a soup with add-hoc formatting with
current branch name, which wasn't always set (see bug 611020).
This CL makes sure all such operations now:
 * properly use types --int and --bool
 * go through the *only* appropriate get/set/unset function.

Furthermore, tests were a mess wrt to raising exceptions when
git processes terminated with an exception. This CL cleaned up,
though I didn't go through all expectations, so some returns of
empty stdout instead of raising CalledProcess error are likely
remaining.

Disclaimer: this CL is not necessarily fixing the referenced bug
below, but it should at least provide better stacktrace when
the bug manifestst itself.

BUG=611020
R=agable@chromium.org

Review-Url: https://codereview.chromium.org/2259043002

git_cl.py

@@ -114,20 +114,19 @@ def RunGit(args, **kwargs):
 
 def RunGitWithCode(args, suppress_stderr=False):
   """Returns return code and stdout."""
+  if suppress_stderr:
+    stderr = subprocess2.VOID
+  else:
+    stderr = sys.stderr
   try:
-    if suppress_stderr:
-      stderr = subprocess2.VOID
-    else:
-      stderr = sys.stderr
-    out, code = subprocess2.communicate(['git'] + args,
-                                        env=GetNoGitPagerEnv(),
-                                        stdout=subprocess2.PIPE,
-                                        stderr=stderr)
-    return code, out[0]
-  except ValueError:
-    # When the subprocess fails, it returns None.  That triggers a ValueError
-    # when trying to unpack the return value into (out, code).
-    return 1, ''
+    (out, _), code = subprocess2.communicate(['git'] + args,
+                                             env=GetNoGitPagerEnv(),
+                                             stdout=subprocess2.PIPE,
+                                             stderr=stderr)
+    return code, out
+  except subprocess2.CalledProcessError as e:
+    logging.debug('Failed running %s', args)
+    return e.returncode, e.stdout
 
 
 def RunGitSilent(args):
@@ -157,33 +156,72 @@ def ask_for_data(prompt):
     sys.exit(1)
 
 
-def git_set_branch_value(key, value):
-  branch = GetCurrentBranch()
-  if not branch:
-    return
+def _git_branch_config_key(branch, key):
+  """Helper method to return Git config key for a branch."""
+  assert branch, 'branch name is required to set git config for it'
+  return 'branch.%s.%s' % (branch, key)
 
-  cmd = ['config']
-  if isinstance(value, int):
-    cmd.append('--int')
-  git_key = 'branch.%s.%s' % (branch, key)
-  RunGit(cmd + [git_key, str(value)])
 
+def _git_get_branch_config_value(key, default=None, value_type=str,
+                                 branch=False):
+  """Returns git config value of given or current branch if any.
 
-def git_get_branch_default(key, default):
-  branch = GetCurrentBranch()
-  if branch:
-    git_key = 'branch.%s.%s' % (branch, key)
-    (_, stdout) = RunGitWithCode(['config', '--int', '--get', git_key])
-    try:
-      return int(stdout.strip())
-    except ValueError:
-      pass
+  Returns default in all other cases.
+  """
+  assert value_type in (int, str, bool)
+  if branch is False:  # Distinguishing default arg value from None.
+    branch = GetCurrentBranch()
+
+  if not branch:
+    return default
+
+  args = ['config']
+  if value_type == int:
+    args.append('--int')
+  elif value_type == bool:
+    args.append('--bool')
+  args.append(_git_branch_config_key(branch, key))
+  code, out = RunGitWithCode(args)
+  if code == 0:
+    value = out.strip()
+    if value_type == int:
+      return int(value)
+    if value_type == bool:
+      return bool(value.lower() == 'true')
+    return value
   return default
 
 
+def _git_set_branch_config_value(key, value, branch=None, **kwargs):
+  """Sets the value or unsets if it's None of a git branch config.
+
+  Valid, though not necessarily existing, branch must be provided,
+  otherwise currently checked out branch is used.
+  """
+  if not branch:
+    branch = GetCurrentBranch()
+    assert branch, 'a branch name OR currently checked out branch is required'
+  args = ['config']
+  # Check for boolean first, becuase bool is int, but int is not bool.
+  if value is None:
+    args.append('--unset')
+  elif isinstance(value, bool):
+    args.append('--bool')
+    value = str(value).lower()
+  elif isinstance(value, int):
+    args.append('--int')
+    value = str(value)
+  else:
+    value = str(value)
+  args.append(_git_branch_config_key(branch, key))
+  if value is not None:
+    args.append(value)
+  RunGit(args, **kwargs)
+
+
 def add_git_similarity(parser):
   parser.add_option(
-      '--similarity', metavar='SIM', type='int', action='store',
+      '--similarity', metavar='SIM', type=int, action='store',
       help='Sets the percentage that a pair of files need to match in order to'
            ' be considered copies (default 50)')
   parser.add_option(
@@ -198,19 +236,20 @@ def add_git_similarity(parser):
     options, args = old_parser_args(args)
 
     if options.similarity is None:
-      options.similarity = git_get_branch_default('git-cl-similarity', 50)
+      options.similarity = _git_get_branch_config_value(
+          'git-cl-similarity', default=50, value_type=int)
     else:
       print('Note: Saving similarity of %d%% in git config.'
             % options.similarity)
-      git_set_branch_value('git-cl-similarity', options.similarity)
+      _git_set_branch_config_value('git-cl-similarity', options.similarity)
 
     options.similarity = max(0, min(options.similarity, 100))
 
     if options.find_copies is None:
-      options.find_copies = bool(
-          git_get_branch_default('git-find-copies', True))
+      options.find_copies = _git_get_branch_config_value(
+          'git-find-copies', default=True, value_type=bool)
     else:
-      git_set_branch_value('git-find-copies', int(options.find_copies))
+      _git_set_branch_config_value('git-find-copies', bool(options.find_copies))
 
     print('Using %d%% similarity for rename/copy detection. '
           'Override with --similarity.' % options.similarity)
@@ -906,7 +945,7 @@ class Changelist(object):
   Notes:
     * Not safe for concurrent multi-{thread,process} use.
     * Caches values from current branch. Therefore, re-use after branch change
-      with care.
+      with great care.
   """
 
   def __init__(self, branchref=None, issue=None, codereview=None, **kwargs):
@@ -966,14 +1005,15 @@ class Changelist(object):
     assert not self.issue
     # Whether we find issue or not, we are doing the lookup.
     self.lookedup_issue = True
-    for codereview, cls in _CODEREVIEW_IMPLEMENTATIONS.iteritems():
-      setting = cls.IssueSetting(self.GetBranch())
-      issue = RunGit(['config', setting], error_ok=True).strip()
-      if issue:
-        self._codereview = codereview
-        self._codereview_impl = cls(self, **kwargs)
-        self.issue = int(issue)
-        return
+    if self.GetBranch():
+      for codereview, cls in _CODEREVIEW_IMPLEMENTATIONS.iteritems():
+        issue = _git_get_branch_config_value(
+            cls.IssueConfigKey(), value_type=int, branch=self.GetBranch())
+        if issue:
+          self._codereview = codereview
+          self._codereview_impl = cls(self, **kwargs)
+          self.issue = int(issue)
+          return
 
     # No issue is set for this branch, so decide based on repo-wide settings.
     return self._load_codereview_impl(
@@ -1025,16 +1065,31 @@ class Changelist(object):
     """Clears cached branch data of this object."""
     self.branch = self.branchref = None
 
+  def _GitGetBranchConfigValue(self, key, default=None, **kwargs):
+    assert 'branch' not in kwargs, 'this CL branch is used automatically'
+    kwargs['branch'] = self.GetBranch()
+    return _git_get_branch_config_value(key, default, **kwargs)
+
+  def _GitSetBranchConfigValue(self, key, value, **kwargs):
+    assert 'branch' not in kwargs, 'this CL branch is used automatically'
+    assert self.GetBranch(), (
+        'this CL must have an associated branch to %sset %s%s' %
+          ('un' if value is None else '',
+           key,
+           '' if value is None else ' to %r' % value))
+    kwargs['branch'] = self.GetBranch()
+    return _git_set_branch_config_value(key, value, **kwargs)
+
   @staticmethod
   def FetchUpstreamTuple(branch):
     """Returns a tuple containing remote and remote ref,
        e.g. 'origin', 'refs/heads/master'
     """
     remote = '.'
-    upstream_branch = RunGit(['config', 'branch.%s.merge' % branch],
-                             error_ok=True).strip()
+    upstream_branch = _git_get_branch_config_value('merge', branch=branch)
+
     if upstream_branch:
-      remote = RunGit(['config', 'branch.%s.remote' % branch]).strip()
+      remote = _git_get_branch_config_value('remote', branch=branch)
     else:
       upstream_branch = RunGit(['config', 'rietveld.upstream-branch'],
                                error_ok=True).strip()
@@ -1168,8 +1223,7 @@ class Changelist(object):
 
     Returns None if it is not set.
     """
-    return RunGit(['config', 'branch.%s.base-url' % self.GetBranch()],
-                  error_ok=True).strip()
+    return self._GitGetBranchConfigValue('base-url')
 
   def GetGitSvnRemoteUrl(self):
     """Return the configured git-svn remote URL parsed from git svn info.
@@ -1202,10 +1256,8 @@ class Changelist(object):
   def GetIssue(self):
     """Returns the issue number as a int or None if not set."""
     if self.issue is None and not self.lookedup_issue:
-      issue = RunGit(['config',
-                      self._codereview_impl.IssueSetting(self.GetBranch())],
-                     error_ok=True).strip()
-      self.issue = int(issue) or None if issue else None
+      self.issue = self._GitGetBranchConfigValue(
+          self._codereview_impl.IssueConfigKey(), value_type=int)
       self.lookedup_issue = True
     return self.issue
 
@@ -1230,41 +1282,44 @@ class Changelist(object):
   def GetPatchset(self):
     """Returns the patchset number as a int or None if not set."""
     if self.patchset is None and not self.lookedup_patchset:
-      patchset = RunGit(['config', self._codereview_impl.PatchsetSetting()],
-                        error_ok=True).strip()
-      self.patchset = int(patchset) or None if patchset else None
+      self.patchset = self._GitGetBranchConfigValue(
+          self._codereview_impl.PatchsetConfigKey(), value_type=int)
       self.lookedup_patchset = True
     return self.patchset
 
   def SetPatchset(self, patchset):
-    """Set this branch's patchset.  If patchset=0, clears the patchset."""
-    patchset_setting = self._codereview_impl.PatchsetSetting()
-    if patchset:
-      RunGit(['config', patchset_setting, str(patchset)])
-      self.patchset = patchset
-    else:
-      RunGit(['config', '--unset', patchset_setting],
-             stderr=subprocess2.PIPE, error_ok=True)
+    """Set this branch's patchset. If patchset=0, clears the patchset."""
+    assert self.GetBranch()
+    if not patchset:
       self.patchset = None
+    else:
+      self.patchset = int(patchset)
+    self._GitSetBranchConfigValue(
+        self._codereview_impl.PatchsetConfigKey(), self.patchset)
 
   def SetIssue(self, issue=None):
-    """Set this branch's issue.  If issue isn't given, clears the issue."""
-    issue_setting = self._codereview_impl.IssueSetting(self.GetBranch())
-    codereview_setting = self._codereview_impl.GetCodereviewServerSetting()
+    """Set this branch's issue. If issue isn't given, clears the issue."""
+    assert self.GetBranch()
     if issue:
+      issue = int(issue)
+      self._GitSetBranchConfigValue(
+          self._codereview_impl.IssueConfigKey(), issue)
       self.issue = issue
-      RunGit(['config', issue_setting, str(issue)])
       codereview_server = self._codereview_impl.GetCodereviewServer()
       if codereview_server:
-        RunGit(['config', codereview_setting, codereview_server])
+        self._GitSetBranchConfigValue(
+            self._codereview_impl.CodereviewServerConfigKey(),
+            codereview_server)
     else:
-      # Reset it regardless. It doesn't hurt.
-      config_settings = [issue_setting, self._codereview_impl.PatchsetSetting()]
-      for prop in (['last-upload-hash'] +
-                    self._codereview_impl._PostUnsetIssueProperties()):
-        config_settings.append('branch.%s.%s' % (self.GetBranch(), prop))
-      for setting in config_settings:
-        RunGit(['config', '--unset', setting], error_ok=True)
+      # Reset all of these just to be clean.
+      reset_suffixes = [
+          'last-upload-hash',
+          self._codereview_impl.IssueConfigKey(),
+          self._codereview_impl.PatchsetConfigKey(),
+          self._codereview_impl.CodereviewServerConfigKey(),
+      ] + self._PostUnsetIssueProperties()
+      for prop in reset_suffixes:
+        self._GitSetBranchConfigValue(prop, None, error_ok=True)
       self.issue = None
       self.patchset = None
 
@@ -1408,8 +1463,8 @@ class Changelist(object):
       elif options.cq_dry_run:
         self.SetCQState(_CQState.DRY_RUN)
 
-      git_set_branch_value('last-upload-hash',
-                           RunGit(['rev-parse', 'HEAD']).strip())
+      _git_set_branch_config_value('last-upload-hash',
+                                   RunGit(['rev-parse', 'HEAD']).strip())
       # Run post upload hooks, if specified.
       if settings.GetRunPostUploadHook():
         presubmit_support.DoPostUploadExecuter(
@@ -1496,27 +1551,24 @@ class _ChangelistCodereviewBase(object):
     """Fetches and returns description from the codereview server."""
     raise NotImplementedError()
 
-  def GetCodereviewServerSetting(self):
-    """Returns git config setting for the codereview server."""
-    raise NotImplementedError()
-
   @classmethod
-  def IssueSetting(cls, branch):
-    return 'branch.%s.%s' % (branch, cls.IssueSettingSuffix())
+  def IssueConfigKey(cls):
+    """Returns branch setting storing issue number."""
+    raise NotImplementedError()
 
   @classmethod
-  def IssueSettingSuffix(cls):
-    """Returns name of git config setting which stores issue number for a given
-    branch."""
+  def PatchsetConfigKey(cls):
+    """Returns branch setting storing patchset number."""
     raise NotImplementedError()
 
-  def PatchsetSetting(self):
-    """Returns name of git config setting which stores issue number."""
+  @classmethod
+  def CodereviewServerConfigKey(cls):
+    """Returns branch setting storing codereview server."""
     raise NotImplementedError()
 
   def _PostUnsetIssueProperties(self):
     """Which branch-specific properties to erase when unsettin issue."""
-    raise NotImplementedError()
+    return []
 
   def GetRieveldObjForPresubmit(self):
     # This is an unfortunate Rietveld-embeddedness in presubmit.
@@ -1603,10 +1655,8 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
       # If we're on a branch then get the server potentially associated
       # with that branch.
       if self.GetIssue():
-        rietveld_server_setting = self.GetCodereviewServerSetting()
-        if rietveld_server_setting:
-          self._rietveld_server = gclient_utils.UpgradeToHttps(RunGit(
-              ['config', rietveld_server_setting], error_ok=True).strip())
+        self._rietveld_server = gclient_utils.UpgradeToHttps(
+            self._GitGetBranchConfigValue(self.CodereviewServerConfigKey()))
       if not self._rietveld_server:
         self._rietveld_server = settings.GetDefaultServerUrl()
     return self._rietveld_server
@@ -1760,23 +1810,16 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
     return self._rpc_server
 
   @classmethod
-  def IssueSettingSuffix(cls):
+  def IssueConfigKey(cls):
     return 'rietveldissue'
 
-  def PatchsetSetting(self):
-    """Return the git setting that stores this change's most recent patchset."""
-    return 'branch.%s.rietveldpatchset' % self.GetBranch()
-
-  def GetCodereviewServerSetting(self):
-    """Returns the git setting that stores this change's rietveld server."""
-    branch = self.GetBranch()
-    if branch:
-      return 'branch.%s.rietveldserver' % branch
-    return None
+  @classmethod
+  def PatchsetConfigKey(cls):
+    return 'rietveldpatchset'
 
-  def _PostUnsetIssueProperties(self):
-    """Which branch-specific properties to erase when unsetting issue."""
-    return ['rietveldserver']
+  @classmethod
+  def CodereviewServerConfigKey(cls):
+    return 'rietveldserver'
 
   def GetRieveldObjForPresubmit(self):
     return self.RpcServer()
@@ -2070,12 +2113,10 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
       # If we're on a branch then get the server potentially associated
       # with that branch.
       if self.GetIssue():
-        gerrit_server_setting = self.GetCodereviewServerSetting()
-        if gerrit_server_setting:
-          self._gerrit_server = RunGit(['config', gerrit_server_setting],
-                                       error_ok=True).strip()
-          if self._gerrit_server:
-            self._gerrit_host = urlparse.urlparse(self._gerrit_server).netloc
+        self._gerrit_server = self._GitGetBranchConfigValue(
+            self.CodereviewServerConfigKey())
+        if self._gerrit_server:
+          self._gerrit_host = urlparse.urlparse(self._gerrit_server).netloc
       if not self._gerrit_server:
         # We assume repo to be hosted on Gerrit, and hence Gerrit server
         # has "-review" suffix for lowest level subdomain.
@@ -2086,9 +2127,17 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
     return self._gerrit_server
 
   @classmethod
-  def IssueSettingSuffix(cls):
+  def IssueConfigKey(cls):
     return 'gerritissue'
 
+  @classmethod
+  def PatchsetConfigKey(cls):
+    return 'gerritpatchset'
+
+  @classmethod
+  def CodereviewServerConfigKey(cls):
+    return 'gerritserver'
+
   def EnsureAuthenticated(self, force):
     """Best effort check that user is authenticated with Gerrit server."""
     if settings.GetGerritSkipEnsureAuthenticated():
@@ -2134,24 +2183,9 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
                      cookie_auth.get_netrc_path(),
                      cookie_auth.get_new_password_message(git_host)))
 
-
-  def PatchsetSetting(self):
-    """Return the git setting that stores this change's most recent patchset."""
-    return 'branch.%s.gerritpatchset' % self.GetBranch()
-
-  def GetCodereviewServerSetting(self):
-    """Returns the git setting that stores this change's Gerrit server."""
-    branch = self.GetBranch()
-    if branch:
-      return 'branch.%s.gerritserver' % branch
-    return None
-
   def _PostUnsetIssueProperties(self):
     """Which branch-specific properties to erase when unsetting issue."""
-    return [
-        'gerritserver',
-        'gerritsquashhash',
-    ]
+    return ['gerritsquashhash']
 
   def GetRieveldObjForPresubmit(self):
     class ThisIsNotRietveldIssue(object):
@@ -2274,8 +2308,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
                      'Press Enter to continue, Ctrl+C to abort.')
 
     differs = True
-    last_upload = RunGit(['config',
-                          'branch.%s.gerritsquashhash' % self.GetBranch()],
+    last_upload = RunGit(['config', self._GitBranchSetting('gerritsquashhash')],
                          error_ok=True).strip()
     # Note: git diff outputs nothing if there is no diff.
     if not last_upload or RunGit(['diff', last_upload]).strip():
@@ -2578,8 +2611,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
           ('Created|Updated %d issues on Gerrit, but only 1 expected.\n'
            'Change-Id: %s') % (len(change_numbers), change_id))
       self.SetIssue(change_numbers[0])
-      RunGit(['config', 'branch.%s.gerritsquashhash' % self.GetBranch(),
-              ref_to_push])
+      self._GitSetBranchConfigValue('gerritsquashhash', ref_to_push)
     return 0
 
   def _AddChangeIdToCommitMessage(self, options, args):
@@ -5079,7 +5111,7 @@ def CMDcheckout(parser, args):
 
   branches = []
   for cls in _CODEREVIEW_IMPLEMENTATIONS.values():
-    branches.extend(find_issues(cls.IssueSettingSuffix()))
+    branches.extend(find_issues(cls.IssueConfigKey()))
   if len(branches) == 0:
     print('No branch found for issue %s.' % target_issue)
     return 1

tests/git_cl_test.py

@@ -21,6 +21,13 @@ import git_common
 import git_footers
 import subprocess2
 
+def callError(code=1, cmd='', cwd='', stdout='', stderr=''):
+  return subprocess2.CalledProcessError(code, cmd, cwd, stdout, stderr)
+
+
+CERR1 = callError(1)
+
+
 class ChangelistMock(object):
   # A class variable so we can access it when we don't have access to the
   # instance that's being set.
@@ -34,6 +41,7 @@ class ChangelistMock(object):
   def UpdateDescription(self, desc):
     ChangelistMock.desc = desc
 
+
 class PresubmitMock(object):
   def __init__(self, *args, **kwargs):
     self.reviewers = []
@@ -233,7 +241,8 @@ class TestGitCl(TestCase):
     self.mock(subprocess2, 'call', self._mocked_call)
     self.mock(subprocess2, 'check_call', self._mocked_call)
     self.mock(subprocess2, 'check_output', self._mocked_call)
-    self.mock(subprocess2, 'communicate', self._mocked_call)
+    self.mock(subprocess2, 'communicate',
+              lambda *a, **kw: ([self._mocked_call(*a, **kw), ''], 0))
     self.mock(git_cl.gclient_utils, 'CheckCallAndFilter', self._mocked_call)
     self.mock(git_common, 'is_dirty_git_tree', lambda x: False)
     self.mock(git_common, 'get_or_create_merge_base',
@@ -269,8 +278,6 @@ class TestGitCl(TestCase):
         self.calls,
         '@%d  Expected: <Missing>   Actual: %r' % (len(self._calls_done), args))
     top = self.calls.pop(0)
-    if len(top) > 2 and top[2]:
-      raise top[2]
     expected_args, result = top
 
     # Also logs otherwise it could get caught in a try/finally and be hard to
@@ -298,6 +305,8 @@ class TestGitCl(TestCase):
           len(self._calls_done), expected_args, args))
 
     self._calls_done.append(top)
+    if isinstance(result, Exception):
+      raise result
     return result
 
   @classmethod
@@ -319,19 +328,19 @@ class TestGitCl(TestCase):
   def _git_base_calls(cls, similarity, find_copies):
     if similarity is None:
       similarity = '50'
-      similarity_call = ((['git', 'config', '--int', '--get',
-                         'branch.master.git-cl-similarity'],), '')
+      similarity_call = ((['git', 'config', '--int',
+                         'branch.master.git-cl-similarity'],), CERR1)
     else:
       similarity_call = ((['git', 'config', '--int',
-                         'branch.master.git-cl-similarity', similarity],), '')
+                           'branch.master.git-cl-similarity', similarity],), '')
 
     if find_copies is None:
       find_copies = True
-      find_copies_call = ((['git', 'config', '--int', '--get',
-                          'branch.master.git-find-copies'],), '')
+      find_copies_call = ((['git', 'config', '--bool',
+                          'branch.master.git-find-copies'],), CERR1)
     else:
-      val = str(int(find_copies))
-      find_copies_call = ((['git', 'config', '--int',
+      val = str(find_copies).lower()
+      find_copies_call = ((['git', 'config', '--bool',
                           'branch.master.git-find-copies', val],), '')
 
     if find_copies:
@@ -349,8 +358,8 @@ class TestGitCl(TestCase):
       find_copies_call,
     ] + cls._is_gerrit_calls() + [
       ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
-      ((['git', 'config', 'branch.master.rietveldissue'],), ''),
-      ((['git', 'config', 'branch.master.gerritissue'],), ''),
+      ((['git', 'config', '--int', 'branch.master.rietveldissue'],), CERR1),
+      ((['git', 'config', '--int', 'branch.master.gerritissue'],), CERR1),
       ((['git', 'config', 'rietveld.server'],),
        'codereview.example.com'),
       ((['git', 'config', 'branch.master.merge'],), 'master'),
@@ -363,8 +372,7 @@ class TestGitCl(TestCase):
       ((['git', 'diff', '--name-status', '--no-renames', '-r',
          'fake_ancestor_sha...', '.'],),
         'M\t.gitignore\n'),
-      ((['git', 'config', 'branch.master.rietveldpatchset'],),
-       ''),
+      ((['git', 'config', '--int', 'branch.master.rietveldpatchset'],), CERR1),
       ((['git', 'log', '--pretty=format:%s%n%n%b',
          'fake_ancestor_sha...'],),
        'foo'),
@@ -403,12 +411,11 @@ class TestGitCl(TestCase):
         ((['git', 'rev-parse', '--show-cdup'],), ''),
         ((['git', 'svn', 'info'],), ''),
         ((['git', 'config', 'rietveld.project'],), ''),
-        ((['git',
-           'config', 'branch.master.rietveldissue', '1'],), ''),
+        ((['git', 'config', '--int', 'branch.master.rietveldissue', '1'],), ''),
         ((['git', 'config', 'branch.master.rietveldserver',
            'https://codereview.example.com'],), ''),
         ((['git',
-           'config', 'branch.master.rietveldpatchset', '2'],), ''),
+           'config', '--int', 'branch.master.rietveldpatchset', '2'],), ''),
     ] + cls._git_post_upload_calls()
 
   @classmethod
@@ -434,7 +441,7 @@ class TestGitCl(TestCase):
          'rev-list', '^' + fake_ancestor, 'HEAD'],), fake_cl),
       # Mock a config miss (error code 1)
       ((['git',
-         'config', 'gitcl.remotebranch'],), (('', None), 1)),
+         'config', 'gitcl.remotebranch'],), CERR1),
     ] + ([
       # Call to GetRemoteBranch()
       ((['git',
@@ -461,14 +468,14 @@ class TestGitCl(TestCase):
          None),
         0)),
       ((['git', 'symbolic-ref', 'HEAD'],), 'refs/heads/working'),
-      ((['git', 'config', '--int', '--get',
-        'branch.working.git-cl-similarity'],), ''),
+      ((['git', 'config', '--int',
+        'branch.working.git-cl-similarity'],), CERR1),
       ((['git', 'symbolic-ref', 'HEAD'],), 'refs/heads/working'),
-      ((['git', 'config', '--int', '--get',
-        'branch.working.git-find-copies'],), ''),
+      ((['git', 'config', '--bool',
+        'branch.working.git-find-copies'],), CERR1),
       ((['git', 'symbolic-ref', 'HEAD'],), 'refs/heads/working'),
       ((['git',
-         'config', 'branch.working.rietveldissue'],), '12345'),
+         'config', '--int', 'branch.working.rietveldissue'],), '12345'),
       ((['git',
          'config', 'rietveld.server'],), 'codereview.example.com'),
       ((['git',
@@ -505,7 +512,7 @@ class TestGitCl(TestCase):
          '.'],),
         'M\tPRESUBMIT.py'),
       ((['git',
-         'config', 'branch.working.rietveldpatchset'],), '31137'),
+         'config', '--int', 'branch.working.rietveldpatchset'],), '31137'),
       ((['git', 'config', 'branch.working.rietveldserver'],),
          'codereview.example.com'),
       ((['git', 'config', 'user.email'],), 'author@example.com'),
@@ -529,11 +536,10 @@ class TestGitCl(TestCase):
        (' PRESUBMIT.py |    2 +-\n'
         ' 1 files changed, 1 insertions(+), 1 deletions(-)\n')),
       ((['git', 'show-ref', '--quiet', '--verify',
-         'refs/heads/git-cl-commit'],),
-       (('', None), 0)),
+         'refs/heads/git-cl-commit'],), ''),
       ((['git', 'branch', '-D', 'git-cl-commit'],), ''),
       ((['git', 'show-ref', '--quiet', '--verify',
-         'refs/heads/git-cl-cherry-pick'],), ''),
+         'refs/heads/git-cl-cherry-pick'],), CERR1),
       ((['git', 'rev-parse', '--show-cdup'],), '\n'),
       ((['git', 'checkout', '-q', '-b', 'git-cl-commit'],), ''),
       ((['git', 'reset', '--soft', 'fake_ancestor_sha'],), ''),
@@ -738,7 +744,7 @@ class TestGitCl(TestCase):
     if skip_auth_check:
       return [((cmd, ), 'true')]
 
-    calls = [((cmd, ), '', subprocess2.CalledProcessError(1, '', '', '', ''))]
+    calls = [((cmd, ), CERR1)]
     if issue:
       calls.extend([
           ((['git', 'config', 'branch.master.gerritserver'],), ''),
@@ -757,16 +763,16 @@ class TestGitCl(TestCase):
   def _gerrit_base_calls(cls, issue=None):
     return [
         ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
-        ((['git', 'config', '--int', '--get',
-          'branch.master.git-cl-similarity'],), ''),
+        ((['git', 'config', '--int', 'branch.master.git-cl-similarity'],),
+         CERR1),
         ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
-        ((['git', 'config', '--int', '--get',
-          'branch.master.git-find-copies'],), ''),
+        ((['git', 'config', '--bool', 'branch.master.git-find-copies'],),
+         CERR1),
       ] + cls._is_gerrit_calls(True) + [
         ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
-        ((['git', 'config', 'branch.master.rietveldissue'],), ''),
-        ((['git', 'config', 'branch.master.gerritissue'],),
-          '' if issue is None else str(issue)),
+        ((['git', 'config', '--int', 'branch.master.rietveldissue'],), CERR1),
+        ((['git', 'config', '--int', 'branch.master.gerritissue'],),
+          CERR1 if issue is None else str(issue)),
         ((['git', 'config', 'branch.master.merge'],), 'refs/heads/master'),
         ((['git', 'config', 'branch.master.remote'],), 'origin'),
         ((['get_or_create_merge_base', 'master',
@@ -783,7 +789,7 @@ class TestGitCl(TestCase):
            'diff', '--name-status', '--no-renames', '-r',
            'fake_ancestor_sha...', '.'],),
          'M\t.gitignore\n'),
-        ((['git', 'config', 'branch.master.gerritpatchset'],), ''),
+        ((['git', 'config', '--int', 'branch.master.gerritpatchset'],), CERR1),
       ] + ([] if issue else [
         ((['git',
            'log', '--pretty=format:%s%n%n%b', 'fake_ancestor_sha...'],),
@@ -902,9 +908,10 @@ class TestGitCl(TestCase):
         ]
     if squash:
       calls += [
-          ((['git', 'config', 'branch.master.gerritissue', '123456'],), ''),
+          ((['git', 'config', '--int', 'branch.master.gerritissue', '123456'],),
+           ''),
           ((['git', 'config', 'branch.master.gerritserver',
-          'https://chromium-review.googlesource.com'],), ''),
+             'https://chromium-review.googlesource.com'],), ''),
           ((['git', 'config', 'branch.master.gerritsquashhash',
              'abcdef0123456789'],), ''),
       ]
@@ -1265,9 +1272,9 @@ class TestGitCl(TestCase):
       # These calls detect codereview to use.
       self.calls += [
         ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
-        ((['git', 'config', 'branch.master.rietveldissue'],), ''),
-        ((['git', 'config', 'branch.master.gerritissue'],), ''),
-        ((['git', 'config', 'rietveld.autoupdate'],), ''),
+        ((['git', 'config', '--int', 'branch.master.rietveldissue'],), CERR1),
+        ((['git', 'config', '--int', 'branch.master.gerritissue'],), CERR1),
+        ((['git', 'config', 'rietveld.autoupdate'],), CERR1),
       ]
 
     if is_gerrit:
@@ -1277,7 +1284,7 @@ class TestGitCl(TestCase):
         ]
     else:
       self.calls += [
-        ((['git', 'config', 'gerrit.host'],), ''),
+        ((['git', 'config', 'gerrit.host'],), CERR1),
         ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'),
         ((['git', 'rev-parse', '--show-cdup'],), ''),
         ((['sed', '-e', 's|^--- a/|--- |; s|^+++ b/|+++ |'],), ''),
@@ -1291,11 +1298,13 @@ class TestGitCl(TestCase):
          'Description\n\n' +
          'patch from issue 123456 at patchset 60001 ' +
          '(http://crrev.com/123456#ps60001)'],), ''),
-      ((['git', 'config', 'branch.master.rietveldissue', '123456'],), ''),
-      ((['git', 'config', 'branch.master.rietveldserver'],), ''),
+      ((['git', 'config', '--int', 'branch.master.rietveldissue', '123456'],),
+       ''),
+      ((['git', 'config', 'branch.master.rietveldserver'],), CERR1),
       ((['git', 'config', 'branch.master.rietveldserver',
          'https://codereview.example.com'],), ''),
-      ((['git', 'config', 'branch.master.rietveldpatchset', '60001'],), ''),
+      ((['git', 'config', '--int', 'branch.master.rietveldpatchset', '60001'],),
+       ''),
     ]
 
   def test_patch_successful(self):
@@ -1310,8 +1319,7 @@ class TestGitCl(TestCase):
   def test_patch_conflict(self):
     self._patch_common()
     self.calls += [
-      ((['git', 'apply', '--index', '-p0', '--3way'],), '',
-       subprocess2.CalledProcessError(1, '', '', '', '')),
+      ((['git', 'apply', '--index', '-p0', '--3way'],), CERR1),
     ]
     self.assertNotEqual(git_cl.main(['patch', '123456']), 0)
 
@@ -1321,7 +1329,8 @@ class TestGitCl(TestCase):
       ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
          'refs/changes/56/123456/7'],), ''),
       ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
-      ((['git', 'config', 'branch.master.gerritissue', '123456'],), ''),
+      ((['git', 'config', '--int', 'branch.master.gerritissue', '123456'],),
+       ''),
       ((['git', 'config', 'branch.master.gerritserver'],), ''),
       ((['git', 'config', 'branch.master.merge'],), 'master'),
       ((['git', 'config', 'branch.master.remote'],), 'origin'),
@@ -1329,7 +1338,7 @@ class TestGitCl(TestCase):
        'https://chromium.googlesource.com/my/repo'),
       ((['git', 'config', 'branch.master.gerritserver',
         'https://chromium-review.googlesource.com'],), ''),
-      ((['git', 'config', 'branch.master.gerritpatchset', '7'],), ''),
+      ((['git', 'config', '--int', 'branch.master.gerritpatchset', '7'],), ''),
     ]
     self.assertEqual(git_cl.main(['patch', '123456']), 0)
 
@@ -1340,7 +1349,8 @@ class TestGitCl(TestCase):
          'refs/changes/56/123456/7'],), ''),
       ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
       ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
-      ((['git', 'config', 'branch.master.gerritissue', '123456'],), ''),
+      ((['git', 'config', '--int', 'branch.master.gerritissue', '123456'],),
+       ''),
       ((['git', 'config', 'branch.master.gerritserver'],), ''),
       ((['git', 'config', 'branch.master.merge'],), 'master'),
       ((['git', 'config', 'branch.master.remote'],), 'origin'),
@@ -1348,7 +1358,7 @@ class TestGitCl(TestCase):
        'https://chromium.googlesource.com/my/repo'),
       ((['git', 'config', 'branch.master.gerritserver',
         'https://chromium-review.googlesource.com'],), ''),
-      ((['git', 'config', 'branch.master.gerritpatchset', '7'],), ''),
+      ((['git', 'config', '--int', 'branch.master.gerritpatchset', '7'],), ''),
     ]
     self.assertEqual(git_cl.main(['patch', '--gerrit', '123456']), 0)
 
@@ -1358,10 +1368,11 @@ class TestGitCl(TestCase):
       ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
          'refs/changes/56/123456/1'],), ''),
       ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
-      ((['git', 'config', 'branch.master.gerritissue', '123456'],), ''),
+      ((['git', 'config', '--int', 'branch.master.gerritissue', '123456'],),
+       ''),
       ((['git', 'config', 'branch.master.gerritserver',
         'https://chromium-review.googlesource.com'],), ''),
-      ((['git', 'config', 'branch.master.gerritpatchset', '1'],), ''),
+      ((['git', 'config', '--int', 'branch.master.gerritpatchset', '1'],), ''),
     ]
     self.assertEqual(git_cl.main(
       ['patch', 'https://chromium-review.googlesource.com/#/c/123456/1']), 0)
@@ -1375,10 +1386,9 @@ class TestGitCl(TestCase):
     self.calls += [
       ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
          'refs/changes/56/123456/1'],), ''),
-      ((['git', 'cherry-pick', 'FETCH_HEAD'],),
-        '', subprocess2.CalledProcessError(1, '', '', '', '')),
-      ((['DieWithError', 'git cherry-pick FETCH_HEAD" failed.\n'],),
-        '', SystemExitMock()),
+      ((['git', 'cherry-pick', 'FETCH_HEAD'],), CERR1),
+      ((['DieWithError', 'Command "git cherry-pick FETCH_HEAD" failed.\n'],),
+       SystemExitMock()),
     ]
     with self.assertRaises(SystemExitMock):
       git_cl.main(['patch',
@@ -1419,12 +1429,9 @@ class TestGitCl(TestCase):
     self.mock(git_cl.sys, 'stdout', StringIO.StringIO())
     self.calls = [
         ((['git', 'config', '--local', '--get-regexp',
-           'branch\\..*\\.rietveldissue'], ), '',
-         subprocess2.CalledProcessError(1, '', '', '', '')),
+           'branch\\..*\\.rietveldissue'], ), CERR1),
         ((['git', 'config', '--local', '--get-regexp',
-           'branch\\..*\\.gerritissue'], ), '',
-         subprocess2.CalledProcessError(1, '', '', '', '')),
-
+           'branch\\..*\\.gerritissue'], ), CERR1),
     ]
     self.assertEqual(1, git_cl.main(['checkout', '99999']))
 
@@ -1484,7 +1491,7 @@ class TestGitCl(TestCase):
               lambda _, v: self._mocked_call(['SetFlags', v]))
     self.calls = [
         ((['git', 'symbolic-ref', 'HEAD'],), 'feature'),
-        ((['git', 'config', 'branch.feature.rietveldissue'],), '123'),
+        ((['git', 'config', '--int', 'branch.feature.rietveldissue'],), '123'),
         ((['git', 'config', 'rietveld.autoupdate'],), ''),
         ((['git', 'config', 'rietveld.server'],), ''),
         ((['git', 'config', 'rietveld.server'],), ''),
@@ -1500,8 +1507,8 @@ class TestGitCl(TestCase):
                   ['SetReview', h, i, labels]))
     self.calls = [
         ((['git', 'symbolic-ref', 'HEAD'],), 'feature'),
-        ((['git', 'config', 'branch.feature.rietveldissue'],), ''),
-        ((['git', 'config', 'branch.feature.gerritissue'],), '123'),
+        ((['git', 'config', '--int', 'branch.feature.rietveldissue'],), CERR1),
+        ((['git', 'config', '--int', 'branch.feature.gerritissue'],), '123'),
         ((['git', 'config', 'branch.feature.gerritserver'],),
          'https://chromium-review.googlesource.com'),
         ((['SetReview', 'chromium-review.googlesource.com', 123,
@@ -1656,9 +1663,9 @@ class TestGitCl(TestCase):
 
     self.calls = [
         ((['git', 'symbolic-ref', 'HEAD'],), 'feature'),
-        ((['git', 'config', 'branch.feature.gerritissue'],), '123'),
-        ((['git', 'config', 'rietveld.autoupdate'],), ''),
-        ((['git', 'config', 'rietveld.bug-prefix'],), ''),
+        ((['git', 'config', '--int', 'branch.feature.gerritissue'],), '123'),
+        ((['git', 'config', 'rietveld.autoupdate'],), CERR1),
+        ((['git', 'config', 'rietveld.bug-prefix'],), CERR1),
         ((['git', 'config', 'core.editor'],), 'vi'),
     ]
     self.assertEqual(0, git_cl.main(['description', '--gerrit']))
@@ -1679,15 +1686,12 @@ class TestGitCl(TestCase):
     self.calls = \
         [((['git', 'for-each-ref', '--format=%(refname)', 'refs/heads'],),
           'refs/heads/master\nrefs/heads/foo\nrefs/heads/bar'),
-         ((['git', 'config', 'branch.master.rietveldissue'],), '1'),
-         ((['git', 'config', 'rietveld.autoupdate'],), ''),
-         ((['git', 'config', 'rietveld.server'],), ''),
-         ((['git', 'config', 'rietveld.server'],), ''),
-         ((['git', 'config', 'branch.foo.rietveldissue'],), '456'),
-         ((['git', 'config', 'rietveld.server'],), ''),
-         ((['git', 'config', 'rietveld.server'],), ''),
-         ((['git', 'config', 'branch.bar.rietveldissue'],), ''),
-         ((['git', 'config', 'branch.bar.gerritissue'],), '789'),
+         ((['git', 'config', '--int', 'branch.master.rietveldissue'],), '1'),
+         ((['git', 'config', 'rietveld.autoupdate'],), CERR1),
+         ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'),
+         ((['git', 'config', '--int', 'branch.foo.rietveldissue'],), '456'),
+         ((['git', 'config', '--int', 'branch.bar.rietveldissue'],), CERR1),
+         ((['git', 'config', '--int', 'branch.bar.gerritissue'],), '789'),
          ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
          ((['git', 'tag', 'git-cl-archived-456-foo', 'foo'],), ''),
          ((['git', 'branch', '-D', 'foo'],), '')]
@@ -1714,10 +1718,9 @@ class TestGitCl(TestCase):
     self.calls = \
         [((['git', 'for-each-ref', '--format=%(refname)', 'refs/heads'],),
           'refs/heads/master'),
-         ((['git', 'config', 'branch.master.rietveldissue'],), '1'),
-         ((['git', 'config', 'rietveld.autoupdate'],), ''),
-         ((['git', 'config', 'rietveld.server'],), ''),
-         ((['git', 'config', 'rietveld.server'],), ''),
+         ((['git', 'config', '--int', 'branch.master.rietveldissue'],), '1'),
+         ((['git', 'config', 'rietveld.autoupdate'],), CERR1),
+         ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'),
          ((['git', 'symbolic-ref', 'HEAD'],), 'master')]
 
     class MockChangelist():
@@ -1740,13 +1743,13 @@ class TestGitCl(TestCase):
     self.mock(git_cl.sys, 'stdout', out)
     self.calls = [
         ((['git', 'symbolic-ref', 'HEAD'],), 'feature'),
-        ((['git', 'config', 'branch.feature.rietveldissue'],), ''),
-        ((['git', 'config', 'branch.feature.gerritissue'],), '123'),
-        ((['git', 'config', '--unset', 'branch.feature.gerritissue'],), ''),
-        ((['git', 'config', '--unset', 'branch.feature.gerritpatchset'],), ''),
+        ((['git', 'config', '--int', 'branch.feature.rietveldissue'],), CERR1),
+        ((['git', 'config', '--int', 'branch.feature.gerritissue'],), '123'),
         # Let this command raise exception (retcode=1) - it should be ignored.
         ((['git', 'config', '--unset', 'branch.feature.last-upload-hash'],),
-         '', subprocess2.CalledProcessError(1, '', '', '', '')),
+         CERR1),
+        ((['git', 'config', '--unset', 'branch.feature.gerritissue'],), ''),
+        ((['git', 'config', '--unset', 'branch.feature.gerritpatchset'],), ''),
         ((['git', 'config', '--unset', 'branch.feature.gerritserver'],), ''),
         ((['git', 'config', '--unset', 'branch.feature.gerritsquashhash'],),
          ''),
@@ -1767,7 +1770,7 @@ class TestGitCl(TestCase):
               lambda _, s: self._mocked_call(['SetCQState', s]))
     self.calls = [
         ((['git', 'symbolic-ref', 'HEAD'],), 'feature'),
-        ((['git', 'config', 'branch.feature.rietveldissue'],), '123'),
+        ((['git', 'config', '--int', 'branch.feature.rietveldissue'],), '123'),
         ((['git', 'config', 'rietveld.autoupdate'],), ''),
         ((['git', 'config', 'rietveld.server'],),
          'https://codereview.chromium.org'),