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

git_cl.py

Index: git_cl.py
diff --git a/git_cl.py b/git_cl.py
index 245b05fb95477466c69023ddec5b3e2e431e6d9a..1af0f78ba70c46f17914449a33b5f2f6eb5c73ff 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -5,7 +5,7 @@
 
 # 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
@@ -156,7 +156,7 @@ def ask_for_data(prompt):
 
 
 def git_set_branch_value(key, value):
-  branch = Changelist().GetBranch()
+  branch = GetCurrentBranch()
   if not branch:
     return
 
@@ -168,7 +168,7 @@ def git_set_branch_value(key, 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])
@@ -813,23 +813,73 @@ class Settings(object):
 
 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
@@ -839,14 +889,7 @@ class Changelist(object):
     self.patchset = None
     self.cc = None
     self.watchers = ()
-    self._auth_config = auth_config
-    self._props = None
     self._remote = None
-    self._rpc_server = None
-
-  @property
-  def auth_config(self):
-    return self._auth_config
 
   def GetCCList(self):
     """Return the users cc'd on this CL.
@@ -919,11 +962,11 @@ class Changelist(object):
             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
 
@@ -1070,60 +1113,17 @@ or verify this branch is set up to track another (via the --track argument to
       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()
@@ -1150,28 +1150,6 @@ or verify this branch is set up to track another (via the --track argument to
              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:
@@ -1232,6 +1210,141 @@ or verify this branch is set up to track another (via the --track argument to
         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.
@@ -1279,21 +1392,7 @@ or verify this branch is set up to track another (via the --track argument to
       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)
 
@@ -1328,8 +1427,6 @@ or verify this branch is set up to track another (via the --track argument to
 
   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):
@@ -1343,6 +1440,60 @@ or verify this branch is set up to track another (via the --track argument to
       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."""
@@ -1888,6 +2039,7 @@ def CMDstatus(parser, args):
   changes = (
       Changelist(branchref=b, auth_config=auth_config)
       for b in branches.splitlines())
+  # TODO(tandrii): refactor to use CLs list instead of branches list.
   branches = [c.GetBranch() for c in changes]
   alignment = max(5, max(len(b) for b in branches))
   print 'Branches associated with reviews:'
@@ -2608,7 +2760,11 @@ def CMDupload(parser, args):
   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]
@@ -3280,7 +3436,7 @@ def PatchIssue(issue_arg, reject, nocommit, directory, auth_config):
                              'patch from issue %(i)s at patchset '
                              '%(p)s (http://crrev.com/%(i)s#ps%(p)s)'
                              % {'i': issue, 'p': patchset})])
-    cl = Changelist(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."