git-cl: Simplify FetchDescription and UpdateDescription

- Merge GetDescription and FetchDescription, and remove unused `force` argument.
- Merge UpdateDescription and UpdateDescriptionRemote.

Bug: 1042324
Change-Id: Ia7a1b0aa1ea12a95bb6a19d9d3c9dd6aeb5bd2b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2039968
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Anthony Polito <apolito@google.com>

git_cl.py

@@ -1389,18 +1389,23 @@ class Changelist(object):
     args = ['log', '--pretty=format:%s%n%n%b', '%s...' % (upstream_branch)]
     return RunGitWithCode(args)[1].strip()
 
-  def GetDescription(self, pretty=False, force=False):
-    if not self.has_description or force:
-      if self.GetIssue():
-        self.description = self.FetchDescription(force=force)
+  def FetchDescription(self, pretty=False):
+    assert self.GetIssue(), 'issue is required to query Gerrit'
+
+    if not self.has_description:
+      data = self._GetChangeDetail(['CURRENT_REVISION', 'CURRENT_COMMIT'])
+      current_rev = data['current_revision']
+      self.description = data['revisions'][current_rev]['commit']['message']
       self.has_description = True
-    if pretty:
-      # Set width to 72 columns + 2 space indent.
-      wrapper = textwrap.TextWrapper(width=74, replace_whitespace=True)
-      wrapper.initial_indent = wrapper.subsequent_indent = '  '
-      lines = self.description.splitlines()
-      return '\n'.join([wrapper.fill(line) for line in lines])
-    return self.description
+
+    if not pretty:
+      return self.description
+
+    # Set width to 72 columns + 2 space indent.
+    wrapper = textwrap.TextWrapper(width=74, replace_whitespace=True)
+    wrapper.initial_indent = wrapper.subsequent_indent = '  '
+    lines = self.description.splitlines()
+    return '\n'.join([wrapper.fill(line) for line in lines])
 
   def GetPatchset(self):
     """Returns the patchset number as a int or None if not set."""
@@ -1474,7 +1479,7 @@ class Changelist(object):
     issue = self.GetIssue()
     patchset = self.GetPatchset()
     if issue:
-      description = self.GetDescription()
+      description = self.FetchDescription()
     else:
       # If the change was never uploaded, use the log messages of all commits
       # up to the branch point, as git cl upload will prefill the description
@@ -1493,7 +1498,22 @@ class Changelist(object):
         upstream=upstream_branch)
 
   def UpdateDescription(self, description, force=False):
-    self.UpdateDescriptionRemote(description, force=force)
+    assert self.GetIssue(), 'issue is required to update description'
+
+    if gerrit_util.HasPendingChangeEdit(
+        self._GetGerritHost(), self._GerritChangeIdentifier()):
+      if not force:
+        confirm_or_exit(
+            'The description cannot be modified while the issue has a pending '
+            'unpublished edit. Either publish the edit in the Gerrit web UI '
+            'or delete it.\n\n', action='delete the unpublished edit')
+
+      gerrit_util.DeletePendingChangeEdit(
+        self._GetGerritHost(), self._GerritChangeIdentifier())
+    gerrit_util.SetCommitMessage(
+        self._GetGerritHost(), self._GerritChangeIdentifier(),
+        description, notify='NONE')
+
     self.description = description
     self.has_description = True
 
@@ -1873,32 +1893,14 @@ class Changelist(object):
     return 'unsent'
 
   def GetMostRecentPatchset(self):
+    if not self.GetIssue():
+      return None
+
     data = self._GetChangeDetail(['CURRENT_REVISION'])
     patchset = data['revisions'][data['current_revision']]['_number']
     self.SetPatchset(patchset)
     return patchset
 
-  def FetchDescription(self, force=False):
-    data = self._GetChangeDetail(['CURRENT_REVISION', 'CURRENT_COMMIT'],
-                                 no_cache=force)
-    current_rev = data['current_revision']
-    return data['revisions'][current_rev]['commit']['message']
-
-  def UpdateDescriptionRemote(self, description, force=False):
-    if gerrit_util.HasPendingChangeEdit(
-        self._GetGerritHost(), self._GerritChangeIdentifier()):
-      if not force:
-        confirm_or_exit(
-            'The description cannot be modified while the issue has a pending '
-            'unpublished edit. Either publish the edit in the Gerrit web UI '
-            'or delete it.\n\n', action='delete the unpublished edit')
-
-      gerrit_util.DeletePendingChangeEdit(
-        self._GetGerritHost(), self._GerritChangeIdentifier())
-    gerrit_util.SetCommitMessage(
-        self._GetGerritHost(), self._GerritChangeIdentifier(),
-        description, notify='NONE')
-
   def AddComment(self, message, publish=None):
     gerrit_util.SetReview(
         self._GetGerritHost(), self._GerritChangeIdentifier(),
@@ -2357,7 +2359,7 @@ class Changelist(object):
       self._GerritCommitMsgHookCheck(offer_removal=not options.force)
       if self.GetIssue():
         # Try to get the message from a previous upload.
-        message = self.GetDescription()
+        message = self.FetchDescription()
         if not message:
           DieWithError(
               'failed to fetch description from current Gerrit change %d\n'
@@ -3818,12 +3820,13 @@ def CMDstatus(parser, args):
     parser.error('Unsupported args: %s' % args)
 
   if options.issue is not None and not options.field:
-    parser.error('--field must be specified with --issue.')
+    parser.error('--field must be given when --issue is set.')
 
   if options.field:
     cl = Changelist(issue=options.issue)
     if options.field.startswith('desc'):
-      print(cl.GetDescription())
+      if cl.GetIssue():
+        print(cl.FetchDescription())
     elif options.field == 'id':
       issueid = cl.GetIssue()
       if issueid:
@@ -3910,7 +3913,7 @@ def CMDstatus(parser, args):
   print('Issue number: %s (%s)' % (cl.GetIssue(), cl.GetIssueURL()))
   if not options.fast:
     print('Issue description:')
-    print(cl.GetDescription(pretty=True))
+    print(cl.FetchDescription(pretty=True))
   return 0
 
 
@@ -4098,7 +4101,7 @@ def CMDdescription(parser, args):
   if args and not args[0].isdigit():
     logging.info('canonical issue/change URL: %s\n', cl.GetIssueURL())
 
-  description = ChangeDescription(cl.GetDescription())
+  description = ChangeDescription(cl.FetchDescription())
 
   if options.display:
     print(description.description)
@@ -4115,7 +4118,7 @@ def CMDdescription(parser, args):
     description.set_description(text)
   else:
     description.prompt()
-  if cl.GetDescription().strip() != description.description:
+  if cl.FetchDescription().strip() != description.description:
     cl.UpdateDescription(description.description, force=options.force)
   return 0
 

tests/git_cl_test.py

@@ -83,7 +83,7 @@ class ChangelistMock(object):
     pass
   def GetIssue(self):
     return 1
-  def GetDescription(self, force=False):
+  def FetchDescription(self):
     return ChangelistMock.desc
   def UpdateDescription(self, desc, force=False):
     ChangelistMock.desc = desc
@@ -169,11 +169,11 @@ class SystemExitMock(Exception):
 
 
 class TestGitClBasic(unittest.TestCase):
-  def test_get_description(self):
+  def test_fetch_description(self):
     cl = git_cl.Changelist(issue=1, codereview_host='host')
     cl.description = 'x'
     cl.has_description = True
-    self.assertEqual(cl.GetDescription(), 'x')
+    self.assertEqual(cl.FetchDescription(), 'x')
 
   def test_set_preserve_tryjobs(self):
     d = git_cl.ChangeDescription('Simple.')
@@ -2150,7 +2150,8 @@ class TestGitCl(TestCase):
       self.assertEqual(git_cl.main(['status', '--issue', '1']), 0)
     except SystemExit as ex:
       self.assertEqual(ex.code, 2)
-      self.assertRegexpMatches(out.getvalue(), r'--field must be specified')
+      self.assertIn(
+          '--field must be given when --issue is set.', out.getvalue())
 
     out = StringIO()
     self.mock(git_cl.sys, 'stderr', out)
@@ -2159,7 +2160,8 @@ class TestGitCl(TestCase):
       self.assertEqual(git_cl.main(['status', '--issue', '1']), 0)
     except SystemExit as ex:
       self.assertEqual(ex.code, 2)
-      self.assertRegexpMatches(out.getvalue(), r'--field must be specified')
+      self.assertIn(
+          '--field must be given when --issue is set.', out.getvalue())
 
   def test_StatusFieldOverrideIssue(self):
     out = StringIO()
@@ -2169,7 +2171,7 @@ class TestGitCl(TestCase):
       self.assertEqual(cl_self.issue, 1)
       return 'foobar'
 
-    self.mock(git_cl.Changelist, 'GetDescription', assertIssue)
+    self.mock(git_cl.Changelist, 'FetchDescription', assertIssue)
     self.assertEqual(
       git_cl.main(['status', '--issue', '1', '--field', 'desc']),
       0)
@@ -2181,7 +2183,7 @@ class TestGitCl(TestCase):
       self.assertEqual(cl_self.issue, 1)
       return 'foobar'
 
-    self.mock(git_cl.Changelist, 'GetDescription', assertIssue)
+    self.mock(git_cl.Changelist, 'FetchDescription', assertIssue)
     self.mock(git_cl.Changelist, 'CloseIssue', lambda *_: None)
     self.assertEqual(
       git_cl.main(['set-close', '--issue', '1']), 0)
@@ -2235,14 +2237,14 @@ class TestGitCl(TestCase):
       # Simulate user changing something.
       return 'Some.\n\nChange-Id: xxx\nBug: 123'
 
-    def UpdateDescriptionRemote(_, desc, force=False):
+    def UpdateDescription(_, desc, force=False):
       self.assertEqual(desc, 'Some.\n\nChange-Id: xxx\nBug: 123')
 
     self.mock(git_cl.sys, 'stdout', StringIO())
-    self.mock(git_cl.Changelist, 'GetDescription',
+    self.mock(git_cl.Changelist, 'FetchDescription',
               lambda *args: current_desc)
-    self.mock(git_cl.Changelist, 'UpdateDescriptionRemote',
-              UpdateDescriptionRemote)
+    self.mock(git_cl.Changelist, 'UpdateDescription',
+              UpdateDescription)
     self.mock(git_cl.gclient_utils, 'RunEditor', RunEditor)
 
     self.calls = [
@@ -2269,7 +2271,7 @@ class TestGitCl(TestCase):
       return desc
 
     self.mock(git_cl.sys, 'stdout', StringIO())
-    self.mock(git_cl.Changelist, 'GetDescription',
+    self.mock(git_cl.Changelist, 'FetchDescription',
               lambda *args: current_desc)
     self.mock(git_cl.gclient_utils, 'RunEditor', RunEditor)
 
@@ -2428,7 +2430,7 @@ class TestGitCl(TestCase):
   def test_cmd_issue_erase_existing_with_change_id(self):
     out = StringIO()
     self.mock(git_cl.sys, 'stdout', out)
-    self.mock(git_cl.Changelist, 'GetDescription',
+    self.mock(git_cl.Changelist, 'FetchDescription',
               lambda _: 'This is a description\n\nChange-Id: Ideadbeef')
     self.calls = [
         ((['git', 'symbolic-ref', 'HEAD'],), 'feature'),
@@ -2594,18 +2596,14 @@ class TestGitCl(TestCase):
         (('GetChangeDetail', 'host', 'my%2Frepo~1',
           ['CURRENT_REVISION', 'CURRENT_COMMIT']),
          gen_detail('rev1', 'desc1')),
-        (('GetChangeDetail', 'host', 'my%2Frepo~1',
-          ['CURRENT_REVISION', 'CURRENT_COMMIT']),
-         gen_detail('rev2', 'desc2')),
     ]
 
     self._mock_gerrit_changes_for_detail_cache()
     cl = git_cl.Changelist(issue=1)
     cl._cached_remote_url = (
         True, 'https://chromium.googlesource.com/a/my/repo.git/')
-    self.assertEqual(cl.GetDescription(), 'desc1')
-    self.assertEqual(cl.GetDescription(), 'desc1')  # cache hit.
-    self.assertEqual(cl.GetDescription(force=True), 'desc2')
+    self.assertEqual(cl.FetchDescription(), 'desc1')
+    self.assertEqual(cl.FetchDescription(), 'desc1')  # cache hit.
 
   def test_print_current_creds(self):
     class CookiesAuthenticatorMock(object):