Revert of git cl: refactor Changelist codereview detection. (patchset #4...

Revert of git cl: refactor Changelist codereview detection. (patchset #4 id:60001 of https://codereview.chromium.org/1830923002/ )

Reason for revert:
broke presubmit

Original issue's description:
> git cl: refactor Changelist codereview detection.
> 
> BUG=579160
> 
> Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299464

TBR=sergiyb@chromium.org,machenbach@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=579160,597638

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

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

git_cl.py

@@ -894,22 +894,27 @@ class Changelist(object):
         assert codereview in ('rietveld', 'gerrit')
       return
 
-    # Automatic selection based on issue number set for a current branch.
-    # Rietveld takes precedence over Gerrit.
+    # Automatic selection.
     assert not self.issue
-    # 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
+    # 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
 
-    # 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')
+    self._codereview_impl = tmp_rietveld
+    return
 
 
   def GetCCList(self):
@@ -1126,11 +1131,10 @@ class Changelist(object):
                    cwd=url).strip()
     return url
 
-  def GetIssue(self):
+  def GetIssue(self, force_lookup=False):
     """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())],
+    if force_lookup or (self.issue is None and not self.lookedup_issue):
+      issue = RunGit(['config', self._codereview_impl.IssueSetting()],
                      error_ok=True).strip()
       self.issue = int(issue) or None if issue else None
       self.lookedup_issue = True
@@ -1176,7 +1180,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(self.GetBranch())
+    issue_setting = self._codereview_impl.IssueSetting()
     codereview_setting = self._codereview_impl.GetCodereviewServerSetting()
     if issue:
       self.issue = issue
@@ -1308,10 +1312,8 @@ class _ChangelistCodereviewBase(object):
     """Returns git config setting for the codereview server."""
     raise NotImplementedError()
 
-  @staticmethod
-  def IssueSetting(branch):
-    """Returns name of git config setting which stores issue number for a given
-    branch."""
+  def IssueSetting(self):
+    """Returns name of git config setting which stores issue number."""
     raise NotImplementedError()
 
   def PatchsetSetting(self):
@@ -1496,9 +1498,9 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
           self._auth_config or auth.make_auth_config())
     return self._rpc_server
 
-  @staticmethod
-  def IssueSetting(branch):
-    return 'branch.%s.rietveldissue' % branch
+  def IssueSetting(self):
+    """Return the git setting that stores this change's issue."""
+    return 'branch.%s.rietveldissue' % self.GetBranch()
 
   def PatchsetSetting(self):
     """Return the git setting that stores this change's most recent patchset."""
@@ -1548,9 +1550,9 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
         self._gerrit_server = 'https://%s' % self._gerrit_host
     return self._gerrit_server
 
-  @staticmethod
-  def IssueSetting(branch):
-    return 'branch.%s.gerritissue' % branch
+  def IssueSetting(self):
+    """Return the git setting that stores this change's issue."""
+    return 'branch.%s.gerritissue' % self.GetBranch()
 
   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,16 +550,18 @@ 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',
@@ -575,7 +577,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,