Handle external changes on `git cl upload`

upload currently overwrites any external changes, e.g. edits/rebases
in Gerrit. Check for these on upload and provide the ability to
incorporate those into the CL.

Bug: 1382528
Change-Id: Id32bf8804c4cdeb12b6a5a7cf206e033bbccd453
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3975855
Reviewed-by: Josip Sokcevic <sokcevic@google.com>
Reviewed-by: Aravind Vasudevan <aravindvasudev@google.com>
Commit-Queue: Gavin Mak <gavinmak@google.com>

git_cl.py

@@ -1903,13 +1903,14 @@ class Changelist(object):
     # Somehow there are no messages even though there are reviewers.
     return 'unsent'
 
-  def GetMostRecentPatchset(self):
+  def GetMostRecentPatchset(self, update=True):
     if not self.GetIssue():
       return None
 
     data = self._GetChangeDetail(['CURRENT_REVISION'])
     patchset = data['revisions'][data['current_revision']]['_number']
-    self.SetPatchset(patchset)
+    if update:
+      self.SetPatchset(patchset)
     return patchset
 
   def GetMostRecentDryRunPatchset(self):
@@ -2090,11 +2091,12 @@ class Changelist(object):
     self._detail_cache.setdefault(cache_key, []).append((options_set, data))
     return data
 
-  def _GetChangeCommit(self):
+  def _GetChangeCommit(self, revision='current'):
     assert self.GetIssue(), 'issue must be set to query Gerrit'
     try:
-      data = gerrit_util.GetChangeCommit(
-          self.GetGerritHost(), self._GerritChangeIdentifier())
+      data = gerrit_util.GetChangeCommit(self.GetGerritHost(),
+                                         self._GerritChangeIdentifier(),
+                                         revision)
     except gerrit_util.GerritError as e:
       if e.http_status == 404:
         raise GerritChangeNotExists(self.GetIssue(), self.GetCodereviewServer())
@@ -2448,12 +2450,21 @@ class Changelist(object):
     """Upload the current branch to Gerrit."""
     if options.squash:
       self._GerritCommitMsgHookCheck(offer_removal=not options.force)
+      external_parent = None
       if self.GetIssue():
         # User requested to change description
         if options.edit_description:
           change_desc.prompt()
-        change_id = self._GetChangeDetail()['change_id']
+        change_detail = self._GetChangeDetail(['CURRENT_REVISION'])
+        change_id = change_detail['change_id']
         change_desc.ensure_change_id(change_id)
+
+        # Check if changes outside of this workspace have been uploaded.
+        current_rev = change_detail['current_revision']
+        last_uploaded_rev = self._GitGetBranchConfigValue(
+            GERRIT_SQUASH_HASH_CONFIG_KEY)
+        if last_uploaded_rev and current_rev != last_uploaded_rev:
+          external_parent = self._UpdateWithExternalChanges()
       else:  # if not self.GetIssue()
         if not options.force and not options.message_file:
           change_desc.prompt()
@@ -2468,7 +2479,7 @@ class Changelist(object):
         change_desc.set_preserve_tryjobs()
 
       remote, upstream_branch = self.FetchUpstreamTuple(self.GetBranch())
-      parent = self._ComputeParent(
+      parent = external_parent or 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:
@@ -2625,6 +2636,10 @@ class Changelist(object):
         'description': change_desc.description,
     }
 
+    # Gerrit may or may not update fast enough to return the correct patchset
+    # number after we push. Get the pre-upload patchset and increment later.
+    latest_ps = self.GetMostRecentPatchset(update=False) or 0
+
     push_stdout = self._RunGitPushWithTraces(refspec, refspec_opts,
                                              git_push_metadata,
                                              options.push_options)
@@ -2639,6 +2654,7 @@ class Changelist(object):
           ('Created|Updated %d issues on Gerrit, but only 1 expected.\n'
            'Change-Id: %s') % (len(change_numbers), change_id), change_desc)
       self.SetIssue(change_numbers[0])
+      self.SetPatchset(latest_ps + 1)
       self._GitSetBranchConfigValue(GERRIT_SQUASH_HASH_CONFIG_KEY, ref_to_push)
 
     if self.GetIssue() and (reviewers or cc):
@@ -2713,6 +2729,82 @@ class Changelist(object):
           change_desc)
     return parent
 
+  def _UpdateWithExternalChanges(self):
+    """Updates workspace with external changes.
+
+    Returns the commit hash that should be used as the merge base on upload.
+    """
+    local_ps = self.GetPatchset()
+    if local_ps is None:
+      return
+
+    external_ps = self.GetMostRecentPatchset(update=False)
+    if external_ps is None or local_ps == external_ps:
+      return
+
+    num_changes = external_ps - local_ps
+    print('\n%d external change(s) have been published to %s. '
+          'Uploading as-is will override them.' %
+          (num_changes, self.GetIssueURL(short=True)))
+    if not ask_for_explicit_yes('Get the latest changes and apply?'):
+      return
+
+    # Get latest Gerrit merge base. Use the first parent even if multiple exist.
+    external_parent = self._GetChangeCommit(revision=external_ps)['parents'][0]
+    external_base = external_parent['commit']
+
+    branch = git_common.current_branch()
+    local_base = self.GetCommonAncestorWithUpstream()
+    if local_base != external_base:
+      print('\nLocal merge base %s is different from Gerrit %s.\n' %
+            (local_base, external_base))
+      if git_common.upstream(branch):
+        DieWithError('Upstream branch set. Consider using `git rebase-update` '
+                     'to make these the same.')
+      print('No upstream branch set. Consider setting it and using '
+            '`git rebase-update`.\nContinuing upload with Gerrit merge base.')
+
+    # Fetch Gerrit's CL base if it doesn't exist locally.
+    remote, _ = self.GetRemoteBranch()
+    if not scm.GIT.IsValidRevision(settings.GetRoot(), external_base):
+      RunGitSilent(['fetch', remote, external_base])
+
+    # Get the diff between local_ps and external_ps.
+    issue = self.GetIssue()
+    changes_ref = 'refs/changes/%d/%d/' % (issue % 100, issue)
+    RunGitSilent(['fetch', remote, changes_ref + str(local_ps)])
+    last_uploaded = RunGitSilent(['rev-parse', 'FETCH_HEAD']).strip()
+    RunGitSilent(['fetch', remote, changes_ref + str(external_ps)])
+    latest_external = RunGitSilent(['rev-parse', 'FETCH_HEAD']).strip()
+    diff = RunGitSilent(['diff', '%s..%s' % (last_uploaded, latest_external)])
+
+    # Diff can be empty in the case of trivial rebases.
+    if not diff:
+      return external_base
+
+    # Apply the diff.
+    with gclient_utils.temporary_file() as diff_tempfile:
+      gclient_utils.FileWrite(diff_tempfile, diff)
+      clean_patch = RunGitWithCode(['apply', '--check', diff_tempfile])[0] == 0
+      RunGitSilent(['apply', '-3', '--intent-to-add', diff_tempfile])
+      if not clean_patch:
+        # Normally patchset is set after upload. But because we exit, that never
+        # happens. Updating here makes sure that subsequent uploads don't need
+        # to fetch/apply the same diff again.
+        self.SetPatchset(external_ps)
+        DieWithError('\nPatch did not apply cleanly. Please resolve any '
+                     'conflicts and reupload.')
+
+      message = 'Incorporate external changes from '
+      if num_changes == 1:
+        message += 'patchset %d' % external_ps
+      else:
+        message += 'patchsets %d to %d' % (local_ps + 1, external_ps)
+      RunGitSilent(['commit', '-am', message])
+      # TODO(crbug.com/1382528): Use the previous commit's message as a default
+      # patchset title instead of this 'Incorporate' message.
+    return external_base
+
   def _AddChangeIdToCommitMessage(self, log_desc, args):
     """Re-commits using the current message, assumes the commit hook is in
     place.

tests/git_cl_test.py

@@ -805,6 +805,8 @@ class TestGitCl(unittest.TestCase):
                            force=False,
                            edit_description=None,
                            default_branch='main',
+                           ref_to_push='abcdef0123456789',
+                           external_parent=None,
                            push_opts=None):
     if post_amend_description is None:
       post_amend_description = description
@@ -830,22 +832,25 @@ class TestGitCl(unittest.TestCase):
         calls += [
           ((['RunEditor'],), edit_description),
         ]
-      ref_to_push = 'abcdef0123456789'
 
-      if custom_cl_base is None:
-        parent = 'origin/' + default_branch
-        git_common.get_or_create_merge_base.return_value = parent
+      if external_parent:
+        parent = external_parent
       else:
-        calls += [
-          ((['git', 'merge-base', '--is-ancestor', custom_cl_base,
-             'refs/remotes/origin/' + default_branch],),
-           callError(1)),   # Means not ancenstor.
-          (('ask_for_data',
-            'Do you take responsibility for cleaning up potential mess '
-            'resulting from proceeding with upload? Press Enter to upload, '
-            'or Ctrl+C to abort'), ''),
-        ]
-        parent = custom_cl_base
+        if custom_cl_base is None:
+          parent = 'origin/' + default_branch
+          git_common.get_or_create_merge_base.return_value = parent
+        else:
+          calls += [
+              (([
+                  'git', 'merge-base', '--is-ancestor', custom_cl_base,
+                  'refs/remotes/origin/' + default_branch
+              ], ), callError(1)),  # Means not ancenstor.
+              (('ask_for_data',
+                'Do you take responsibility for cleaning up potential mess '
+                'resulting from proceeding with upload? Press Enter to upload, '
+                'or Ctrl+C to abort'), ''),
+          ]
+          parent = custom_cl_base
 
       calls += [
         ((['git', 'rev-parse', 'HEAD:'],),  # `HEAD:` means HEAD's tree hash.
@@ -1097,6 +1102,7 @@ class TestGitCl(unittest.TestCase):
                               notify=False,
                               post_amend_description=None,
                               issue=None,
+                              patchset=None,
                               cc=None,
                               fetched_status=None,
                               other_cl_owner=None,
@@ -1112,6 +1118,8 @@ class TestGitCl(unittest.TestCase):
                               edit_description=None,
                               fetched_description=None,
                               default_branch='main',
+                              ref_to_push='abcdef0123456789',
+                              external_parent=None,
                               push_opts=None,
                               reset_issue=False):
     """Generic gerrit upload test framework."""
@@ -1130,6 +1138,8 @@ class TestGitCl(unittest.TestCase):
                 same_auth=('git-owner.example.com', '', 'pass'))).start()
     mock.patch('git_cl.Changelist._GerritCommitMsgHookCheck',
               lambda _, offer_removal: None).start()
+    mock.patch('git_cl.Changelist.GetMostRecentPatchset', lambda _, update:
+               patchset).start()
     mock.patch('git_cl.gclient_utils.RunEditor',
               lambda *_, **__: self._mocked_call(['RunEditor'])).start()
     mock.patch('git_cl.DownloadGerritHook', lambda force: self._mocked_call(
@@ -1220,15 +1230,18 @@ class TestGitCl(unittest.TestCase):
           force=force,
           edit_description=edit_description,
           default_branch=default_branch,
+          ref_to_push=ref_to_push,
+          external_parent=external_parent,
           push_opts=push_opts)
     # Uncomment when debugging.
     # print('\n'.join(map(lambda x: '%2i: %s' % x, enumerate(self.calls))))
     git_cl.main(['upload'] + upload_args)
     if squash:
-      self.assertIssueAndPatchset(patchset=None)
+      self.assertIssueAndPatchset(patchset=str((patchset or 0) + 1))
       self.assertEqual(
-          'abcdef0123456789',
-          scm.GIT.GetBranchConfig('', 'main', 'gerritsquashhash'))
+          ref_to_push,
+          scm.GIT.GetBranchConfig('', 'main',
+                                  git_cl.GERRIT_SQUASH_HASH_CONFIG_KEY))
 
   def test_gerrit_upload_traces_no_gitcookies(self):
     self._run_gerrit_upload_test(
@@ -1501,6 +1514,23 @@ class TestGitCl(unittest.TestCase):
         change_id='123456789',
         edit_description=description)
 
+  @mock.patch('git_cl.Changelist._UpdateWithExternalChanges',
+              return_value='newparent')
+  def test_upload_change_uses_external_base(self, *_mocks):
+    squash_hash = 'branch.main.' + git_cl.GERRIT_SQUASH_HASH_CONFIG_KEY
+    self.mockGit.config[squash_hash] = 'beef2'
+
+    self._run_gerrit_upload_test(
+        ['--squash'],
+        'desc ✔\n\nBUG=\n\nChange-Id: 123456789',
+        [],
+        squash=True,
+        issue=123456,
+        change_id='123456789',
+        ref_to_push='beef2',
+        external_parent='newparent',
+    )
+
   @mock.patch('git_cl.RunGit')
   @mock.patch('git_cl.CMDupload')
   @mock.patch('sys.stdin', StringIO('\n'))