git cl: refactor Changelist codereview detection.

git_cl.py

 #!/usr/bin/env python
 # Copyright (c) 2012 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.
 
 # Copyright (C) 2008 Evan Martin <martine@danga.com>
 
 """A git-command for integrating reviews on Rietveld and Gerrit."""
 
 from distutils.version import LooseVersion
@@ skipping 876 matching lines @@
     if codereview:
       codereview = codereview.lower()
       if codereview == 'gerrit':
         self._codereview_impl = _GerritChangelistImpl(self, **kwargs)
       elif codereview == 'rietveld':
         self._codereview_impl = _RietveldChangelistImpl(self, **kwargs)
       else:
         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
+    # 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
 
-    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
-
-    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(

[Michael Achenbach, 2016/03/24 12:44:54]

nit: indentation

[tandrii(chromium), 2016/03/24 12:48:48]

fix: https://codereview.chromium.org/1830703004

+          codereview='gerrit' if settings.GetIsGerrit() else 'rietveld')
 
 
   def GetCCList(self):
     """Return the users cc'd on this CL.
 
     Return is a string suitable for passing to gcl with the --cc flag.
     """
     if self.cc is None:
       base_cc = settings.GetDefaultCCList()
       more_cc = ','.join(self.watchers)
@@ skipping 196 matching lines @@
     remote, _ = self.GetRemoteBranch()
     url = RunGit(['config', 'remote.%s.url' % remote], error_ok=True).strip()
 
     # If URL is pointing to a local directory, it is probably a git cache.
     if os.path.isdir(url):
       url = RunGit(['config', 'remote.%s.url' % remote],
                    error_ok=True,
                    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
     return self.issue
 
   def GetIssueURL(self):
     """Get the URL for a particular issue."""
     issue = self.GetIssue()
     if not issue:
       return None
@@ skipping 25 matching lines @@
     if patchset:
       RunGit(['config', patchset_setting, str(patchset)])
       self.patchset = patchset
     else:
       RunGit(['config', '--unset', patchset_setting],
              stderr=subprocess2.PIPE, error_ok=True)
       self.patchset = None
 
   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
       RunGit(['config', issue_setting, str(issue)])
       codereview_server = self._codereview_impl.GetCodereviewServer()
       if codereview_server:
         RunGit(['config', codereview_setting, codereview_server])
     else:
       current_issue = self.GetIssue()
       if current_issue:
@@ skipping 111 matching lines @@
     raise NotImplementedError()
 
   def FetchDescription(self):
     """Fetches and returns description from the codereview server."""
     raise NotImplementedError()
 
   def GetCodereviewServerSetting(self):
     """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):
     """Returns name of git config setting which stores issue number."""
     raise NotImplementedError()
 
   def GetRieveldObjForPresubmit(self):
     # This is an unfortunate Rietveld-embeddedness in presubmit.
     # For non-Rietveld codereviews, this probably should return a dummy object.
     raise NotImplementedError()
@@ skipping 164 matching lines @@
 
   def RpcServer(self):
     """Returns an upload.RpcServer() to access this review's rietveld instance.
     """
     if not self._rpc_server:
       self._rpc_server = rietveld.CachingRietveld(
           self.GetCodereviewServer(),
           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."""
     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
@@ skipping 29 matching lines @@
             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.
         parts = urlparse.urlparse(self.GetRemoteUrl()).netloc.split('.')
         parts[0] = parts[0] + '-review'
         self._gerrit_host = '.'.join(parts)
         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."""
     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
@@ skipping 2703 matching lines @@
 if __name__ == '__main__':
   # These affect sys.stdout so do it outside of main() to simplify mocks in
   # unit testing.
   fix_encoding.fix_encoding()
   colorama.init()
   try:
     sys.exit(main(sys.argv[1:]))
   except KeyboardInterrupt:
     sys.stderr.write('interrupted\n')
     sys.exit(1)