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 130 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':

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

Why not use an enum? Or at least define these as constants, so that users can
autocomplete to reduce typos.

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

because it's python, and having RIETVELD = 'rietveld' is pointless.
+ my editor autocompletes strings, too :)

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

Acknowledged.

+        self._codereview_impl = _GerritChangelistImpl(self)
+      elif codereview == 'rietveld':
+        self._codereview_impl = _RietveldChangelistImpl(self, **kwargs)
+      else:
+        assert codereview in ('rietveld', 'gerrit')

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

nit: IMHO

assert False, 'unsupported codereview'

would be enough

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

yes, but AssertionError is much nicer when you see what's allowed.

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

Acknowledged.

+      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.

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

Why not check if it's gerrit first? Then you won't need to save a tmp object
here.

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

because if Rietveld issue is defined, it should be rietveld, even if gerrit
issue is also defined. This is a safeguard. Thus, doing it reverse requires
having a tmp_gerrit.

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

Acknowledged.

+
+    # 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'

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

nit: remove \n in the end - let the terminal break the long line where needed.

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

well, it's apparently customary in this file to break ourselves. And IMO, in
this case specifically it's nicer.

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

Acknowledged.

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

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

nit: codereview_server = self._codereview_impl.GetCodereviewServer and then
re-use twice

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

Done.

+        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)

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

discussed this with tandrii offline: 'maruel@' was here before, but otherwise
I'd recommend putting group address rather than direct names, e.g.
chrome-infra@... but it's up to tandrii and maruel

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

Acknowledged.

+
+  def __getattr__(self, attr):
+    # Forward methods to codereview specific implementation.

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

I think this was intended to be above the def __getattr__ definition as it seems
to apply to the rest of the methods in this class

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

Yes, though I still need specific comment for why __getattr__ is needed.

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

Acknowledged.

+    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
+    # _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):

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

Would be nice to have a doc here to know what kind of object is expected and/or
what to return if the implementation can not provide one... or just mention that
this is a hack and that no-one should ever implement this in a subclass :-)

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

Done.

+    # 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

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

nit: please add

global settings

on the line before... even though we don't modify it here, it's confusing for
the reader and requires knowing that it's defined somewhere on the global level

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

good point.

+    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'

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

nit: please remove \n everywhere a sentence continues. let the terminal do the
breaking... feel free to do it in another patch though

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

see above - this seems to be the style here. And this message itself is
certainly a copy-paste from before. leaving as is.

that said, I actually prefer terminal breaking here. Ah, also mind that some
scripts parse git cl output using regexes assuming end of line. It's bad, yes,
but I don't want to revert my CL for the reason of breaking them :(

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

what a can of worms...
Ack.

+             '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):

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

OMG, I would never have guessed what this function returns from the name...
reminds me of Verified class in the CQ code, but worse. Thanks for renaming it.

+  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):
+    super(_GerritChangelistImpl, self).__init__(changelist)
+    self._change_id = None
+    self._gerrit_server = None
+
+  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 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_server = 'https://%s' % '.'.join(parts)
+    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'
+                'or use Rietveld for codereview.\n' % attr)
+          raise NotImplementedError()
+        return handler
+    return ThisIsNotRietveldIssue()
+
+  def GetStatus(self):
+    raise NotImplementedError()
+
+  def FetchDescription(self):
+    raise NotImplementedError()
+
+  def UpdateDescriptionRemote(self, description):
+    raise NotImplementedError()
+
+  def CloseIssue(self):
+    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.

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

Why? Sometimes I use 'git cl status' to see if a branch has an associated CL or
not. It's true that I can do the same by checking if it's missing from the list
or not, but IMHO it's nice when a branch is listed and explicitly marked as not
having an associated CL.

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

see line 2111 - there is a list of branches.
then they get list of changes for each branch (right above) - nothing is missed,
don't worry.
but line below (2120) - they get branch names the way CLs see them. Now, the
only reason is that they want branch names without "refs/heads" apparently. And
then inside the loop, for each branch they create a new CL object, again.
WHY????
So, my TODO is to rewrite loop below to be based on CL objects, not branches.

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

Acknowledged.

   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()))

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

Since this is also Rietveld-specific and accesses private field, please add a
TODO comment in the beginning of RietveldUpload:

# TODO(tandrii): Move into implementation class.

or better yet actually move RietveldUpload and GerritUpload into
_RietveldChangelistImpl and _GerritChangelistImpl classes respectively and wrap
both of them with a public Upload method in Changelist. In this case, get rid of
cl parameter. The methods can be left here for compatibilty:


def RietveldUpload(options, args, cl, change):
  cl.Upload(options, args, change)


def GerritUpload(options, args, cl, change):
  cl.Upload(options, args, change)

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

that's exactly what I intended to do! But not in this CL :)

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

Acknowledged.

   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)