Revert of git cl: Rework Changelist class for Rietveld/Gerrit use. (patchset...

Revert of git cl: Rework Changelist class for Rietveld/Gerrit use. (patchset #3 id:40001 of https://codereview.chromium.org/1830973003/ )

Reason for revert:
Speculative revert, see crbug.com/598428.

Original issue's description:
> git cl: Rework Changelist class for Rietveld/Gerrit use.
> 
> This adds pluggable codereview-specific implementations into
> Changelist class. The specific implementation is chosen at
> Changelist automatically, with Rietveld being default for
> backwards compatibility.
> 
> Gerrit implementation for Gerrit is incomplete, and will be
> added in later CLs. However, it is sufficient to ensure
> current functionality of this tool is not diminished.
> 
> Sadly, the base class isn't completely free from Rietveld
> assumptions because of presubmit_support. Apparently, PRESUBMIT
> scripts can make use of Rietveld instance for RPCs directly.
> This use doesn't make sense for Gerrit, which substitutes
> rietveld instance with a dummy object, which raises exception
> on any attribute access with a diagnostic message.
> 
> This also includes refactoring of some related code which
> (ab)used ChangeList. Overall, this CL adds a few extra call to
> git config in order to determine which codereview to use, but
> but it shouldn't have any performance impact.
> 
> 
> 
> These is a reland of these 4 CLs + a fix.
> patch from issue 1827523003 at patchset 20001 (http://crrev.com/1827523003#ps20001)
> patch from issue 1830703004 at patchset 1 (http://crrev.com/1830703004#ps1)
> patch from issue 1830923002 at patchset 60001 (http://crrev.com/1830923002#ps60001)
> patch from issue 1805193002 at patchset 380001 (http://crrev.com/1805193002#ps380001)
> 
> 
> 
> R=machenbach@chromium.org,sergiyb@chromium.org,andybons@chromium.org
> BUG=579160,597638
> 
> Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299506

TBR=andybons@chromium.org,machenbach@chromium.org,sergiyb@chromium.org,tandrii@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/1840833002

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

git_cl.py

@@ -5,7 +5,7 @@
 
 # Copyright (C) 2008 Evan Martin <martine@danga.com>
 
-"""A git-command for integrating reviews on Rietveld and Gerrit."""
+"""A git-command for integrating reviews on Rietveld."""
 
 from distutils.version import LooseVersion
 from multiprocessing.pool import ThreadPool
@@ -47,7 +47,6 @@ import commit_queue
 import dart_format
 import fix_encoding
 import gclient_utils
-import gerrit_util
 import git_cache
 import git_common
 import git_footers
@@ -157,7 +156,7 @@ def ask_for_data(prompt):
 
 
 def git_set_branch_value(key, value):
-  branch = GetCurrentBranch()
+  branch = Changelist().GetBranch()
   if not branch:
     return
 
@@ -169,7 +168,7 @@ def git_set_branch_value(key, value):
 
 
 def git_get_branch_default(key, default):
-  branch = GetCurrentBranch()
+  branch = Changelist().GetBranch()
   if branch:
     git_key = 'branch.%s.%s' % (branch, key)
     (_, stdout) = RunGitWithCode(['config', '--int', '--get', git_key])
@@ -814,61 +813,23 @@ class Settings(object):
 
 def ShortBranchName(branch):
   """Convert a name like 'refs/heads/foo' to just 'foo'."""
-  return branch.replace('refs/heads/', '', 1)
-
-
-def GetCurrentBranchRef():
-  """Returns branch ref (e.g., refs/heads/master) or None."""
-  return RunGit(['symbolic-ref', 'HEAD'],
-                stderr=subprocess2.VOID, error_ok=True).strip() or None
-
-
-def GetCurrentBranch():
-  """Returns current branch or None.
-
-  For refs/heads/* branches, returns just last part. For others, full ref.
-  """
-  branchref = GetCurrentBranchRef()
-  if branchref:
-    return ShortBranchName(branchref)
-  return None
+  return branch.replace('refs/heads/', '')
 
 
 class Changelist(object):
-  """Changelist works with one changelist in local branch.
-
-  Supports two codereview backends: Rietveld or Gerrit, selected at object
-  creation.
-
-  Not safe for concurrent multi-{thread,process} use.
-  """
-
-  def __init__(self, branchref=None, issue=None, codereview=None, **kwargs):
-    """Create a new ChangeList instance.
-
-    If issue is given, the codereview must be given too.
-
-    If `codereview` is given, it must be 'rietveld' or 'gerrit'.
-    Otherwise, it's decided based on current configuration of the local branch,
-    with default being 'rietveld' for backwards compatibility.
-    See _load_codereview_impl for more details.
-
-    **kwargs will be passed directly to codereview implementation.
-    """
+  def __init__(self, branchref=None, issue=None, auth_config=None):
     # Poke settings so we get the "configure your server" message if necessary.
     global settings
     if not settings:
       # Happens when git_cl.py is used as a utility library.
       settings = Settings()
-
-    if issue:
-      assert codereview, 'codereview must be known, if issue is known'
-
+    settings.GetDefaultServerUrl()
     self.branchref = branchref
     if self.branchref:
       self.branch = ShortBranchName(self.branchref)
     else:
       self.branch = None
+    self.rietveld_server = None
     self.upstream_branch = None
     self.lookedup_issue = False
     self.issue = issue or None
@@ -878,39 +839,14 @@ class Changelist(object):
     self.patchset = None
     self.cc = None
     self.watchers = ()
+    self._auth_config = auth_config
+    self._props = None
     self._remote = None
+    self._rpc_server = None
 
-    self._codereview_impl = None
-    self._load_codereview_impl(codereview, **kwargs)
-
-  def _load_codereview_impl(self, codereview=None, **kwargs):
-    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 based on issue number set for a current branch.
-    # Rietveld takes precedence over Gerrit.
-    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
-
-    # 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')
-
+  @property
+  def auth_config(self):
+    return self._auth_config
 
   def GetCCList(self):
     """Return the users cc'd on this CL.
@@ -938,7 +874,8 @@ class Changelist(object):
   def GetBranch(self):
     """Returns the short branch name, e.g. 'master'."""
     if not self.branch:
-      branchref = GetCurrentBranchRef()
+      branchref = RunGit(['symbolic-ref', 'HEAD'],
+                         stderr=subprocess2.VOID, error_ok=True).strip()
       if not branchref:
         return None
       self.branchref = branchref
@@ -982,12 +919,11 @@ class Changelist(object):
             remote = 'origin'
             upstream_branch = 'refs/heads/trunk'
           else:
-            DieWithError(
-               'Unable to determine default branch to diff against.\n'
-               'Either pass complete "git diff"-style arguments, like\n'
-               '  git cl upload origin/master\n'
-               'or verify this branch is set up to track another \n'
-               '(via the --track argument to "git checkout -b ...").')
+            DieWithError("""Unable to determine default branch to diff against.
+Either pass complete "git diff"-style arguments, like
+  git cl upload origin/master
+or verify this branch is set up to track another (via the --track argument to
+"git checkout -b ...").""")
 
     return remote, upstream_branch
 
@@ -1129,24 +1065,65 @@ 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()
+      issue = RunGit(['config', self._IssueSetting()], error_ok=True).strip()
       self.issue = int(issue) or None if issue else None
       self.lookedup_issue = True
     return self.issue
 
+  def GetRietveldServer(self):
+    if not self.rietveld_server:
+      # If we're on a branch then get the server potentially associated
+      # with that branch.
+      if self.GetIssue():
+        rietveld_server_config = self._RietveldServer()
+        if rietveld_server_config:
+          self.rietveld_server = gclient_utils.UpgradeToHttps(RunGit(
+              ['config', rietveld_server_config], error_ok=True).strip())
+      if not self.rietveld_server:
+        self.rietveld_server = settings.GetDefaultServerUrl()
+    return self.rietveld_server
+
+  def GetGerritServer(self):
+    # We don't support multiple Gerrit servers, and assume it to be same as
+    # origin, except with a '-review' suffix for first subdomain.
+    parts = urlparse.urlparse(self.GetRemoteUrl()).netloc.split('.')
+    parts[0] = parts[0] + '-review'
+    return 'https://%s' % '.'.join(parts)
+
   def GetIssueURL(self):
     """Get the URL for a particular issue."""
-    issue = self.GetIssue()
-    if not issue:
+    if not self.GetIssue():
       return None
-    return '%s/%s' % (self._codereview_impl.GetCodereviewServer(), issue)
+    if settings.GetIsGerrit():
+      return '%s/%s' % (self.GetGerritServer(), self.GetIssue())
+    return '%s/%s' % (self.GetRietveldServer(), self.GetIssue())
 
   def GetDescription(self, pretty=False):
     if not self.has_description:
       if self.GetIssue():
-        self.description = self._codereview_impl.FetchDescription()
+        issue = self.GetIssue()
+        try:
+          self.description = self.RpcServer().get_description(issue).strip()
+        except urllib2.HTTPError as e:
+          if e.code == 404:
+            DieWithError(
+                ('\nWhile fetching the description for issue %d, received a '
+                 '404 (not found)\n'
+                 'error. It is likely that you deleted this '
+                 'issue on the server. If this is the\n'
+                 'case, please run\n\n'
+                 '    git cl issue 0\n\n'
+                 'to clear the association with the deleted issue. Then run '
+                 'this command again.') % issue)
+          else:
+            DieWithError(
+                '\nFailed to fetch issue description. HTTP error %d' % e.code)
+        except urllib2.URLError as e:
+          print >> sys.stderr, (
+              'Warning: Failed to retrieve CL description due to network '
+              'failure.')
+          self.description = ''
+
       self.has_description = True
     if pretty:
       wrapper = textwrap.TextWrapper()
@@ -1157,7 +1134,7 @@ 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()],
+      patchset = RunGit(['config', self._PatchsetSetting()],
                         error_ok=True).strip()
       self.patchset = int(patchset) or None if patchset else None
       self.lookedup_patchset = True
@@ -1165,29 +1142,47 @@ class Changelist(object):
 
   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)])
+      RunGit(['config', self._PatchsetSetting(), str(patchset)])
       self.patchset = patchset
     else:
-      RunGit(['config', '--unset', patchset_setting],
+      RunGit(['config', '--unset', self._PatchsetSetting()],
              stderr=subprocess2.PIPE, error_ok=True)
       self.patchset = None
 
+  def GetMostRecentPatchset(self):
+    return self.GetIssueProperties()['patchsets'][-1]
+
+  def GetPatchSetDiff(self, issue, patchset):
+    return self.RpcServer().get(
+        '/download/issue%s_%s.diff' % (issue, patchset))
+
+  def GetIssueProperties(self):
+    if self._props is None:
+      issue = self.GetIssue()
+      if not issue:
+        self._props = {}
+      else:
+        self._props = self.RpcServer().get_issue_properties(issue, True)
+    return self._props
+
+  def GetApprovingReviewers(self):
+    return get_approving_reviewers(self.GetIssueProperties())
+
+  def AddComment(self, message):
+    return self.RpcServer().add_comment(self.GetIssue(), message)
+
   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()
     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])
+      RunGit(['config', self._IssueSetting(), str(issue)])
+      if not settings.GetIsGerrit() and self.rietveld_server:
+        RunGit(['config', self._RietveldServer(), self.rietveld_server])
     else:
       current_issue = self.GetIssue()
       if current_issue:
-        RunGit(['config', '--unset', issue_setting])
+        RunGit(['config', '--unset', self._IssueSetting()])
       self.issue = None
       self.SetPatchset(None)
 
@@ -1237,186 +1232,6 @@ class Changelist(object):
         author,
         upstream=upstream_branch)
 
-  def UpdateDescription(self, description):
-    self.description = description
-    return self._codereview_impl.UpdateDescriptionRemote(description)
-
-  def RunHook(self, committing, may_prompt, verbose, change):
-    """Calls sys.exit() if the hook fails; returns a HookResults otherwise."""
-    try:
-      return presubmit_support.DoPresubmitChecks(change, committing,
-          verbose=verbose, output_stream=sys.stdout, input_stream=sys.stdin,
-          default_presubmit=None, may_prompt=may_prompt,
-          rietveld_obj=self._codereview_impl.GetRieveldObjForPresubmit())
-    except presubmit_support.PresubmitFailure, e:
-      DieWithError(
-          ('%s\nMaybe your depot_tools is out of date?\n'
-           'If all fails, contact maruel@') % e)
-
-  # Forward methods to codereview specific implementation.
-
-  def CloseIssue(self):
-    return self._codereview_impl.CloseIssue()
-
-  def GetStatus(self):
-    return self._codereview_impl.GetStatus()
-
-  def GetCodereviewServer(self):
-    return self._codereview_impl.GetCodereviewServer()
-
-  def GetApprovingReviewers(self):
-    return self._codereview_impl.GetApprovingReviewers()
-
-  def GetMostRecentPatchset(self):
-    return self._codereview_impl.GetMostRecentPatchset()
-
-  def __getattr__(self, attr):
-    # This is because lots of untested code accesses Rietveld-specific stuff
-    # directly, and it's hard to fix for sure. So, just let it work, and fix
-    # on a cases by case basis.
-    return getattr(self._codereview_impl, attr)
-
-
-class _ChangelistCodereviewBase(object):
-  """Abstract base class encapsulating codereview specifics of a changelist."""
-  def __init__(self, changelist):
-    self._changelist = changelist  # instance of Changelist
-
-  def __getattr__(self, attr):
-    # Forward methods to changelist.
-    # TODO(tandrii): maybe clean up _GerritChangelistImpl and
-    # _RietveldChangelistImpl to avoid this hack?
-    return getattr(self._changelist, attr)
-
-  def GetStatus(self):
-    """Apply a rough heuristic to give a simple summary of an issue's review
-    or CQ status, assuming adherence to a common workflow.
-
-    Returns None if no issue for this branch, or specific string keywords.
-    """
-    raise NotImplementedError()
-
-  def GetCodereviewServer(self):
-    """Returns server URL without end slash, like "https://codereview.com"."""
-    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()
-
-  @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()
-
-  def UpdateDescriptionRemote(self, description):
-    """Update the description on codereview site."""
-    raise NotImplementedError()
-
-  def CloseIssue(self):
-    """Closes the issue."""
-    raise NotImplementedError()
-
-  def GetApprovingReviewers(self):
-    """Returns a list of reviewers approving the change.
-
-    Note: not necessarily committers.
-    """
-    raise NotImplementedError()
-
-  def GetMostRecentPatchset(self):
-    """Returns the most recent patchset number from the codereview site."""
-    raise NotImplementedError()
-
-
-class _RietveldChangelistImpl(_ChangelistCodereviewBase):
-  def __init__(self, changelist, auth_config=None, rietveld_server=None):
-    super(_RietveldChangelistImpl, self).__init__(changelist)
-    assert settings, 'must be initialized in _ChangelistCodereviewBase'
-    settings.GetDefaultServerUrl()
-
-    self._rietveld_server = rietveld_server
-    self._auth_config = auth_config
-    self._props = None
-    self._rpc_server = None
-
-  def GetAuthConfig(self):
-    return self._auth_config
-
-  def GetCodereviewServer(self):
-    if not self._rietveld_server:
-      # 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())
-      if not self._rietveld_server:
-        self._rietveld_server = settings.GetDefaultServerUrl()
-    return self._rietveld_server
-
-  def FetchDescription(self):
-    issue = self.GetIssue()
-    assert issue
-    try:
-      return self.RpcServer().get_description(issue).strip()
-    except urllib2.HTTPError as e:
-      if e.code == 404:
-        DieWithError(
-            ('\nWhile fetching the description for issue %d, received a '
-             '404 (not found)\n'
-             'error. It is likely that you deleted this '
-             'issue on the server. If this is the\n'
-             'case, please run\n\n'
-             '    git cl issue 0\n\n'
-             'to clear the association with the deleted issue. Then run '
-             'this command again.') % issue)
-      else:
-        DieWithError(
-            '\nFailed to fetch issue description. HTTP error %d' % e.code)
-    except urllib2.URLError as e:
-      print >> sys.stderr, (
-          'Warning: Failed to retrieve CL description due to network '
-          'failure.')
-      return ''
-
-  def GetMostRecentPatchset(self):
-    return self.GetIssueProperties()['patchsets'][-1]
-
-  def GetPatchSetDiff(self, issue, patchset):
-    return self.RpcServer().get(
-        '/download/issue%s_%s.diff' % (issue, patchset))
-
-  def GetIssueProperties(self):
-    if self._props is None:
-      issue = self.GetIssue()
-      if not issue:
-        self._props = {}
-      else:
-        self._props = self.RpcServer().get_issue_properties(issue, True)
-    return self._props
-
-  def GetApprovingReviewers(self):
-    return get_approving_reviewers(self.GetIssueProperties())
-
-  def AddComment(self, message):
-    return self.RpcServer().add_comment(self.GetIssue(), message)
-
   def GetStatus(self):
     """Apply a rough heuristic to give a simple summary of an issue's review
     or CQ status, assuming adherence to a common workflow.
@@ -1464,11 +1279,26 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
       return 'reply'
     return 'waiting'
 
-  def UpdateDescriptionRemote(self, description):
+  def RunHook(self, committing, may_prompt, verbose, change):
+    """Calls sys.exit() if the hook fails; returns a HookResults otherwise."""
+
+    try:
+      return presubmit_support.DoPresubmitChecks(change, committing,
+          verbose=verbose, output_stream=sys.stdout, input_stream=sys.stdin,
+          default_presubmit=None, may_prompt=may_prompt,
+          rietveld_obj=self.RpcServer())
+    except presubmit_support.PresubmitFailure, e:
+      DieWithError(
+          ('%s\nMaybe your depot_tools is out of date?\n'
+           'If all fails, contact maruel@') % e)
+
+  def UpdateDescription(self, description):
+    self.description = description
     return self.RpcServer().update_description(
         self.GetIssue(), self.description)
 
   def CloseIssue(self):
+    """Updates the description and closes the issue."""
     return self.RpcServer().close_issue(self.GetIssue())
 
   def SetFlag(self, flag, value):
@@ -1492,116 +1322,25 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
     """
     if not self._rpc_server:
       self._rpc_server = rietveld.CachingRietveld(
-          self.GetCodereviewServer(),
+          self.GetRietveldServer(),
           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):
+  def _PatchsetSetting(self):
     """Return the git setting that stores this change's most recent patchset."""
     return 'branch.%s.rietveldpatchset' % self.GetBranch()
 
-  def GetCodereviewServerSetting(self):
+  def _RietveldServer(self):
     """Returns the git setting that stores this change's rietveld server."""
     branch = self.GetBranch()
     if branch:
       return 'branch.%s.rietveldserver' % branch
     return None
 
-  def GetRieveldObjForPresubmit(self):
-    return self.RpcServer()
-
-
-class _GerritChangelistImpl(_ChangelistCodereviewBase):
-  def __init__(self, changelist, auth_config=None):
-    # auth_config is Rietveld thing, kept here to preserve interface only.
-    super(_GerritChangelistImpl, self).__init__(changelist)
-    self._change_id = None
-    self._gerrit_server = None  # e.g. https://chromium-review.googlesource.com
-    self._gerrit_host = None   # e.g. chromium-review.googlesource.com
-
-  def _GetGerritHost(self):
-    # Lazy load of configs.
-    self.GetCodereviewServer()
-    return self._gerrit_host
-
-  def GetCodereviewServer(self):
-    if not self._gerrit_server:
-      # 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
-      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
-
-  @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
-    return None
-
-  def GetRieveldObjForPresubmit(self):
-    class ThisIsNotRietveldIssue(object):
-      def __nonzero__(self):
-        # This is a hack to make presubmit_support think that rietveld is not
-        # defined, yet still ensure that calls directly result in a decent
-        # exception message below.
-        return False
-
-      def __getattr__(self, attr):
-        print(
-            'You aren\'t using Rietveld at the moment, but Gerrit.\n'
-            'Using Rietveld in your PRESUBMIT scripts won\'t work.\n'
-            'Please, either change your PRESUBIT to not use rietveld_obj.%s,\n'
-            'or use Rietveld for codereview.\n'
-            'See also http://crbug.com/579160.' % attr)
-        raise NotImplementedError()
-    return ThisIsNotRietveldIssue()
-
-  def GetStatus(self):
-    # TODO(tandrii)
-    raise NotImplementedError()
-
-  def GetMostRecentPatchset(self):
-    data = gerrit_util.GetChangeDetail(self._GetGerritHost(), self.GetIssue(),
-                                       ['CURRENT_REVISION'])
-    return data['revisions'][data['current_revision']]['_number']
-
-  def FetchDescription(self):
-    data = gerrit_util.GetChangeDetail(self._GetGerritHost(), self.GetIssue(),
-                                       ['COMMIT_FOOTERS', 'CURRENT_REVISION'])
-    return data['revisions'][data['current_revision']]['commit_with_footers']
-
-  def UpdateDescriptionRemote(self, description):
-    # TODO(tandrii)
-    raise NotImplementedError()
-
-  def CloseIssue(self):
-    gerrit_util.AbandonChange(self._GetGerritHost(), self.GetIssue(), msg='')
-
 
 class ChangeDescription(object):
   """Contains a parsed form of the change description."""
@@ -2147,7 +1886,6 @@ def CMDstatus(parser, args):
   changes = (
       Changelist(branchref=b, auth_config=auth_config)
       for b in branches.splitlines())
-  # TODO(tandrii): refactor to use CLs list instead of branches list.
   branches = [c.GetBranch() for c in changes]
   alignment = max(5, max(len(b) for b in branches))
   print 'Branches associated with reviews:'
@@ -2263,7 +2001,7 @@ def CMDcomments(parser, args):
     except ValueError:
       DieWithError('A review issue id is expected to be a number')
 
-  cl = Changelist(issue=issue, codereview='rietveld', auth_config=auth_config)
+  cl = Changelist(issue=issue, auth_config=auth_config)
 
   if options.comment:
     cl.AddComment(options.comment)
@@ -2642,10 +2380,8 @@ def GetTargetRef(remote, remote_branch, target_branch, pending_prefix):
 def RietveldUpload(options, args, cl, change):
   """upload the patch to rietveld."""
   upload_args = ['--assume_yes']  # Don't ask about untracked files.
-  upload_args.extend(['--server', cl.GetCodereviewServer()])
-  # TODO(tandrii): refactor this ugliness into _RietveldChangelistImpl.
-  upload_args.extend(auth.auth_config_to_command_options(
-      cl._codereview_impl.GetAuthConfig()))
+  upload_args.extend(['--server', cl.GetRietveldServer()])
+  upload_args.extend(auth.auth_config_to_command_options(cl.auth_config))
   if options.emulate_svn_auto_props:
     upload_args.append('--emulate_svn_auto_props')
 
@@ -2888,9 +2624,9 @@ def CMDupload(parser, args):
   # during the actual upload.
   if not settings.GetIsGerrit() and auth_config.use_oauth2:
     authenticator = auth.get_authenticator_for_host(
-        cl.GetCodereviewServer(), auth_config)
+        cl.GetRietveldServer(), auth_config)
     if not authenticator.has_cached_credentials():
-      raise auth.LoginRequiredError(cl.GetCodereviewServer())
+      raise auth.LoginRequiredError(cl.GetRietveldServer())
 
   # Apply watchlists on upload.
   change = cl.GetChange(base_branch, None)
@@ -3484,13 +3220,12 @@ def PatchIssue(issue_arg, reject, nocommit, directory, auth_config):
   # consequences of the caller not checking this could be dire.
   assert(not git_common.is_dirty_git_tree('apply'))
 
-  # TODO(tandrii): implement for Gerrit.
   if type(issue_arg) is int or issue_arg.isdigit():
     # Input is an issue id.  Figure out the URL.
     issue = int(issue_arg)
-    cl = Changelist(issue=issue, codereview='rietveld', auth_config=auth_config)
+    cl = Changelist(issue=issue, auth_config=auth_config)
     patchset = cl.GetMostRecentPatchset()
-    patch_data = cl._codereview_impl.GetPatchSetDiff(issue, patchset)
+    patch_data = cl.GetPatchSetDiff(issue, patchset)
   else:
     # Assume it's a URL to the patch. Default to https.
     issue_url = gclient_utils.UpgradeToHttps(issue_arg)
@@ -3499,8 +3234,8 @@ def PatchIssue(issue_arg, reject, nocommit, directory, auth_config):
       DieWithError('Must pass an issue ID or full URL for '
           '\'Download raw patch set\'')
     issue = int(match.group(2))
-    cl = Changelist(issue=issue, codereview='rietveld',
-                    rietvled_server=match.group(1), auth_config=auth_config)
+    cl = Changelist(issue=issue, auth_config=auth_config)
+    cl.rietveld_server = match.group(1)
     patchset = int(match.group(3))
     patch_data = urllib2.urlopen(issue_arg).read()
 
@@ -3544,8 +3279,7 @@ def PatchIssue(issue_arg, reject, nocommit, directory, auth_config):
                              'patch from issue %(i)s at patchset '
                              '%(p)s (http://crrev.com/%(i)s#ps%(p)s)'
                              % {'i': issue, 'p': patchset})])
-    cl = Changelist(codereview='rietveld', auth_config=auth_config,
-                    rietveld_server=cl.GetCodereviewServer())
+    cl = Changelist(auth_config=auth_config)
     cl.SetIssue(issue)
     cl.SetPatchset(patchset)
     print "Committed patch locally."

tests/git_cl_test.py

@@ -163,21 +163,20 @@ class TestGitCl(TestCase):
                    '-M'+similarity, 'fake_ancestor_sha', 'HEAD'],), '+dat')
 
     return [
+      ((['git', 'config', 'rietveld.autoupdate'],), ''),
+      ((['git', 'config', 'rietveld.server'],),
+       'codereview.example.com'),
       ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
       similarity_call,
       ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
       find_copies_call,
       ((['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'],),
        'fake_ancestor_sha'),
+      ((['git', 'config', 'gerrit.host'],), ''),
+      ((['git', 'config', 'branch.master.rietveldissue'],), ''),
       ] + cls._git_sanity_checks('fake_ancestor_sha', 'master') + [
       ((['git', 'rev-parse', '--show-cdup'],), ''),
       ((['git', 'rev-parse', 'HEAD'],), '12345'),
@@ -280,6 +279,8 @@ class TestGitCl(TestCase):
           'svn-remote.svn.fetch trunk/src:refs/remotes/origin/master'),
          None),
         0)),
+      ((['git',
+         'config', 'rietveld.server'],), 'codereview.example.com'),
       ((['git', 'symbolic-ref', 'HEAD'],), 'refs/heads/working'),
       ((['git', 'config', '--int', '--get',
         'branch.working.git-cl-similarity'],), ''),
@@ -287,10 +288,6 @@ class TestGitCl(TestCase):
       ((['git', 'config', '--int', '--get',
         'branch.working.git-find-copies'],), ''),
       ((['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'),
@@ -324,6 +321,8 @@ class TestGitCl(TestCase):
          'diff', '--name-status', '--no-renames', '-r', 'fake_ancestor_sha...',
          '.'],),
         'M\tPRESUBMIT.py'),
+      ((['git',
+         'config', 'branch.working.rietveldissue'],), '12345'),
       ((['git',
          'config', 'branch.working.rietveldpatchset'],), '31137'),
       ((['git', 'config', 'branch.working.rietveldserver'],),
@@ -335,6 +334,8 @@ class TestGitCl(TestCase):
   @classmethod
   def _dcommit_calls_bypassed(cls):
     return [
+      ((['git',
+         'config', 'branch.working.rietveldissue'],), '12345'),
       ((['git', 'config', 'branch.working.rietveldserver'],),
          'codereview.example.com'),
   ]
@@ -342,6 +343,7 @@ class TestGitCl(TestCase):
   @classmethod
   def _dcommit_calls_3(cls):
     return [
+      ((['git', 'config', 'gerrit.host'],), ''),
       ((['git',
          'diff', '--no-ext-diff', '--stat', '--find-copies-harder',
          '-l100000', '-C50', 'fake_ancestor_sha',
@@ -544,6 +546,10 @@ class TestGitCl(TestCase):
   @classmethod
   def _gerrit_base_calls(cls):
     return [
+        ((['git', 'config', 'rietveld.autoupdate'],),
+         ''),
+        ((['git',
+           'config', 'rietveld.server'],), 'codereview.example.com'),
         ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
         ((['git', 'config', '--int', '--get',
           'branch.master.git-cl-similarity'],), ''),
@@ -551,14 +557,11 @@ class TestGitCl(TestCase):
         ((['git', 'config', '--int', '--get',
           'branch.master.git-find-copies'],), ''),
         ((['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'),
+        ((['git', 'config', 'gerrit.host'],), 'True'),
         ] + cls._git_sanity_checks('fake_ancestor_sha', 'master') + [
         ((['git', 'rev-parse', '--show-cdup'],), ''),
         ((['git', 'rev-parse', 'HEAD'],), '12345'),
@@ -566,7 +569,9 @@ class TestGitCl(TestCase):
            'diff', '--name-status', '--no-renames', '-r',
            'fake_ancestor_sha...', '.'],),
          'M\t.gitignore\n'),
-        ((['git', 'config', 'branch.master.gerritpatchset'],), ''),
+        ((['git', 'config', 'branch.master.rietveldissue'],), ''),
+        ((['git',
+           'config', 'branch.master.rietveldpatchset'],), ''),
         ((['git',
            'log', '--pretty=format:%s%n%n%b', 'fake_ancestor_sha...'],),
          'foo'),
@@ -657,12 +662,7 @@ class TestGitCl(TestCase):
         ]
     if squash:
       calls += [
-          ((['git', 'config', 'branch.master.gerritissue', '123456'],), ''),
-          ((['git', 'config', 'branch.master.gerritserver'],), ''),
-          ((['git', 'config', 'remote.origin.url'],),
-            'https://chromium.googlesource.com/my/repo.git'),
-          ((['git', 'config', 'branch.master.gerritserver',
-          'https://chromium-review.googlesource.com'],), ''),
+          ((['git', 'config', 'branch.master.rietveldissue', '123456'],), ''),
           ((['git', 'rev-parse', 'HEAD'],), 'abcdef0123456789'),
           ((['git', 'update-ref', '-m', 'Uploaded abcdef0123456789',
             'refs/heads/git_cl_uploads/master', 'abcdef0123456789'],),
@@ -886,12 +886,9 @@ class TestGitCl(TestCase):
     self.assertNotEqual(git_cl.main(['diff']), 0)
 
   def _patch_common(self):
-    self.mock(git_cl._RietveldChangelistImpl, 'GetMostRecentPatchset',
-              lambda x: '60001')
-    self.mock(git_cl._RietveldChangelistImpl, 'GetPatchSetDiff',
-              lambda *args: None)
-    self.mock(git_cl.Changelist, 'GetDescription',
-              lambda *args: 'Description')
+    self.mock(git_cl.Changelist, 'GetMostRecentPatchset', lambda x: '60001')
+    self.mock(git_cl.Changelist, 'GetPatchSetDiff', lambda *args: None)
+    self.mock(git_cl.Changelist, 'GetDescription', lambda *args: 'Description')
     self.mock(git_cl.Changelist, 'SetIssue', lambda *args: None)
     self.mock(git_cl.Changelist, 'SetPatchset', lambda *args: None)
     self.mock(git_cl, 'IsGitVersionAtLeast', lambda *args: True)
@@ -911,8 +908,6 @@ class TestGitCl(TestCase):
          'Description\n\n' +
          'patch from issue 123456 at patchset 60001 ' +
          '(http://crrev.com/123456#ps60001)'],), ''),
-      ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
-      ((['git', 'config', 'branch.master.rietveldserver'],), ''),
     ]
     self.assertEqual(git_cl.main(['patch', '123456']), 0)