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)
 
 
-class Changelist(object):
-  def __init__(self, branchref=None, issue=None, auth_config=None):
+def GetCurrentBranchRef():
+  """Returns branch ref (e.g., refs/heads/master) or None."""
+  return RunGit(['symbolic-ref', 'HEAD'],

[Michael Achenbach, 2016/03/16 09:28:19]

The old implementation of ChangeList GetBranch could call this method to avoid
duplication.

[tandrii(chromium), 2016/03/21 15:06:05]

Done.

+                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
+
+
+def Changelist(branchref=None, issue=None, auth_config=None):

[Michael Achenbach, 2016/03/16 09:28:19]

Crazy stuff like this might break - not sure if it's the only occurrence:
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/git_...

[tandrii(chromium), 2016/03/21 15:06:05]

I'm uploading fixes to the rest of depot_tools. If outsides use it, well, there
is only one sure way to figure out = landing this.

+  """Returns Changelist instance for Gerrit or Rietveld automatically.
+
+  If branch has exactly 1 associated issue, then whichever codereview the issue is from.
+  Otherwise, Rietveld.
+  """
+  # TODO(tandrii): if issue is given, then probably codereview is known as well,
+  # so we should refactor the calling code and call appropriate implementation
+  # class directly.

[Michael Achenbach, 2016/03/16 09:28:19]

As you write, this piece needs more refactoring in the future. It would be nice
if we could determine which class to create outside of those classes.

[tandrii(chromium), 2016/03/21 15:06:05]

so, i haven't yet decided what signals should be used for decision. But there
are three info sources to determine which type of CL:

1. per-repo setting (either codereview.settings or .git/config record)
2. overriding on cmd
3. per-branch setting

Currently, only #1 is supported, but I think I'll need #3, and possibly #2. So,
for now, i'm merely ensuring we are defaulting to Rietveld, with a possibility
for Gerrit override.

+  rietveld_cl = RietveldChangelist(branchref=branchref, issue=issue,
+                                   auth_config=auth_config)
+  # Check if there is associated Rietveld Issue first,

[Michael Achenbach, 2016/03/16 09:28:19]

nit: s/Issue/issue

[tandrii(chromium), 2016/03/21 15:06:05]

Done.

+  # so as to avoid checking Gerrit issue and polluting test cases.
+  if rietveld_cl.GetIssue():
+    return rietveld_cl
+  assert not issue, 'if issue, then the "if" condition above must have held!'
+
+  # GetIssue() actually calls GetBranch() which shells out git call,
+  # unless branchref is known beforehand. Avoid repeating for Gerrit case.
+  if not branchref:
+    branchref = rietveld_cl.branchref
+
+  # Only load Gerrit if it has associated issue.
+  gerrit_cl = GerritChangelist(branchref=branchref, issue=issue)
+  if gerrit_cl.GetIssue():
+    return gerrit_cl()

[Michael Achenbach, 2016/03/16 09:28:19]

Why call?

[tandrii(chromium), 2016/03/21 15:06:05]

bug. I wonder, how the hell did it work in the test. Fixed.

+
+  return rietveld_cl
+
+
+class ChangelistBase(object):
+  """Generic Changelist implementation, not tied to codereview.
+
+  Not safe for concurrent multi-{thread,process} use.
+  """
+
+  def __init__(self, branchref=None, issue=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()
-    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
     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
 
   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 ''
@@ skipping 52 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 126 matching lines @@
     return url
 
   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._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.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._FetchDescription()
       self.has_description = True
     if pretty:
       wrapper = textwrap.TextWrapper()
       wrapper.initial_indent = wrapper.subsequent_indent = '  '
       return wrapper.fill(self.description)
     return self.description

[Michael Achenbach, 2016/03/16 09:28:19]

Another case where the base class knows about the subclasses, i.e. it knows that
a description attribute was set.

[tandrii(chromium), 2016/03/21 15:06:05]

fixed.

 
   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()],
                         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."""
     if patchset:
       RunGit(['config', self._PatchsetSetting(), str(patchset)])
       self.patchset = patchset
     else:
       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."""
     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])
     else:
       current_issue = self.GetIssue()
       if current_issue:
@@ skipping 40 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._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._GetRieveldObjForPresubmit())
+    except presubmit_support.PresubmitFailure, e:
+      DieWithError(
+          ('%s\nMaybe your depot_tools is out of date?\n'
+           'If all fails, contact maruel@') % e)
+
+  # Codereview specific methods are implemented in respective child classes.
+
   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 sets description from the codereview server."""
+    raise NotImplementedError()
+
+  def _IssueSetting(self):
+    """Returns name of git config setting which stores issue number."""

[Michael Achenbach, 2016/03/16 09:28:19]

Not sure about this documentation and the same below. Maybe copy the same text
as used in the subclasses?

[tandrii(chromium), 2016/03/21 15:06:05]

Removed from subclasses and fixed here, to avoid confusion.

+    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):
+    raise NotImplementedError()
+
+  def CloseIssue(self):
+    """Updates the description and closes the issue."""
+    raise NotImplementedError()
+
+
+class RietveldChangelist(ChangelistBase):
+  def __init__(self, branchref=None, issue=None, auth_config=None):
+    super(RietveldChangelist, self).__init__(branchref, issue)
+    assert settings
+    settings.GetDefaultServerUrl()
+
+    self.rietveld_server = None
+    self._auth_config = auth_config
+    self._props = None
+    self._rpc_server = None
+
+  @property
+  def auth_config(self):
+    return self._auth_config
+
+  def GetCodereviewServer(self):
+    # TODO: update all users to use GetCodereviewServer instead.
+    return self.GetRietveldServer()
+
+  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 _FetchDescription(self):
+    """Fetches and sets description from the codereview server."""
+    issue = self.GetIssue()
+    assert issue
+    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 = ''
+
+  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():
@@ skipping 14 matching lines @@
     """Returns an upload.RpcServer() to access this review's rietveld instance.
     """
     if not self._rpc_server:
       self._rpc_server = rietveld.CachingRietveld(
           self.GetRietveldServer(),
           self._auth_config or auth.make_auth_config())
     return self._rpc_server
 
   def _IssueSetting(self):
     """Return the git setting that stores this change's issue."""
-    if settings.GetIsGerrit():
-      return 'branch.%s.gerritissue' % self.GetBranch()
     return 'branch.%s.rietveldissue' % self.GetBranch()
 
   def _PatchsetSetting(self):
     """Return the git setting that stores this change's most recent patchset."""
     return 'branch.%s.rietveldpatchset' % self.GetBranch()
 
   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 GerritChangelist(ChangelistBase):
+  def __init__(self, branchref=None, issue=None):
+    super(GerritChangelist, self).__init__(branchref, issue)
+    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._GerritServerSetting()
+        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()

[Michael Achenbach, 2016/03/16 09:28:19]

I maybe wouldn't call the CL a refactoring, as it also contains a substantial
amount of new code and new features like this.

[tandrii(chromium), 2016/03/21 15:06:05]

Good eye :) Yeah, this bit is new, indeed. And so is the hook's rietveld
replacement. So, I've renamed it to rework. Is that better?

+
+  def _GerritServerSetting(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()
+
 
 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 700 matching lines @@
   auth.add_auth_options(parser)
   (options, args) = parser.parse_args(args)
   auth_config = auth.extract_auth_config_from_options(options)
 
   if git_common.is_dirty_git_tree('upload'):
     return 1
 
   options.reviewers = cleanup_list(options.reviewers)
   options.cc = cleanup_list(options.cc)
 
-  cl = Changelist(auth_config=auth_config)
+  if settings.GetIsGerrit():
+    # TODO(tandrii): improve based on (repo, branch, options).
+    cl = GerritChangelist()

[Michael Achenbach, 2016/03/16 09:28:19]

I assume that's for the issue==None case? Or could somebody change the settings
while already having an issue and then call upload again?

Maybe consider adding this logic to the Changelist factory method and going
through that one instead.

[tandrii(chromium), 2016/03/21 15:06:05]

I have exactly a use case for the latter, though it's complicated enough that I
might as well give up on it. For now this is as it is to stay closer to prev.
behavior.
I considered doing exactly that, but it'd would complicate *this* CL too much,
and I don't want to revert-reland it many times :)
I'm thus postponing this decision for later.

[Michael Achenbach, 2016/03/24 09:58:11]

Acknowledged.

+  else:
+    cl = RietveldChangelist(auth_config=auth_config)
   if args:
     # TODO(ukai): is it ok for gerrit case?
     base_branch = args[0]
   else:
     if cl.GetBranch() is None:
       DieWithError('Can\'t upload from detached HEAD state. Get on a branch!')
 
     # Default to diffing against common ancestor of upstream branch
     base_branch = cl.GetCommonAncestorWithUpstream()
     args = [base_branch, 'HEAD']
@@ skipping 651 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 = RietveldChangelist(auth_config=auth_config)

[Michael Achenbach, 2016/03/16 09:28:19]

Does that deserve a TODO? Or is it a rietveld-only feature?

[tandrii(chromium), 2016/03/21 15:06:05]

For now, it's sooo Rietveld-specific, that TODO at this line is pointless. The
whole "git cl patch" would need Gerrit implementation from scratch = it's more
than TODO.

[Michael Achenbach, 2016/03/24 09:58:11]

Acknowledged.

     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)