diff --git a/git_cl.py b/git_cl.py index a93f7db5d8..6e07ff878a 100755 --- a/git_cl.py +++ b/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. diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 45e229d8bc..102e2451eb 100755 --- a/tests/git_cl_test.py +++ b/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'))