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

Revert of git cl: Rework Changelist class for Rietveld/Gerrit use. (patchset #8 id:380001 of https://codereview.chromium.org/1805193002/ )

Reason for revert:
Broke one project presubmit.

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

TBR=phajdan.jr@chromium.org,andybons@chromium.org,machenbach@chromium.org,sergiyb@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=579160, 597638
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299476

[tandrii(chromium), 2016-03-24 17:19:36]

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

[commit-bot: I haz the power, 2016-03-24 17:19:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1831813003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1831813003/1

[commit-bot: I haz the power, 2016-03-24 17:19:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-24 17:19:48]

Failed to apply patch for git_cl.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file git_cl.py
  Hunk #6 FAILED at 839.
  Hunk #7 succeeded at 899 (offset -5 lines).
  Hunk #8 succeeded at 944 (offset -5 lines).
  Hunk #9 FAILED at 1092.
  Hunk #10 succeeded at 1118 (offset -4 lines).
  Hunk #11 FAILED at 1130.
  Hunk #12 FAILED at 1202.
  Hunk #13 succeeded at 1425 (offset -2 lines).
  Hunk #14 FAILED at 1470.
  Hunk #15 succeeded at 2116 (offset -3 lines).
  Hunk #16 succeeded at 2231 (offset -3 lines).
  Hunk #17 succeeded at 2610 (offset -3 lines).
  Hunk #18 succeeded at 2854 (offset -3 lines).
  Hunk #19 succeeded at 3450 (offset -3 lines).
  Hunk #20 succeeded at 3464 (offset -3 lines).
  Hunk #21 succeeded at 3509 (offset -3 lines).
  5 out of 21 hunks FAILED -- saving rejects to file git_cl.py.rej

Patch:       git_cl.py
Index: git_cl.py
diff --git a/git_cl.py b/git_cl.py
index
98a2f01b44485b746fb82c3ad290d5f379e3eab7..59e412123fcd73404fddc29c1e6fd0245c66440c
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 and Gerrit."""
+"""A git-command for integrating reviews on Rietveld."""
 
 from distutils.version import LooseVersion
 from multiprocessing.pool import ThreadPool
@@ -47,7 +47,6 @@
 import dart_format
 import fix_encoding
 import gclient_utils
-import gerrit_util
 import git_cache
 import git_common
 import git_footers
@@ -157,7 +156,7 @@
 
 
 def git_set_branch_value(key, value):
-  branch = GetCurrentBranch()
+  branch = Changelist().GetBranch()
   if not branch:
     return
 
@@ -169,7 +168,7 @@
 
 
 def git_get_branch_default(key, default):
-  branch = GetCurrentBranch()
+  branch = Changelist().GetBranch()
   if branch:
     git_key = 'branch.%s.%s' % (branch, key)
     (_, stdout) = RunGitWithCode(['config', '--int', '--get', git_key])
@@ -814,61 +813,23 @@
 
 def ShortBranchName(branch):
   """Convert a name like 'refs/heads/foo' to just 'foo'."""
-  return branch.replace('refs/heads/', '', 1)
-
-
-def GetCurrentBranchRef():
-  """Returns branch ref (e.g., refs/heads/master) or None."""
-  return RunGit(['symbolic-ref', 'HEAD'],
-                stderr=subprocess2.VOID, error_ok=True).strip() or None
-
-
-def GetCurrentBranch():
-  """Returns current branch or None.
-
-  For refs/heads/* branches, returns just last part. For others, full ref.
-  """
-  branchref = GetCurrentBranchRef()
-  if branchref:
-    return ShortBranchName(branchref)
-  return None
+  return branch.replace('refs/heads/', '')
 
 
 class Changelist(object):
-  """Changelist works with one changelist in local branch.
-
-  Supports two codereview backends: Rietveld or Gerrit, selected at object
-  creation.
-
-  Not safe for concurrent multi-{thread,process} use.
-  """
-
-  def __init__(self, branchref=None, issue=None, codereview=None, **kwargs):
-    """Create a new ChangeList instance.
-
-    If issue is given, the codereview must be given too.
-
-    If `codereview` is given, it must be 'rietveld' or 'gerrit'.
-    Otherwise, it's decided based on current configuration of the local branch,
-    with default being 'rietveld' for backwards compatibility.
-    See _load_codereview_impl for more details.
-
-    **kwargs will be passed directly to codereview implementation.
-    """
+  def __init__(self, branchref=None, issue=None, auth_config=None):
     # Poke settings so we get the "configure your server" message if necessary.
     global settings
     if not settings:
       # Happens when git_cl.py is used as a utility library.
       settings = Settings()
-
-    if issue:
-      assert codereview, 'codereview must be known, if issue is known'
-
+    settings.GetDefaultServerUrl()
     self.branchref = branchref
     if self.branchref:
       self.branch = ShortBranchName(self.branchref)
     else:
       self.branch = None
+    self.rietveld_server = None
     self.upstream_branch = None
     self.lookedup_issue = False
     self.issue = issue or None
@@ -878,44 +839,14 @@
     self.patchset = None
     self.cc = None
     self.watchers = ()
+    self._auth_config = auth_config
+    self._props = None
     self._remote = None
-
-    self._codereview_impl = None
-    self._load_codereview_impl(codereview, **kwargs)
-
-  def _load_codereview_impl(self, codereview=None, **kwargs):
-    if codereview:
-      codereview = codereview.lower()
-      if codereview == 'gerrit':
-        self._codereview_impl = _GerritChangelistImpl(self, **kwargs)
-      elif codereview == 'rietveld':
-        self._codereview_impl = _RietveldChangelistImpl(self, **kwargs)
-      else:
-        assert codereview in ('rietveld', 'gerrit')
-      return
-
-    # Automatic selection.
-    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, **kwargs)
-    if self.GetIssue(force_lookup=True):
-      return
-
-    # OK, no issue is set for this branch.
-    # If Gerrit is set repo-wide => Gerrit.
-    if settings.GetIsGerrit():
-      return
-
-    self._codereview_impl = tmp_rietveld
-    return
-
+    self._rpc_server = None
+
+  @property
+  def auth_config(self):
+    return self._auth_config
 
   def GetCCList(self):
     """Return the users cc'd on this CL.
@@ -943,7 +874,8 @@
   def GetBranch(self):
     """Returns the short branch name, e.g. 'master'."""
     if not self.branch:
-      branchref = GetCurrentBranchRef()
+      branchref = RunGit(['symbolic-ref', 'HEAD'],
+                         stderr=subprocess2.VOID, error_ok=True).strip()
       if not branchref:
         return None
       self.branchref = branchref
@@ -987,12 +919,11 @@
             remote = 'origin'
             upstream_branch = 'refs/heads/trunk'
           else:
-            DieWithError(
-               'Unable to determine default branch to diff against.\n'
-               'Either pass complete "git diff"-style arguments, like\n'
-               '  git cl upload origin/master\n'
-               'or verify this branch is set up to track another \n'
-               '(via the --track argument to "git checkout -b ...").')
+            DieWithError("""Unable to determine default branch to diff against.
+Either pass complete "git diff"-style arguments, like
+  git cl upload origin/master
+or verify this branch is set up to track another (via the --track argument to
+"git checkout -b ...").""")
 
     return remote, upstream_branch
 
@@ -1131,26 +1062,68 @@
                    cwd=url).strip()
     return url
 
-  def GetIssue(self, force_lookup=False):
+  def GetIssue(self):
     """Returns the issue number as a int or None if not set."""
-    if force_lookup or (self.issue is None and not self.lookedup_issue):
-      issue = RunGit(['config', self._codereview_impl.IssueSetting()],
-                     error_ok=True).strip()
+    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."""
-    issue = self.GetIssue()
-    if not issue:
+    if not self.GetIssue():
       return None
-    return '%s/%s' % (self._codereview_impl.GetCodereviewServer(), issue)
+    if settings.GetIsGerrit():
+      return '%s/%s' % (self.GetGerritServer(), self.GetIssue())
+    return '%s/%s' % (self.GetRietveldServer(), self.GetIssue())
 
   def GetDescription(self, pretty=False):
     if not self.has_description:
       if self.GetIssue():
-        self.description = self._codereview_impl.FetchDescription()
+        issue = self.GetIssue()
+        try:
+          self.description = self.RpcServer().get_description(issue).strip()
+        except urllib2.HTTPError as e:
+          if e.code == 404:
+            DieWithError(
+                ('\nWhile fetching the description for issue %d, received a '
+                 '404 (not found)\n'
+                 'error. It is likely that you deleted this '
+                 'issue on the server. If this is the\n'
+                 'case, please run\n\n'
+                 '    git cl issue 0\n\n'
+                 'to clear the association with the deleted issue. Then run '
+                 'this command again.') % issue)
+          else:
+            DieWithError(
+                '\nFailed to fetch issue description. HTTP error %d' % e.code)
+        except urllib2.URLError as e:
+          print >> sys.stderr, (
+              'Warning: Failed to retrieve CL description due to network '
+              'failure.')
+          self.description = ''
+
       self.has_description = True
  …
(message too large)

[tandrii(chromium), 2016-03-24 17:33:22]

Description was changed from

==========
Revert of git cl: Rework Changelist class for Rietveld/Gerrit use. (patchset #8
id:380001 of https://codereview.chromium.org/1805193002/ )

Reason for revert:
Broke one project presubmit.

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

TBR=phajdan.jr@chromium.org,andybons@chromium.org,machenbach@chromium.org,ser...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=579160
==========

to

==========
Revert of git cl: Rework Changelist class for Rietveld/Gerrit use. (patchset #8
id:380001 of https://codereview.chromium.org/1805193002/ )

Reason for revert:
Broke one project presubmit.

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

TBR=phajdan.jr@chromium.org,andybons@chromium.org,machenbach@chromium.org,ser...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=579160,597638
==========

[tandrii(chromium), 2016-03-24 17:33:27]

The CQ bit was checked by tandrii@chromium.org

[commit-bot: I haz the power, 2016-03-24 17:33:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1831813003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1831813003/1

[commit-bot: I haz the power, 2016-03-24 17:33:34]

Description was changed from

==========
Revert of git cl: Rework Changelist class for Rietveld/Gerrit use. (patchset #8
id:380001 of https://codereview.chromium.org/1805193002/ )

Reason for revert:
Broke one project presubmit.

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

TBR=phajdan.jr@chromium.org,andybons@chromium.org,machenbach@chromium.org,ser...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=579160,597638
==========

to

==========
Revert of git cl: Rework Changelist class for Rietveld/Gerrit use. (patchset #8
id:380001 of https://codereview.chromium.org/1805193002/ )

Reason for revert:
Broke one project presubmit.

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

TBR=phajdan.jr@chromium.org,andybons@chromium.org,machenbach@chromium.org,ser...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=579160,597638

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299476
==========

[commit-bot: I haz the power, 2016-03-24 17:33:34]

Committed patchset #1 (id:1) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=299476

[Sergiy Byelozyorov, 2016-03-24 17:49:06]

lgtm