git cl: Rework Changelist class for Rietveld/Gerrit use.

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."""
+"""A git-command for integrating reviews on Rietveld and Gerrit."""
 
 from distutils.version import LooseVersion
 from multiprocessing.pool import ThreadPool
 import base64
 import collections
 import glob
 import httplib
 import json
 import logging
 import optparse
@@ skipping 21 matching lines @@
 from third_party import colorama
 from third_party import httplib2
 from third_party import upload
 import auth
 from luci_hacks import trigger_luci_job as luci_trigger
 import clang_format
 import commit_queue
 import dart_format
 import fix_encoding
 import gclient_utils
+import gerrit_util
 import git_cache
 import git_common
 import git_footers
 import owners
 import owners_finder
 import presubmit_support
 import rietveld
 import scm
 import subcommand
 import subprocess2
@@ skipping 89 matching lines @@
 
 def ask_for_data(prompt):
   try:
     return raw_input(prompt)
   except KeyboardInterrupt:
     # Hide the exception.
     sys.exit(1)
 
 
 def git_set_branch_value(key, value):
-  branch = Changelist().GetBranch()
+  branch = GetCurrentBranch()
   if not branch:
     return
 
   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_default(key, default):
-  branch = Changelist().GetBranch()
+  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
   return default
 
 
@@ skipping 624 matching lines @@
   def _GetBranchConfig(self, branch_name, param, **kwargs):
     return self._GetConfig('branch.' + branch_name + '.' + param, **kwargs)
 
   def _GetConfig(self, param, **kwargs):
     self.LazyUpdateIfNeeded()
     return RunGit(['config', param], **kwargs).strip()
 
 
 def ShortBranchName(branch):
   """Convert a name like 'refs/heads/foo' to just 'foo'."""
-  return branch.replace('refs/heads/', '')
+  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
 
 
 class Changelist(object):
-  def __init__(self, branchref=None, issue=None, auth_config=None):
+  """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.
+    """
     # 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()
-    settings.GetDefaultServerUrl()
+
+    if issue:
+      assert codereview, 'codereview must be known, if issue is known'
+
     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
     self.has_description = False
     self.description = None
     self.lookedup_patchset = False
     self.patchset = None
     self.cc = None
     self.watchers = ()
-    self._auth_config = auth_config
-    self._props = None
     self._remote = None
-    self._rpc_server = None
 
-  @property
-  def auth_config(self):
-    return self._auth_config
+    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)
+      elif codereview == 'rietveld':
+        self._codereview_impl = _RietveldChangelistImpl(self, **kwargs)
+      else:
+        assert codereview in ('rietveld', 'gerrit')
+      return
+
+    # Automatic selection.
+    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)
+    if self.GetIssue(force_lookup=True):
+      return
+    # If Gerrit is set repo-wide => Gerrit.
+    if settings.GetIsGerrit():
+      return
+
+    self._codereview_impl = tmp_rietveld
+    return
+
 
   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)
       self.cc = ','.join(filter(None, (base_cc, more_cc))) or ''
     return self.cc
 
   def GetCCListWithoutDefault(self):
     """Return the users cc'd on this CL excluding default ones."""
     if self.cc is None:
       self.cc = ','.join(self.watchers)
     return self.cc
 
   def SetWatchers(self, watchers):
     """Set the list of email addresses that should be cc'd based on the changed
        files in this CL.
     """
     self.watchers = watchers
 
   def GetBranch(self):
     """Returns the short branch name, e.g. 'master'."""
     if not self.branch:
-      branchref = RunGit(['symbolic-ref', 'HEAD'],
-                         stderr=subprocess2.VOID, error_ok=True).strip()
+      branchref = GetCurrentBranchRef()
       if not branchref:
         return None
       self.branchref = branchref
       self.branch = ShortBranchName(self.branchref)
     return self.branch
 
   def GetBranchRef(self):
     """Returns the full branch name, e.g. 'refs/heads/master'."""
     self.GetBranch()  # Poke the lazy loader.
     return self.branchref
@@ skipping 23 matching lines @@
           if 'origin/master' in remote_branches:
             # Fall back on origin/master if it exits.
             remote = 'origin'
             upstream_branch = 'refs/heads/master'
           elif 'origin/trunk' in remote_branches:
             # Fall back on origin/trunk if it exists. Generally a shared
             # git-svn clone
             remote = 'origin'
             upstream_branch = 'refs/heads/trunk'
           else:
-            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 ...").""")
+            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 ...").')
 
     return remote, upstream_branch
 
   def GetCommonAncestorWithUpstream(self):
     upstream_branch = self.GetUpstreamBranch()
     if not BranchExists(upstream_branch):
       DieWithError('The upstream for the current branch (%s) does not exist '
                    'anymore.\nPlease fix it and try again.' % self.GetBranch())
     return git_common.get_or_create_merge_base(self.GetBranch(),
                                                upstream_branch)
@@ skipping 118 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):
+  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._IssueSetting()], error_ok=True).strip()
+    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
     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."""
-    if not self.GetIssue():
+    issue = self.GetIssue()
+    if not issue:
       return None
-    if settings.GetIsGerrit():
-      return '%s/%s' % (self.GetGerritServer(), self.GetIssue())
-    return '%s/%s' % (self.GetRietveldServer(), self.GetIssue())
+    return '%s/%s' % (self._codereview_impl.GetCodereviewServer(), issue)
 
   def GetDescription(self, pretty=False):
     if not self.has_description:
       if self.GetIssue():
-        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.description = self._codereview_impl.FetchDescription()
       self.has_description = True
     if pretty:
       wrapper = textwrap.TextWrapper()
       wrapper.initial_indent = wrapper.subsequent_indent = '  '
       return wrapper.fill(self.description)
     return self.description
 
   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._PatchsetSetting()],
+      patchset = RunGit(['config', self._codereview_impl.PatchsetSetting()],
                         error_ok=True).strip()
       self.patchset = int(patchset) or None if patchset else None
       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', self._PatchsetSetting(), str(patchset)])
+      RunGit(['config', patchset_setting, str(patchset)])
       self.patchset = patchset
     else:
-      RunGit(['config', '--unset', self._PatchsetSetting()],
+      RunGit(['config', '--unset', patchset_setting],
              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()
+    codereview_setting = self._codereview_impl.GetCodereviewServerSetting()
     if issue:
       self.issue = issue
-      RunGit(['config', self._IssueSetting(), str(issue)])
-      if not settings.GetIsGerrit() and self.rietveld_server:
-        RunGit(['config', self._RietveldServer(), self.rietveld_server])
+      RunGit(['config', issue_setting, str(issue)])
+      if self._codereview_impl.GetCodereviewServer():
+        RunGit(['config', codereview_setting,
+                self._codereview_impl.GetCodereviewServer()])
     else:
       current_issue = self.GetIssue()
       if current_issue:
-        RunGit(['config', '--unset', self._IssueSetting()])
+        RunGit(['config', '--unset', issue_setting])
       self.issue = None
       self.SetPatchset(None)
 
   def GetChange(self, upstream_branch, author):
     if not self.GitSanityChecks(upstream_branch):
       DieWithError('\nGit sanity check failure')
 
     root = settings.GetRelativeRoot()
     if not root:
       root = '.'
@@ skipping 29 matching lines @@
     return presubmit_support.GitChange(
         name,
         description,
         absroot,
         files,
         issue,
         patchset,
         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)
+
+  def __getattr__(self, attr):
+    # Forward methods to codereview specific implementation.
+    return getattr(self._codereview_impl, attr)
+
+  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()
+
+
+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

[tandrii(chromium), 2016/03/22 17:41:52]

sergiyb@ this is the ugly hack that you've missed. That said, I probably should
fix it, but I'm still not 100% that tests provide good goverage.

[Sergiy Byelozyorov, 2016/03/23 10:22:42]

I didn't miss it. I saw your comment and decided that it's fine. Or do you mean
that I missed it with respect to some other comment?

+    # _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()
+
+  def IssueSetting(self):
+    """Returns name of git config setting which stores issue number."""
+    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.
+    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
+    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.
 
     Returns None if no issue for this branch, or one of the following keywords:
       * 'error'   - error from review tool (including deleted issues)
       * 'unsent'  - not sent for review
       * 'waiting' - waiting for review
       * 'reply'   - waiting for owner to reply to review
       * 'lgtm'    - LGTM from at least one approved reviewer
       * 'commit'  - in the commit queue
       * 'closed'  - closed
     """
@@ skipping 24 matching lines @@
     messages = props.get('messages') or []
 
     if not messages:
       # No message was sent.
       return 'unsent'
     if messages[-1]['sender'] != props.get('owner_email'):
       # Non-LGTM reply from non-owner
       return 'reply'
     return 'waiting'
 
-  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
+  def UpdateDescriptionRemote(self, 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):
     """Patchset must match."""
     if not self.GetPatchset():
       DieWithError('The patchset needs to match. Send another patchset.')
     try:
       return self.RpcServer().set_flag(
           self.GetIssue(), self.GetPatchset(), flag, value)
     except urllib2.HTTPError, e:
       if e.code == 404:
         DieWithError('The issue %s doesn\'t exist.' % self.GetIssue())
       if e.code == 403:
         DieWithError(
             ('Access denied to issue %s. Maybe the patchset %s doesn\'t '
              'match?') % (self.GetIssue(), self.GetPatchset()))
       raise
 
   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.GetRietveldServer(),
+          self.GetCodereviewServer(),
           self._auth_config or auth.make_auth_config())
     return self._rpc_server
 
-  def _IssueSetting(self):
+  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 _RietveldServer(self):
+  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
 
+  def GetRieveldObjForPresubmit(self):
+    return self.RpcServer()
+
+
+class _GerritChangelistImpl(_ChangelistCodereviewBase):
+  def __init__(self, changelist, auth_config=None):

[Sergiy Byelozyorov, 2016/03/21 17:17:07]

please add a comment explaining that auth_config parameter is needed for
legacy-compatibility here

[tandrii(chromium), 2016/03/22 17:41:53]

good point.

+    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.
+    if self.GetCodereviewServer():

[Sergiy Byelozyorov, 2016/03/21 17:17:07]

did you miss 'not' here?

[tandrii(chromium), 2016/03/22 17:41:52]

yeah, totally buggy. Fixed in next PS.

+      return
+    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():

[Sergiy Byelozyorov, 2016/03/21 17:17:07]

I think GetIssue is part of the Changelist class, not _ChangelistCodereviewBase
or this class... so this will probably fail

[tandrii(chromium), 2016/03/22 17:41:52]

no, because of a __getattr__ in ChangeListImplBase, which forward to Changelist
class. I am not changing now,though I'm considering to change this everywhere.
The problem is that it's less readable, though easier to navigate code.

[Sergiy Byelozyorov, 2016/03/23 10:22:42]

Acknowledged.

+        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('.')

[Sergiy Byelozyorov, 2016/03/21 17:17:07]

ditto for GetRemoteUrl...

[tandrii(chromium), 2016/03/22 17:41:52]

ditto.

[Sergiy Byelozyorov, 2016/03/23 10:22:42]

Acknowledged.

+        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()
+
+  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 __getattr__(self, attr):
+        def handler(*_, **__):
+          print('You aren\'t using Rietveld at the moment, but Gerrit.\n'
+                'Using Rietveld in your PRESUBMIT scripts won\'t work.\n'
+                'Either change your PRESUBIT to not use rietveld_obj.%s,\n'

[Sergiy Byelozyorov, 2016/03/21 17:17:07]

nit: please add 'Please' in the beginning of the sentence

[tandrii(chromium), 2016/03/22 17:41:52]

Done.

+                'or use Rietveld for codereview.\n' % attr)
+          raise NotImplementedError()
+        return handler
+    return ThisIsNotRietveldIssue()
+
+  def GetStatus(self):
+    # TODO(tandrii)
+    raise NotImplementedError()
+
+  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):
+    # TODO(tandrii)
+    raise NotImplementedError()
+
 
 class ChangeDescription(object):
   """Contains a parsed form of the change description."""
   R_LINE = r'^[ \t]*(TBR|R)[ \t]*=[ \t]*(.*?)[ \t]*$'
   BUG_LINE = r'^[ \t]*(BUG)[ \t]*=[ \t]*(.*?)[ \t]*$'
 
   def __init__(self, description):
     self._description_lines = (description or '').strip().splitlines()
 
   @property               # www.logilab.org/ticket/89786
@@ skipping 525 matching lines @@
     return 0
 
   branches = RunGit(['for-each-ref', '--format=%(refname)', 'refs/heads'])
   if not branches:
     print('No local branch found.')
     return 0
 
   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:'
   output = get_cl_statuses(branches,
                            fine_grained=not options.fast,
                            max_processes=options.maxjobs,
                            auth_config=auth_config)
 
   branch_statuses = {}
   alignment = max(5, max(len(ShortBranchName(b)) for b in branches))
@@ skipping 95 matching lines @@
   options, args = parser.parse_args(args)
   auth_config = auth.extract_auth_config_from_options(options)
 
   issue = None
   if options.issue:
     try:
       issue = int(options.issue)
     except ValueError:
       DieWithError('A review issue id is expected to be a number')
 
-  cl = Changelist(issue=issue, auth_config=auth_config)
+  cl = Changelist(issue=issue, codereview='rietveld', auth_config=auth_config)
 
   if options.comment:
     cl.AddComment(options.comment)
     return 0
 
   data = cl.GetIssueProperties()
   summary = []
   for message in sorted(data.get('messages', []), key=lambda x: x['date']):
     summary.append({
         'date': message['date'],
@@ skipping 358 matching lines @@
     remote_branch = remote_branch.replace('refs/remotes/', 'refs/')
   # If a pending prefix exists then replace refs/ with it.
   if pending_prefix:
     remote_branch = remote_branch.replace('refs/', pending_prefix)
   return remote_branch
 
 
 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.GetRietveldServer()])
-  upload_args.extend(auth.auth_config_to_command_options(cl.auth_config))
+  upload_args.extend(['--server', cl.GetCodereviewServer()])
+  upload_args.extend(auth.auth_config_to_command_options(
+      cl._codereview_impl.GetAuthConfig()))
   if options.emulate_svn_auto_props:
     upload_args.append('--emulate_svn_auto_props')
 
   change_desc = None
 
   if options.email is not None:
     upload_args.extend(['--email', options.email])
 
   if cl.GetIssue():
     if options.title:
@@ skipping 222 matching lines @@
 
     # Default to diffing against common ancestor of upstream branch
     base_branch = cl.GetCommonAncestorWithUpstream()
     args = [base_branch, 'HEAD']
 
   # Make sure authenticated to Rietveld before running expensive hooks. It is
   # a fast, best efforts check. Rietveld still can reject the authentication
   # during the actual upload.
   if not settings.GetIsGerrit() and auth_config.use_oauth2:
     authenticator = auth.get_authenticator_for_host(
-        cl.GetRietveldServer(), auth_config)
+        cl.GetCodereviewServer(), auth_config)
     if not authenticator.has_cached_credentials():
-      raise auth.LoginRequiredError(cl.GetRietveldServer())
+      raise auth.LoginRequiredError(cl.GetCodereviewServer())
 
   # Apply watchlists on upload.
   change = cl.GetChange(base_branch, None)
   watchlist = watchlists.Watchlists(change.RepositoryRoot())
   files = [f.LocalPath() for f in change.AffectedFiles()]
   if not options.bypass_watchlists:
     cl.SetWatchers(watchlist.GetWatchersForPaths(files))
 
   if not options.bypass_hooks:
     if options.reviewers or options.tbr_owners:
@@ skipping 573 matching lines @@
   return PatchIssue(issue_arg, options.reject, options.nocommit,
                     options.directory, auth_config)
 
 
 def PatchIssue(issue_arg, reject, nocommit, directory, auth_config):
   # PatchIssue should never be called with a dirty tree.  It is up to the
   # caller to check this, but just in case we assert here since the
   # 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, auth_config=auth_config)
+    cl = Changelist(issue=issue, codereview='rietveld', auth_config=auth_config)
     patchset = cl.GetMostRecentPatchset()
-    patch_data = cl.GetPatchSetDiff(issue, patchset)
+    patch_data = cl._codereview_impl.GetPatchSetDiff(issue, patchset)
   else:
     # Assume it's a URL to the patch. Default to https.
     issue_url = gclient_utils.UpgradeToHttps(issue_arg)
     match = re.match(r'(.*?)/download/issue(\d+)_(\d+).diff', issue_url)
     if not match:
       DieWithError('Must pass an issue ID or full URL for '
           '\'Download raw patch set\'')
     issue = int(match.group(2))
-    cl = Changelist(issue=issue, auth_config=auth_config)
-    cl.rietveld_server = match.group(1)
+    cl = Changelist(issue=issue, codereview='rietveld',
+                    rietvled_server=match.group(1), auth_config=auth_config)
     patchset = int(match.group(3))
     patch_data = urllib2.urlopen(issue_arg).read()
 
   # Switch up to the top-level directory, if necessary, in preparation for
   # applying the patch.
   top = settings.GetRelativeRoot()
   if top:
     os.chdir(top)
 
   # Git patches have a/ at the beginning of source paths.  We strip that out
@@ skipping 23 matching lines @@
   except subprocess2.CalledProcessError:
     print 'Failed to apply the patch'
     return 1
 
   # If we had an issue, commit the current state and register the issue.
   if not nocommit:
     RunGit(['commit', '-m', (cl.GetDescription() + '\n\n' +
                              'patch from issue %(i)s at patchset '
                              '%(p)s (http://crrev.com/%(i)s#ps%(p)s)'
                              % {'i': issue, 'p': patchset})])
-    cl = Changelist(auth_config=auth_config)
+    cl = Changelist(codereview='rietveld', auth_config=auth_config,
+                    rietveld_server=cl.GetCodereviewServer())
     cl.SetIssue(issue)
     cl.SetPatchset(patchset)
     print "Committed patch locally."
   else:
     print "Patch applied to index."
   return 0
 
 
 def CMDrebase(parser, args):
   """Rebases current branch on top of svn repo."""
@@ skipping 714 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)