git-cl: Use _create_description_from_log instead of GetLocalDescription

Also refactor code to eliminate multiple calls to _create_description_from_log.

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

git_cl.py

@@ -966,7 +966,7 @@ def _create_description_from_log(args):
     log_args = [args[0] + '..' + args[1]]
   else:
     log_args = args[:]  # Hope for the best!
-  return RunGit(['log', '--pretty=format:%s\n\n%b'] + log_args)
+  return RunGit(['log', '--pretty=format:%s%n%n%b'] + log_args)
 
 
 class GerritChangeNotExists(Exception):
@@ -1016,7 +1016,6 @@ class Changelist(object):
     self.lookedup_issue = False
     self.issue = issue or None
     self.description = None
-    self.local_description = None
     self.lookedup_patchset = False
     self.patchset = None
     self.cc = None
@@ -1266,13 +1265,6 @@ class Changelist(object):
       return None
     return '%s/%s' % (self.GetCodereviewServer(), issue)
 
-  def GetLocalDescription(self, upstream_branch):
-    """Return the log messages of all commits up to the branch point."""
-    if self.local_description is None:
-      args = ['log', '--pretty=format:%s%n%n%b', '%s...' % (upstream_branch)]
-      self.local_description = RunGitWithCode(args)[1].strip()
-    return self.local_description
-
   def FetchDescription(self, pretty=False):
     assert self.GetIssue(), 'issue is required to query Gerrit'
 
@@ -1348,7 +1340,7 @@ class Changelist(object):
       self.issue = None
       self.patchset = None
 
-  def GetChange(self, upstream_branch):
+  def GetChange(self, upstream_branch, description):
     if not self.GitSanityChecks(upstream_branch):
       DieWithError('\nGit sanity check failure')
 
@@ -1368,13 +1360,6 @@ class Changelist(object):
 
     issue = self.GetIssue()
     patchset = self.GetPatchset()
-    if issue:
-      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
-      # with these log messages.
-      description = self.GetLocalDescription(upstream_branch)
 
     author = self.GetAuthor()
     return presubmit_support.GitChange(
@@ -1493,17 +1478,23 @@ class Changelist(object):
     self.EnsureAuthenticated(force=options.force)
     self.EnsureCanUploadPatchset(force=options.force)
 
+    # Get description message for upload.
+    if self.GetIssue():
+      description = self.FetchDescription()
+    elif options.message:
+      description = options.message
+    else:
+      description = _create_description_from_log(git_diff_args)
+      if options.title and options.squash:
+        description = options.title + '\n\n' + message
+
     # Apply watchlists on upload.
-    change = self.GetChange(base_branch)
+    change = self.GetChange(base_branch, description)
     watchlist = watchlists.Watchlists(change.RepositoryRoot())
     files = [f.LocalPath() for f in change.AffectedFiles()]
     if not options.bypass_watchlists:
       self.ExtendCC(watchlist.GetWatchersForPaths(files))
 
-    if self.GetIssue():
-      description = self.FetchDescription()
-    else:
-      description = self.GetLocalDescription(base_branch)
     if options.reviewers or options.tbrs or options.add_owners_to:
       # Set the reviewer list now so that presubmit checks can access it.
       change_description = ChangeDescription(description)
@@ -1524,7 +1515,8 @@ class Changelist(object):
       self.ExtendCC(hook_results['more_cc'])
 
     print_stats(git_diff_args)
-    ret = self.CMDUploadChange(options, git_diff_args, custom_cl_base, change)
+    ret = self.CMDUploadChange(
+        options, git_diff_args, custom_cl_base, change, description)
     if not ret:
       self._GitSetBranchConfigValue(
           'last-upload-hash', RunGit(['rev-parse', 'HEAD']).strip())
@@ -2021,7 +2013,7 @@ class Changelist(object):
       if self.GetIssue():
         description = self.FetchDescription()
       else:
-        description = self.GetLocalDescription(upstream)
+        description = _create_description_from_log([upstream])
       self.RunHook(
           committing=True,
           may_prompt=not force,
@@ -2258,7 +2250,8 @@ class Changelist(object):
 
     return push_stdout
 
-  def CMDUploadChange(self, options, git_diff_args, custom_cl_base, change):
+  def CMDUploadChange(
+      self, options, git_diff_args, custom_cl_base, change, message):
     """Upload the current branch to Gerrit."""
     if options.squash is None:
       # Load default for user, repo, squash=true, in this order.
@@ -2283,12 +2276,6 @@ class Changelist(object):
     if options.squash:
       self._GerritCommitMsgHookCheck(offer_removal=not options.force)
       if self.GetIssue():
-        # Try to get the message from a previous upload.
-        message = self.FetchDescription()
-        if not message:
-          DieWithError(
-              'failed to fetch description from current Gerrit change %d\n'
-              '%s' % (self.GetIssue(), self.GetIssueURL()))
         if not title:
           if options.message:
             # When uploading a subsequent patchset, -m|--message is taken
@@ -2347,12 +2334,6 @@ class Changelist(object):
         assert [change_id] == git_footers.get_footer_change_id(message)
         change_desc = ChangeDescription(message, bug=bug, fixed=fixed)
       else:  # if not self.GetIssue()
-        if options.message:
-          message = options.message
-        else:
-          message = _create_description_from_log(git_diff_args)
-          if options.title:
-            message = options.title + '\n\n' + message
         change_desc = ChangeDescription(message, bug=bug, fixed=fixed)
         if not options.force:
           change_desc.prompt()
@@ -2389,15 +2370,14 @@ class Changelist(object):
         ref_to_push = RunGit(
             ['commit-tree', tree, '-p', parent, '-F', desc_tempfile]).strip()
     else:  # if not options.squash
-      change_desc = ChangeDescription(
-          options.message or _create_description_from_log(git_diff_args))
+      change_desc = ChangeDescription(message)
       if not change_desc.description:
         DieWithError("Description is empty. Aborting...")
 
       if not git_footers.get_footer_change_id(change_desc.description):
         DownloadGerritHook(False)
         change_desc.set_description(
-            self._AddChangeIdToCommitMessage(options, git_diff_args))
+            self._AddChangeIdToCommitMessage(message, git_diff_args))
       if options.reviewers or options.tbrs or options.add_owners_to:
         change_desc.update_reviewers(options.reviewers, options.tbrs,
                                      options.add_owners_to, change)
@@ -2609,13 +2589,11 @@ class Changelist(object):
           change_desc)
     return parent
 
-  def _AddChangeIdToCommitMessage(self, options, args):
+  def _AddChangeIdToCommitMessage(self, log_desc, args):
     """Re-commits using the current message, assumes the commit hook is in
     place.
     """
-    log_desc = options.message or _create_description_from_log(args)
-    git_command = ['commit', '--amend', '-m', log_desc]
-    RunGit(git_command)
+    RunGit(['commit', '--amend', '-m', log_desc])
     new_log_desc = _create_description_from_log(args)
     if git_footers.get_footer_change_id(new_log_desc):
       print('git-cl: Added Change-Id to commit message.')
@@ -4038,7 +4016,7 @@ def CMDdescription(parser, args):
       text = '\n'.join(l.rstrip() for l in sys.stdin)
     elif text == '+':
       base_branch = cl.GetCommonAncestorWithUpstream()
-      text = cl.GetLocalDescription(base_branch)
+      text = _create_description_from_log([base_branch])
 
     description.set_description(text)
   else:
@@ -4070,7 +4048,7 @@ def CMDlint(parser, args):
   os.chdir(settings.GetRoot())
   try:
     cl = Changelist()
-    change = cl.GetChange(cl.GetCommonAncestorWithUpstream())
+    change = cl.GetChange(cl.GetCommonAncestorWithUpstream(), '')
     files = [f.LocalPath() for f in change.AffectedFiles()]
     if not files:
       print('Cannot lint an empty CL')
@@ -4130,7 +4108,7 @@ def CMDpresubmit(parser, args):
   if cl.GetIssue():
     description = cl.FetchDescription()
   else:
-    description = cl.GetLocalDescription(base_branch)
+    description = _create_description_from_log([base_branch])
 
   cl.RunHook(
       committing=not options.upload,
@@ -4965,7 +4943,7 @@ def CMDowners(parser, args):
     # Default to diffing against the common ancestor of the upstream branch.
     base_branch = cl.GetCommonAncestorWithUpstream()
 
-  change = cl.GetChange(base_branch)
+  change = cl.GetChange(base_branch, '')
   affected_files = [f.LocalPath() for f in change.AffectedFiles()]
 
   if options.batch:

split_cl.py

@@ -194,7 +194,7 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run,
     EnsureInGitRepository()
 
     cl = changelist()
-    change = cl.GetChange(cl.GetCommonAncestorWithUpstream())
+    change = cl.GetChange(cl.GetCommonAncestorWithUpstream(), '')
     files = change.AffectedFiles()
 
     if not files:

tests/git_cl_test.py

@@ -852,8 +852,6 @@ class TestGitCl(unittest.TestCase):
     if post_amend_description is None:
       post_amend_description = description
     cc = cc or []
-    # Determined in `_gerrit_base_calls`.
-    determined_ancestor_revision = custom_cl_base or 'fake_ancestor_sha'
 
     calls = []
 
@@ -863,12 +861,6 @@ class TestGitCl(unittest.TestCase):
 
     # If issue is given, then description is fetched from Gerrit instead.
     if issue is None:
-      calls += [
-        ((['git', 'log', '--pretty=format:%s\n\n%b',
-           ((custom_cl_base + '..') if custom_cl_base else
-            'fake_ancestor_sha..HEAD')],),
-         description),
-      ]
       if squash:
         title = 'Initial_upload'
     else:
@@ -881,14 +873,6 @@ class TestGitCl(unittest.TestCase):
     if not git_footers.get_footer_change_id(description) and not squash:
       calls += [
         (('DownloadGerritHook', False), ''),
-        # Amending of commit message to get the Change-Id.
-        ((['git', 'log', '--pretty=format:%s\n\n%b',
-           determined_ancestor_revision + '..HEAD'],),
-         description),
-        ((['git', 'commit', '--amend', '-m', description],), ''),
-        ((['git', 'log', '--pretty=format:%s\n\n%b',
-           determined_ancestor_revision + '..HEAD'],),
-         post_amend_description)
       ]
     if squash:
       if force or not issue:
@@ -1192,7 +1176,10 @@ class TestGitCl(unittest.TestCase):
               lambda path: self._mocked_call(['os.path.isfile', path])).start()
     mock.patch('git_cl.Changelist.GitSanityChecks', return_value=True).start()
     mock.patch(
-        'git_cl.Changelist.GetLocalDescription', return_value='foo').start()
+        'git_cl._create_description_from_log', return_value=description).start()
+    mock.patch(
+        'git_cl.Changelist._AddChangeIdToCommitMessage',
+        return_value=post_amend_description or description).start()
     mock.patch(
         'git_cl.ask_for_data',
         lambda prompt: self._mocked_call('ask_for_data', prompt)).start()
@@ -2818,15 +2805,6 @@ class ChangelistTest(unittest.TestCase):
     self.addCleanup(mock.patch.stopall)
     self.temp_count = 0
 
-  @mock.patch('git_cl.RunGitWithCode')
-  def testGetLocalDescription(self, _mock):
-    git_cl.RunGitWithCode.return_value = (0, 'description')
-    cl = git_cl.Changelist()
-    self.assertEqual('description', cl.GetLocalDescription('branch'))
-    self.assertEqual('description', cl.GetLocalDescription('branch'))
-    git_cl.RunGitWithCode.assert_called_once_with(
-        ['log', '--pretty=format:%s%n%n%b', 'branch...'])
-
   def testRunHook(self):
     expected_results = {
         'more_cc': ['more@example.com', 'cc@example.com'],
@@ -3031,7 +3009,7 @@ class CMDPresubmitTestCase(CMDTestCaseBase):
         'git_cl.Changelist.FetchDescription',
         return_value='fetch description').start()
     mock.patch(
-        'git_cl.Changelist.GetLocalDescription',
+        'git_cl._create_description_from_log',
         return_value='get description').start()
     mock.patch('git_cl.Changelist.RunHook').start()