Reland "git-cl: Refactor CMDUploadChange"

This is a reland of 9f29465e52 Original change's description: > git-cl: Refactor CMDUploadChange > > Changes: > - Add _GetDescriptionForUpload and _GetTitleForUpload. > - Add ensure_change_id to ChangeDescription, that ensures the > description has a particular Change-Id. > - If more than a Change-Id was entered on new issue upload, remove > all, and add a new one. > > Change-Id: I0eae474eb07ea3036973b18571f72a80da425b21 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2101811 > Reviewed-by: Josip Sokcevic <sokcevic@google.com> > Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org> Change-Id: Ib2c0ad61b169f6b0c3141674591b0d3fa42bc664 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2103532 Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org> Reviewed-by: Josip Sokcevic <sokcevic@google.com>
5 years ago · 5a644f8276
parent 1850bf6d17
commit 5a644f8276
2 changed files with 123 additions and 150 deletions
--- a/git_cl.py
+++ b/git_cl.py
@ -1447,6 +1447,62 @@ class Changelist(object):
      p = subprocess2.Popen(['vpython', PRESUBMIT_SUPPORT] + args)
      p.wait()

+  def _GetDescriptionForUpload(self, options, git_diff_args, files):
+    # 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
+
+    # Extract bug number from branch name.
+    bug = options.bug
+    fixed = options.fixed
+    match = re.match(r'(?P<type>bug|fix(?:e[sd])?)[_-]?(?P<bugnum>\d+)',
+                     self.GetBranch())
+    if not bug and not fixed and match:
+      if match.group('type') == 'bug':
+        bug = match.group('bugnum')
+      else:
+        fixed = match.group('bugnum')
+
+    change_description = ChangeDescription(description, bug, fixed)
+
+    # Set the reviewer list now so that presubmit checks can access it.
+    if options.reviewers or options.tbrs or options.add_owners_to:
+      change_description.update_reviewers(
+          options.reviewers, options.tbrs, options.add_owners_to, files,
+          self.GetAuthor())
+
+    return change_description
+
+  def _GetTitleForUpload(self, options):
+    # When not squashing, just return options.title.
+    if not options.squash:
+      return options.title
+
+    # On first upload, patchset title is always this string, while options.title
+    # gets converted to first line of message.
+    if not self.GetIssue():
+      return 'Initial upload'
+
+    # When uploading subsequent patchsets, options.message is taken as the title
+    # if options.title is not provided.
+    if options.title:
+      return options.title
+    if options.message:
+      return options.message.strip()
+
+    # Use the subject of the last commit as title by default.
+    title = RunGit(
+        ['show', '-s', '--format=%s', 'HEAD']).strip()
+    if options.force:
+      return title
+    return ask_for_data('Title for patchset [%s]: ' % title) or title
+
  def CMDUpload(self, options, git_diff_args, orig_args):
    """Uploads a change to codereview."""
    custom_cl_base = None
@ -1466,30 +1522,13 @@ 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.
    watchlist = watchlists.Watchlists(settings.GetRoot())
    files = self.GetAffectedFiles(base_branch)
    if not options.bypass_watchlists:
      self.ExtendCC(watchlist.GetWatchersForPaths(files))

-    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)
-      change_description.update_reviewers(
-          options.reviewers, options.tbrs, options.add_owners_to, files,
-          self.GetAuthor())
-      description = change_description.description
-
+    change_desc = self._GetDescriptionForUpload(options, git_diff_args, files)
    if not options.bypass_hooks:
      hook_results = self.RunHook(
          committing=False,
@ -1497,19 +1536,20 @@ class Changelist(object):
          verbose=options.verbose,
          parallel=options.parallel,
          upstream=base_branch,
-          description=description,
+          description=change_desc.description,
          all_files=False)
      self.ExtendCC(hook_results['more_cc'])

    print_stats(git_diff_args)
    ret = self.CMDUploadChange(
-        options, git_diff_args, custom_cl_base, description)
+        options, git_diff_args, custom_cl_base, change_desc)
    if not ret:
      self._GitSetBranchConfigValue(
          'last-upload-hash', RunGit(['rev-parse', 'HEAD']).strip())
      # Run post upload hooks, if specified.
      if settings.GetRunPostUploadHook():
-        self.RunPostUploadHook(options.verbose, base_branch, description)
+        self.RunPostUploadHook(
+            options.verbose, base_branch, change_desc.description)

      # Upload all dependencies if specified.
      if options.dependencies:
@ -2237,7 +2277,7 @@ class Changelist(object):
    return push_stdout

  def CMDUploadChange(
-      self, options, git_diff_args, custom_cl_base, message):
+      self, options, git_diff_args, custom_cl_base, change_desc):
    """Upload the current branch to Gerrit."""
    if options.squash is None:
      # Load default for user, repo, squash=true, in this order.
@ -2245,101 +2285,42 @@ class Changelist(object):

    remote, remote_branch = self.GetRemoteBranch()
    branch = GetTargetRef(remote, remote_branch, options.target_branch)
-    # This may be None; default fallback value is determined in logic below.
-    title = options.title
-
-    # Extract bug number from branch name.
-    bug = options.bug
-    fixed = options.fixed
-    match = re.match(r'(?P<type>bug|fix(?:e[sd])?)[_-]?(?P<bugnum>\d+)',
-                     self.GetBranch())
-    if not bug and not fixed and match:
-      if match.group('type') == 'bug':
-        bug = match.group('bugnum')
-      else:
-        fixed = match.group('bugnum')

    if options.squash:
      self._GerritCommitMsgHookCheck(offer_removal=not options.force)
      if self.GetIssue():
-        if not title:
-          if options.message:
-            # When uploading a subsequent patchset, -m|--message is taken
-            # as the patchset title if --title was not provided.
-            title = options.message.strip()
-          else:
-            default_title = RunGit(
-                ['show', '-s', '--format=%s', 'HEAD']).strip()
-            if options.force:
-              title = default_title
-            else:
-              title = ask_for_data(
-                  'Title for patchset [%s]: ' % default_title) or default_title
-
        # User requested to change description
        if options.edit_description:
-          change_desc = ChangeDescription(message, bug=bug, fixed=fixed)
          change_desc.prompt()
-          message = change_desc.description
-
        change_id = self._GetChangeDetail()['change_id']
-
-        # Make sure that the Change-Id in the description matches the one
-        # fetched from Gerrit.
-        footer_change_ids = git_footers.get_footer_change_id(message)
-        if footer_change_ids != [change_id]:
-          if footer_change_ids:
-            # Remove any existing Change-Id footers since they don't match the
-            # expected change_id footer.
-            message = git_footers.remove_footer(message, 'Change-Id')
-          # Add the expected Change-Id footer.
-          message = git_footers.add_footer_change_id(message, change_id)
-          print('WARNING: Change-Id has been set to Change-Id fetched from '
-                'Gerrit. Use `git cl issue 0` if you want to clear it and '
-                'set a new one.')
-        change_desc = ChangeDescription(message, bug=bug, fixed=fixed)
+        change_desc.ensure_change_id(change_id)
      else:  # if not self.GetIssue()
-        change_desc = ChangeDescription(message, bug=bug, fixed=fixed)
        if not options.force:
          change_desc.prompt()
-
-        # On first upload, patchset title is always this string, while
-        # --title flag gets converted to first line of message.
-        title = 'Initial upload'
-        if not change_desc.description:
-          DieWithError("Description is empty. Aborting...")
        change_ids = git_footers.get_footer_change_id(change_desc.description)
-        if len(change_ids) > 1:
-          DieWithError('too many Change-Id footers, at most 1 allowed.')
-        if not change_ids:
-          # Generate the Change-Id automatically.
-          change_desc.set_description(git_footers.add_footer_change_id(
-              change_desc.description,
-              GenerateGerritChangeId(change_desc.description)))
-          change_ids = git_footers.get_footer_change_id(change_desc.description)
-          assert len(change_ids) == 1
-        change_id = change_ids[0]
+        if len(change_ids) == 1:
+          change_id = change_ids[0]
+        else:
+          change_id = GenerateGerritChangeId(change_desc.description)
+          change_desc.ensure_change_id(change_id)

      if options.preserve_tryjobs:
        change_desc.set_preserve_tryjobs()

      remote, upstream_branch = self.FetchUpstreamTuple(self.GetBranch())
-      parent = self._ComputeParent(remote, upstream_branch, custom_cl_base,
-                                   options.force, change_desc)
+      parent = self._ComputeParent(
+          remote, upstream_branch, custom_cl_base, options.force, change_desc)
      tree = RunGit(['rev-parse', 'HEAD:']).strip()
      with gclient_utils.temporary_file() as desc_tempfile:
        gclient_utils.FileWrite(desc_tempfile, change_desc.description)
        ref_to_push = RunGit(
            ['commit-tree', tree, '-p', parent, '-F', desc_tempfile]).strip()
    else:  # if not options.squash
-      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(message, git_diff_args))
+            self._AddChangeIdToCommitMessage(
+                change_desc.description, git_diff_args))
      ref_to_push = 'HEAD'
      # For no-squash mode, we assume the remote called "origin" is the one we
      # want. It is not worthwhile to support different workflows for
@ -2347,7 +2328,6 @@ class Changelist(object):
      parent = 'origin/%s' % branch
      change_id = git_footers.get_footer_change_id(change_desc.description)[0]

-    assert change_desc
    SaveDescriptionBackup(change_desc)
    commits = RunGitSilent(['rev-list', '%s..%s' % (parent,
                                                    ref_to_push)]).splitlines()
@ -2399,6 +2379,7 @@ class Changelist(object):
    # TODO(tandrii): options.message should be posted as a comment
    # if --send-mail is set on non-initial upload as Rietveld used to do it.

+    title = self._GetTitleForUpload(options)
    if title:
      # Punctuation and whitespace in |title| must be percent-encoded.
      refspec_opts.append('m=' + gerrit_util.PercentEncodeForGitRef(title))
@ -2673,6 +2654,21 @@ class ChangeDescription(object):
      lines.pop(-1)
    self._description_lines = lines

+  def ensure_change_id(self, change_id):
+    description = self.description
+    footer_change_ids = git_footers.get_footer_change_id(description)
+    # Make sure that the Change-Id in the description matches the given one.
+    if footer_change_ids != [change_id]:
+      if footer_change_ids:
+        # Remove any existing Change-Id footers since they don't match the
+        # expected change_id footer.
+        description = git_footers.remove_footer(description, 'Change-Id')
+        print('WARNING: Change-Id has been set to %s. Use `git cl issue 0` '
+              'if you want to set a new one.')
+      # Add the expected Change-Id footer.
+      description = git_footers.add_footer_change_id(description, change_id)
+      self.set_description(description)
+
  def update_reviewers(
      self, reviewers, tbrs, add_owners_to, affected_files, author_email):
    """Rewrites the R=/TBR= line(s) as a single line each.
--- a/tests/git_cl_test.py
+++ b/tests/git_cl_test.py
@ -850,12 +850,11 @@ class TestGitCl(unittest.TestCase):

  def _gerrit_upload_calls(self, description, reviewers, squash,
                           squash_mode='default',
-                           expected_upstream_ref='origin/refs/heads/master',
                           title=None, notify=False,
                           post_amend_description=None, issue=None, cc=None,
                           custom_cl_base=None, tbr=None,
                           short_hostname='chromium',
-                           labels=None, change_id=None, original_title=None,
+                           labels=None, change_id=None,
                           final_description=None, gitcookies_exists=True,
                           force=False, edit_description=None):
    if post_amend_description is None:
@ -868,23 +867,12 @@ class TestGitCl(unittest.TestCase):
      self.mockGit.config['gerrit.override-squash-uploads'] = (
          'true' if squash_mode == 'override_squash' else 'false')

-    # If issue is given, then description is fetched from Gerrit instead.
-    if issue is None:
-      if squash:
-        title = 'Initial_upload'
-    else:
-      if not title:
-        calls += [
-          ((['git', 'show', '-s', '--format=%s', 'HEAD'],), ''),
-          (('ask_for_data', 'Title for patchset []: '), 'User input'),
-        ]
-        title = 'User_input'
    if not git_footers.get_footer_change_id(description) and not squash:
      calls += [
        (('DownloadGerritHook', False), ''),
      ]
    if squash:
-      if not force and not issue:
+      if not issue and not force:
        calls += [
          ((['RunEditor'],), description),
        ]
@ -923,13 +911,11 @@ class TestGitCl(unittest.TestCase):
      ]
    else:
      ref_to_push = 'HEAD'
+      parent = 'origin/refs/heads/master'

    calls += [
      (('SaveDescriptionBackup',), None),
-      ((['git', 'rev-list',
-         (custom_cl_base if custom_cl_base else expected_upstream_ref) + '..' +
-         ref_to_push],),
-      '1hashPerLine\n'),
+      ((['git', 'rev-list', parent + '..' + ref_to_push],),'1hashPerLine\n'),
    ]

    metrics_arguments = []
@ -945,8 +931,19 @@ class TestGitCl(unittest.TestCase):
        ref_suffix = '%notify=NONE'
        metrics_arguments.append('notify=NONE')

+    # If issue is given, then description is fetched from Gerrit instead.
+    if issue is None:
+      if squash:
+        title = 'Initial upload'
+    else:
+      if not title:
+        calls += [
+          ((['git', 'show', '-s', '--format=%s', 'HEAD'],), ''),
+          (('ask_for_data', 'Title for patchset []: '), 'User input'),
+        ]
+        title = 'User input'
    if title:
-      ref_suffix += ',m=' + title
+      ref_suffix += ',m=' + gerrit_util.PercentEncodeForGitRef(title)
      metrics_arguments.append('m')

    if short_hostname == 'chromium':
@ -1029,7 +1026,6 @@ class TestGitCl(unittest.TestCase):
    ]

    final_description = final_description or post_amend_description.strip()
-    original_title = original_title or title or '<untitled>'
    # Trace-related calls
    calls += [
        # Write a description with context for the current trace.
@ -1045,7 +1041,7 @@ class TestGitCl(unittest.TestCase):
             'short_hostname': short_hostname,
             'change_id': change_id,
             'description': final_description,
-             'title': original_title,
+             'title': title or '<untitled>',
             'trace_name': 'TRACES_DIR/20170316T200041.000000',
           }],),
         None,
@ -1118,7 +1114,6 @@ class TestGitCl(unittest.TestCase):
      reviewers=None,
      squash=True,
      squash_mode=None,
-      expected_upstream_ref='origin/refs/heads/master',
      title=None,
      notify=False,
      post_amend_description=None,
@ -1131,7 +1126,6 @@ class TestGitCl(unittest.TestCase):
      short_hostname='chromium',
      labels=None,
      change_id=None,
-      original_title=None,
      final_description=None,
      gitcookies_exists=True,
      force=False,
@ -1186,6 +1180,8 @@ class TestGitCl(unittest.TestCase):
    mock.patch(
        'git_cl.Changelist._AddChangeIdToCommitMessage',
        return_value=post_amend_description or description).start()
+    mock.patch(
+        'git_cl.GenerateGerritChangeId', return_value=change_id).start()
    mock.patch(
        'git_cl.ask_for_data',
        lambda prompt: self._mocked_call('ask_for_data', prompt)).start()
@ -1212,7 +1208,6 @@ class TestGitCl(unittest.TestCase):
      self.calls += self._gerrit_upload_calls(
          description, reviewers, squash,
          squash_mode=squash_mode,
-          expected_upstream_ref=expected_upstream_ref,
          title=title, notify=notify,
          post_amend_description=post_amend_description,
          issue=issue, cc=cc,
@ -1220,7 +1215,6 @@ class TestGitCl(unittest.TestCase):
          short_hostname=short_hostname,
          labels=labels,
          change_id=change_id,
-          original_title=original_title,
          final_description=final_description,
          gitcookies_exists=gitcookies_exists,
          force=force,
@ -1245,6 +1239,13 @@ class TestGitCl(unittest.TestCase):
        gitcookies_exists=False)

  def test_gerrit_upload_without_change_id(self):
+    self._run_gerrit_upload_test(
+        [],
+        'desc ✔\n\nBUG=\n\nChange-Id: Ixxx',
+        [],
+        change_id='Ixxx')
+
+  def test_gerrit_upload_without_change_id_nosquash(self):
    self._run_gerrit_upload_test(
        ['--no-squash'],
        'desc ✔\n\nBUG=\n',
@ -1289,15 +1290,14 @@ class TestGitCl(unittest.TestCase):
        'desc ✔\n\nBUG=\n\nChange-Id: I123456789',
        squash=False,
        squash_mode='override_nosquash',
-        title='We%27ll_escape_%5E%5F_%5E_special_chars%2E%2E%2E%40%7Bu%7D',
        change_id='I123456789',
-        original_title='We\'ll escape ^_ ^ special chars...@{u}')
+        title='We\'ll escape ^_ ^ special chars...@{u}')

  def test_gerrit_reviewers_cmd_line(self):
    self._run_gerrit_upload_test(
        ['-r', 'foo@example.com', '--send-mail'],
        'desc ✔\n\nBUG=\n\nChange-Id: I123456789',
-        ['foo@example.com'],
+        reviewers=['foo@example.com'],
        squash=False,
        squash_mode='override_nosquash',
        notify=True,
@ -1311,9 +1311,7 @@ class TestGitCl(unittest.TestCase):
        u'desc=\n\nBug: 10000\nChange-Id: Ixxx',
        [],
        force=True,
-        expected_upstream_ref='origin/master',
        fetched_description='desc=\n\nChange-Id: Ixxx',
-        original_title='Initial upload',
        change_id='Ixxx')

  def test_gerrit_upload_corrects_wrong_change_id(self):
@ -1322,10 +1320,8 @@ class TestGitCl(unittest.TestCase):
        u'desc=\n\nBug: 10000\nChange-Id: Ixxxx',
        [],
        issue='123456',
-        expected_upstream_ref='origin/master',
        edit_description='desc=\n\nBug: 10000\nChange-Id: Izzzz',
        fetched_description='desc=\n\nChange-Id: Ixxxx',
-        original_title='Title',
        title='Title',
        change_id='Ixxxx')

@ -1335,9 +1331,7 @@ class TestGitCl(unittest.TestCase):
        u'desc=\n\nFixed: 10000\nChange-Id: Ixxx',
        [],
        force=True,
-        expected_upstream_ref='origin/master',
        fetched_description='desc=\n\nChange-Id: Ixxx',
-        original_title='Initial upload',
        change_id='Ixxx')

  def test_gerrit_reviewer_multiple(self):
@ -1349,21 +1343,17 @@ class TestGitCl(unittest.TestCase):
        'CC=more@example.com,people@example.com\n\n'
        'Change-Id: 123456789',
        ['reviewer@example.com', 'another@example.com'],
-        expected_upstream_ref='origin/master',
        cc=['more@example.com', 'people@example.com'],
        tbr='reviewer@example.com',
        labels={'Code-Review': 2},
-        change_id='123456789',
-        original_title='Initial upload')
+        change_id='123456789')

  def test_gerrit_upload_squash_first_is_default(self):
    self._run_gerrit_upload_test(
        [],
        'desc ✔\nBUG=\n\nChange-Id: 123456789',
        [],
-        expected_upstream_ref='origin/master',
-        change_id='123456789',
-        original_title='Initial upload')
+        change_id='123456789')

  def test_gerrit_upload_squash_first(self):
    self._run_gerrit_upload_test(
@ -1371,9 +1361,7 @@ class TestGitCl(unittest.TestCase):
        'desc ✔\nBUG=\n\nChange-Id: 123456789',
        [],
        squash=True,
-        expected_upstream_ref='origin/master',
-        change_id='123456789',
-        original_title='Initial upload')
+        change_id='123456789')

  def test_gerrit_upload_squash_first_with_labels(self):
    self._run_gerrit_upload_test(
@ -1381,10 +1369,8 @@ class TestGitCl(unittest.TestCase):
        'desc ✔\nBUG=\n\nChange-Id: 123456789',
        [],
        squash=True,
-        expected_upstream_ref='origin/master',
        labels={'Commit-Queue': 1, 'Auto-Submit': 1},
-        change_id='123456789',
-        original_title='Initial upload')
+        change_id='123456789')

  def test_gerrit_upload_squash_first_against_rev(self):
    custom_cl_base = 'custom_cl_base_rev_or_branch'
@ -1393,10 +1379,8 @@ class TestGitCl(unittest.TestCase):
        'desc ✔\nBUG=\n\nChange-Id: 123456789',
        [],
        squash=True,
-        expected_upstream_ref='origin/master',
        custom_cl_base=custom_cl_base,
-        change_id='123456789',
-        original_title='Initial upload')
+        change_id='123456789')
    self.assertIn(
        'If you proceed with upload, more than 1 CL may be created by Gerrit',
        sys.stdout.getvalue())
@ -1408,10 +1392,8 @@ class TestGitCl(unittest.TestCase):
        description,
        [],
        squash=True,
-        expected_upstream_ref='origin/master',
        issue=123456,
-        change_id='123456789',
-        original_title='User input')
+        change_id='123456789')

  @mock.patch('sys.stderr', StringIO())
  def test_gerrit_upload_squash_reupload_to_abandoned(self):
@ -1422,7 +1404,6 @@ class TestGitCl(unittest.TestCase):
          description,
          [],
          squash=True,
-          expected_upstream_ref='origin/master',
          issue=123456,
          fetched_status='ABANDONED',
          change_id='123456789')
@ -1441,11 +1422,9 @@ class TestGitCl(unittest.TestCase):
          description,
          [],
          squash=True,
-          expected_upstream_ref='origin/master',
          issue=123456,
          other_cl_owner='other@example.com',
-          change_id='123456789',
-          original_title='User input')
+          change_id='123456789')
    self.assertIn(
        'WARNING: Change 123456 is owned by other@example.com, but you '
        'authenticate to Gerrit as yet-another@example.com.\n'
@ -1461,10 +1440,8 @@ class TestGitCl(unittest.TestCase):
        [],
        fetched_description=fetched_description,
        squash=True,
-        expected_upstream_ref='origin/master',
        issue=123456,
        change_id='123456789',
-        original_title='User input',
        edit_description=description)

  @mock.patch('git_cl.RunGit')