git cl: refactor Changelist codereview detection.

BUG=579160

Review URL: https://codereview.chromium.org/1830923002

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@299464 0039d316-1c4b-4281-b951-d872f2087c98

git_cl.py

@@ -894,27 +894,22 @@ class Changelist(object):
         assert codereview in ('rietveld', 'gerrit')
       return
 
-    # Automatic selection.
+    # Automatic selection based on issue number set for a current branch.
+    # Rietveld takes precedence over Gerrit.
     assert not self.issue
-    # Check if this branch is associated with Rietveld => Rieveld.
-    self._codereview_impl = _RietveldChangelistImpl(self, **kwargs)
-    if self.GetIssue(force_lookup=True):
-      return
-
-    tmp_rietveld = self._codereview_impl  # Save Rietveld object.
-
-    # Check if this branch has Gerrit issue associated => Gerrit.
-    self._codereview_impl = _GerritChangelistImpl(self, **kwargs)
-    if self.GetIssue(force_lookup=True):
-      return
-
-    # OK, no issue is set for this branch.
-    # If Gerrit is set repo-wide => Gerrit.
-    if settings.GetIsGerrit():
-      return
+    # Whether we find issue or not, we are doing the lookup.
+    self.lookedup_issue = True
+    for cls in [_RietveldChangelistImpl, _GerritChangelistImpl]:
+      setting = cls.IssueSetting(self.GetBranch())
+      issue = RunGit(['config', setting], error_ok=True).strip()
+      if issue:
+        self._codereview_impl = cls(self, **kwargs)
+        self.issue = int(issue)
+        return
 
-    self._codereview_impl = tmp_rietveld
-    return
+    # No issue is set for this branch, so decide based on repo-wide settings.
+    return self._load_codereview_impl(
+          codereview='gerrit' if settings.GetIsGerrit() else 'rietveld')
 
 
   def GetCCList(self):
@@ -1131,10 +1126,11 @@ class Changelist(object):
                    cwd=url).strip()
     return url
 
-  def GetIssue(self, force_lookup=False):
+  def GetIssue(self):
     """Returns the issue number as a int or None if not set."""
-    if force_lookup or (self.issue is None and not self.lookedup_issue):
-      issue = RunGit(['config', self._codereview_impl.IssueSetting()],
+    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.lookedup_issue = True
@@ -1180,7 +1176,7 @@ class Changelist(object):
 
   def SetIssue(self, issue=None):
     """Set this branch's issue.  If issue isn't given, clears the issue."""
-    issue_setting = self._codereview_impl.IssueSetting()
+    issue_setting = self._codereview_impl.IssueSetting(self.GetBranch())
     codereview_setting = self._codereview_impl.GetCodereviewServerSetting()
     if issue:
       self.issue = issue
@@ -1312,8 +1308,10 @@ class _ChangelistCodereviewBase(object):
     """Returns git config setting for the codereview server."""
     raise NotImplementedError()
 
-  def IssueSetting(self):
-    """Returns name of git config setting which stores issue number."""
+  @staticmethod
+  def IssueSetting(branch):
+    """Returns name of git config setting which stores issue number for a given
+    branch."""
     raise NotImplementedError()
 
   def PatchsetSetting(self):
@@ -1498,9 +1496,9 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
           self._auth_config or auth.make_auth_config())
     return self._rpc_server
 
-  def IssueSetting(self):
-    """Return the git setting that stores this change's issue."""
-    return 'branch.%s.rietveldissue' % self.GetBranch()
+  @staticmethod
+  def IssueSetting(branch):
+    return 'branch.%s.rietveldissue' % branch
 
   def PatchsetSetting(self):
     """Return the git setting that stores this change's most recent patchset."""
@@ -1550,9 +1548,9 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
         self._gerrit_server = 'https://%s' % self._gerrit_host
     return self._gerrit_server
 
-  def IssueSetting(self):
-    """Return the git setting that stores this change's issue."""
-    return 'branch.%s.gerritissue' % self.GetBranch()
+  @staticmethod
+  def IssueSetting(branch):
+    return 'branch.%s.gerritissue' % branch
 
   def PatchsetSetting(self):
     """Return the git setting that stores this change's most recent patchset."""

tests/git_cl_test.py

@@ -167,13 +167,13 @@ class TestGitCl(TestCase):
       similarity_call,
       ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
       find_copies_call,
-      ((['git', 'config', 'rietveld.autoupdate'],), ''),
-      ((['git', 'config', 'rietveld.server'],),
-       'codereview.example.com'),
       ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
       ((['git', 'config', 'branch.master.rietveldissue'],), ''),
       ((['git', 'config', 'branch.master.gerritissue'],), ''),
+      ((['git', 'config', 'rietveld.autoupdate'],), ''),
       ((['git', 'config', 'gerrit.host'],), ''),
+      ((['git', 'config', 'rietveld.server'],),
+       'codereview.example.com'),
       ((['git', 'config', 'branch.master.merge'],), 'master'),
       ((['git', 'config', 'branch.master.remote'],), 'origin'),
       ((['get_or_create_merge_base', 'master', 'master'],),
@@ -286,11 +286,11 @@ class TestGitCl(TestCase):
       ((['git', 'symbolic-ref', 'HEAD'],), 'refs/heads/working'),
       ((['git', 'config', '--int', '--get',
         'branch.working.git-find-copies'],), ''),
-      ((['git',
-         'config', 'rietveld.server'],), 'codereview.example.com'),
       ((['git', 'symbolic-ref', 'HEAD'],), 'refs/heads/working'),
       ((['git',
          'config', 'branch.working.rietveldissue'],), '12345'),
+      ((['git',
+         'config', 'rietveld.server'],), 'codereview.example.com'),
       ((['git',
          'config', 'branch.working.merge'],), 'refs/heads/master'),
       ((['git', 'config', 'branch.working.remote'],), 'origin'),
@@ -550,18 +550,16 @@ class TestGitCl(TestCase):
         ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
         ((['git', 'config', '--int', '--get',
           'branch.master.git-find-copies'],), ''),
-        ((['git', 'config', 'rietveld.autoupdate'],), ''),
-        ((['git', 'config', 'rietveld.server'],), ''),
-        ((['git', 'config', 'rietveld.server'],), ''),
         ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
         ((['git', 'config', 'branch.master.rietveldissue'],), ''),
         ((['git', 'config', 'branch.master.gerritissue'],), ''),
+        ((['git', 'config', 'rietveld.autoupdate'],), ''),
         ((['git', 'config', 'gerrit.host'],), 'True'),
         ((['git', 'config', 'branch.master.merge'],), 'master'),
         ((['git', 'config', 'branch.master.remote'],), 'origin'),
         ((['get_or_create_merge_base', 'master', 'master'],),
          'fake_ancestor_sha'),
-        ] + cls._git_sanity_checks('fake_ancestor_sha', 'master') + [
+      ] + cls._git_sanity_checks('fake_ancestor_sha', 'master') + [
         ((['git', 'rev-parse', '--show-cdup'],), ''),
         ((['git', 'rev-parse', 'HEAD'],), '12345'),
         ((['git',
@@ -577,7 +575,7 @@ class TestGitCl(TestCase):
            'diff', '--no-ext-diff', '--stat', '--find-copies-harder',
            '-l100000', '-C50', 'fake_ancestor_sha', 'HEAD'],),
          '+dat'),
-        ]
+      ]
 
   @classmethod
   def _gerrit_upload_calls(cls, description, reviewers, squash,