From 550e924d20906bc105215c617ca910e6859f569d Mon Sep 17 00:00:00 2001 From: Andrii Shyshkalov Date: Wed, 12 Apr 2017 17:14:49 +0200 Subject: [PATCH] Gerrit git cl upload : respect diff_base argumnent. This CL propagates all the way to become parent commit of the syntentic commit generated by squashing the current branch. BUG=649846 R=agable@chromium.org Change-Id: Ided7ebbb5c3a1114cac18adb62b3a9c27610018c Reviewed-on: https://chromium-review.googlesource.com/475229 Reviewed-by: Aaron Gable Commit-Queue: Andrii Shyshkalov --- git_cl.py | 69 +++++++++++----- tests/git_cl_test.py | 186 ++++++++++++++++++++++++++----------------- 2 files changed, 164 insertions(+), 91 deletions(-) diff --git a/git_cl.py b/git_cl.py index 426b75257..bd9e1a291 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1583,9 +1583,9 @@ class Changelist(object): def CMDUpload(self, options, git_diff_args, orig_args): """Uploads a change to codereview.""" + custom_cl_base = None if git_diff_args: - # TODO(ukai): is it ok for gerrit case? - base_branch = git_diff_args[0] + custom_cl_base = base_branch = git_diff_args[0] else: if self.GetBranch() is None: DieWithError('Can\'t upload from detached HEAD state. Get on a branch!') @@ -1640,7 +1640,7 @@ class Changelist(object): confirm_or_exit(action='upload') print_stats(options.similarity, options.find_copies, git_diff_args) - ret = self.CMDUploadChange(options, git_diff_args, change) + ret = self.CMDUploadChange(options, git_diff_args, custom_cl_base, change) if not ret: if options.use_commit_queue: self.SetCQState(_CQState.COMMIT) @@ -1869,7 +1869,7 @@ class _ChangelistCodereviewBase(object): """ raise NotImplementedError() - def CMDUploadChange(self, options, args, change): + def CMDUploadChange(self, options, git_diff_args, custom_cl_base, change): """Uploads a change to codereview.""" raise NotImplementedError() @@ -2185,7 +2185,7 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase): codereview='rietveld') return None - def CMDUploadChange(self, options, args, change): + def CMDUploadChange(self, options, args, custom_cl_base, change): """Upload the patch to Rietveld.""" upload_args = ['--assume_yes'] # Don't ask about untracked files. upload_args.extend(['--server', self.GetCodereviewServer()]) @@ -2786,7 +2786,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): else: print('OK, will keep Gerrit commit-msg hook in place.') - def CMDUploadChange(self, options, args, change): + def CMDUploadChange(self, options, git_diff_args, custom_cl_base, change): """Upload the current branch to Gerrit.""" if options.squash and options.no_squash: DieWithError('Can only use one of --squash or --no-squash') @@ -2797,10 +2797,6 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): elif options.no_squash: options.squash = False - # We assume the remote called "origin" is the one we want. - # It is probably not worthwhile to support different workflows. - gerrit_remote = 'origin' - remote, remote_branch = self.GetRemoteBranch() branch = GetTargetRef(remote, remote_branch, options.target_branch) @@ -2872,7 +2868,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): if options.message: message = options.message else: - message = CreateDescriptionFromLog(args) + message = CreateDescriptionFromLog(git_diff_args) if options.title: message = options.title + '\n\n' + message change_desc = ChangeDescription(message) @@ -2898,22 +2894,26 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): change_id = change_ids[0] remote, upstream_branch = self.FetchUpstreamTuple(self.GetBranch()) - parent = self._ComputeParent(remote, upstream_branch, change_desc) + parent = self._ComputeParent(remote, upstream_branch, custom_cl_base, + options.force, change_desc) tree = RunGit(['rev-parse', 'HEAD:']).strip() ref_to_push = RunGit(['commit-tree', tree, '-p', parent, '-m', message]).strip() else: change_desc = ChangeDescription( - options.message or CreateDescriptionFromLog(args)) + options.message or CreateDescriptionFromLog(git_diff_args)) 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, - args)) + change_desc.set_description( + self._AddChangeIdToCommitMessage(options, git_diff_args)) ref_to_push = 'HEAD' - parent = '%s/%s' % (gerrit_remote, branch) + # For no-squash mode, we assume the remote called "origin" is the one we + # want. It is not worthwhile to support different workflows for + # no-squash mode. + parent = 'origin/%s' % branch change_id = git_footers.get_footer_change_id(change_desc.description)[0] assert change_desc @@ -3020,7 +3020,33 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): is_reviewer=False, notify=bool(options.send_mail)) return 0 - def _ComputeParent(self, remote, upstream_branch, change_desc): + def _ComputeParent(self, remote, upstream_branch, custom_cl_base, force, + change_desc): + """Computes parent of the generated commit to be uploaded to Gerrit. + + Returns revision or a ref name. + """ + if custom_cl_base: + # Try to avoid creating additional unintended CLs when uploading, unless + # user wants to take this risk. + local_ref_of_target_remote = self.GetRemoteBranch()[1] + code, _ = RunGitWithCode(['merge-base', '--is-ancestor', custom_cl_base, + local_ref_of_target_remote]) + if code == 1: + print('\nWARNING: manually specified base of this CL `%s` ' + 'doesn\'t seem to belong to target remote branch `%s`.\n\n' + 'If you proceed with upload, more than 1 CL may be created by ' + 'Gerrit as a result, in turn confusing or crashing git cl.\n\n' + 'If you are certain that specified base `%s` has already been ' + 'uploaded to Gerrit as another CL, you may proceed.\n' % + (custom_cl_base, local_ref_of_target_remote, custom_cl_base)) + if not force: + confirm_or_exit( + 'Do you take responsibility for cleaning up potential mess ' + 'resulting from proceeding with upload?', + action='upload') + return custom_cl_base + if remote != '.': return self.GetCommonAncestorWithUpstream() @@ -3028,12 +3054,15 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): # squashed version. upstream_branch_name = scm.GIT.ShortBranchName(upstream_branch) - # TODO(tandrii): Remove this master-specific hack - # Bug: https://bugs.chromium.org/p/chromium/issues/detail?id=682104 if upstream_branch_name == 'master': - return RunGitSilent(['rev-parse', 'origin/master']).strip() + return 'origin/master' # Check the squashed hash of the parent. + # TODO(tandrii): consider checking parent change in Gerrit and using its + # hash if tree hash of latest parent revision (patchset) in Gerrit matches + # the tree hash of the parent branch. The upside is less likely bogus + # requests to reupload parent change just because it's uploadhash is + # missing, yet the downside likely exists, too (albeit unknown to me yet). parent = RunGit(['config', 'branch.%s.gerritsquashhash' % upstream_branch_name], error_ok=True).strip() diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 0ea2a6a29..eeb977af9 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -1316,28 +1316,35 @@ class TestGitCl(TestCase): @classmethod def _gerrit_base_calls(cls, issue=None, fetched_description=None, - fetched_status=None, other_cl_owner=None): + fetched_status=None, other_cl_owner=None, + custom_cl_base=None): calls = [ - ((['git', 'symbolic-ref', 'HEAD'],), 'master'), - ((['git', 'config', 'branch.master.git-cl-similarity'],), - CERR1), - ((['git', 'symbolic-ref', 'HEAD'],), 'master'), - ((['git', 'config', '--bool', 'branch.master.git-find-copies'],), - CERR1), + ((['git', 'symbolic-ref', 'HEAD'],), 'master'), + ((['git', 'config', 'branch.master.git-cl-similarity'],), CERR1), + ((['git', 'symbolic-ref', 'HEAD'],), 'master'), + ((['git', 'config', '--bool', 'branch.master.git-find-copies'],), CERR1), ] calls += cls._is_gerrit_calls(True) calls += [ - ((['git', 'symbolic-ref', 'HEAD'],), 'master'), - ((['git', 'config', 'branch.master.rietveldissue'],), CERR1), - ((['git', 'config', 'branch.master.gerritissue'],), - CERR1 if issue is None else str(issue)), + ((['git', 'symbolic-ref', 'HEAD'],), 'master'), + ((['git', 'config', 'branch.master.rietveldissue'],), CERR1), + ((['git', 'config', 'branch.master.gerritissue'],), + CERR1 if issue is None else str(issue)), + ] + + if custom_cl_base: + ancestor_revision = custom_cl_base + else: + # Determine ancestor_revision to be merge base. + ancestor_revision = 'fake_ancestor_sha' + calls += [ ((['git', 'config', 'branch.master.merge'],), 'refs/heads/master'), ((['git', 'config', 'branch.master.remote'],), 'origin'), ((['get_or_create_merge_base', 'master', - 'refs/remotes/origin/master'],), - 'fake_ancestor_sha'), - # Calls to verify branch point is ancestor - ] + 'refs/remotes/origin/master'],), ancestor_revision), + ] + + # Calls to verify branch point is ancestor calls += cls._gerrit_ensure_auth_calls(issue=issue) if issue: @@ -1367,32 +1374,31 @@ class TestGitCl(TestCase): (('ask_for_data', 'Press Enter to upload, or Ctrl+C to abort'), ''), ] - calls += cls._git_sanity_checks('fake_ancestor_sha', 'master', - get_remote_branch=False) + calls += cls._git_sanity_checks(ancestor_revision, 'master', + get_remote_branch=False) calls += [ - ((['git', 'rev-parse', '--show-cdup'],), ''), - ((['git', 'rev-parse', 'HEAD'],), '12345'), + ((['git', 'rev-parse', '--show-cdup'],), ''), + ((['git', 'rev-parse', 'HEAD'],), '12345'), - ((['git', - 'diff', '--name-status', '--no-renames', '-r', - 'fake_ancestor_sha...', '.'],), - 'M\t.gitignore\n'), - ((['git', 'config', 'branch.master.gerritpatchset'],), CERR1), + ((['git', 'diff', '--name-status', '--no-renames', '-r', + ancestor_revision + '...', '.'],), + 'M\t.gitignore\n'), + ((['git', 'config', 'branch.master.gerritpatchset'],), CERR1), ] if not issue: calls += [ - ((['git', - 'log', '--pretty=format:%s%n%n%b', 'fake_ancestor_sha...'],), + ((['git', 'log', '--pretty=format:%s%n%n%b', + ancestor_revision + '...'],), 'foo'), ] calls += [ - ((['git', 'config', 'user.email'],), 'me@example.com'), - ((['git', - 'diff', '--no-ext-diff', '--stat', '-l100000', '-C50', - 'fake_ancestor_sha', 'HEAD'],), - '+dat'), + ((['git', 'config', 'user.email'],), 'me@example.com'), + ((['git', 'diff', '--no-ext-diff', '--stat', '-l100000', '-C50'] + + ([custom_cl_base] if custom_cl_base else + [ancestor_revision, 'HEAD']),), + '+dat'), ] return calls @@ -1402,11 +1408,15 @@ class TestGitCl(TestCase): expected_upstream_ref='origin/refs/heads/master', ref_suffix='', title=None, notify=False, post_amend_description=None, issue=None, cc=None, - git_mirror=None): + git_mirror=None, + custom_cl_base=None): if post_amend_description is None: post_amend_description = description - calls = [] cc = cc or [] + # Determined in `_gerrit_base_calls`. + determined_ancestor_revision = custom_cl_base or 'fake_ancestor_sha' + + calls = [] if squash_mode == 'default': calls.extend([ @@ -1424,62 +1434,80 @@ class TestGitCl(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', - 'fake_ancestor_sha..HEAD'],), - description)] + ((['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: if not title: calls += [ - ((['git', 'show', '-s', '--format=%s', 'HEAD'],), ''), - (('ask_for_data', 'Title for patchset []: '), 'User input'), + ((['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) - ((False, ), - ''), - # Amending of commit message to get the Change-Id. - ((['git', 'log', '--pretty=format:%s\n\n%b', - 'fake_ancestor_sha..HEAD'],), - description), - ((['git', 'commit', '--amend', '-m', description],), - ''), - ((['git', 'log', '--pretty=format:%s\n\n%b', - 'fake_ancestor_sha..HEAD'],), - post_amend_description) - ] + (('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 not issue: # Prompting to edit description on first upload. calls += [ - ((['git', 'config', 'core.editor'],), ''), - ((['RunEditor'],), description), + ((['git', 'config', 'core.editor'],), ''), + ((['RunEditor'],), description), ] ref_to_push = 'abcdef0123456789' calls += [ - ((['git', 'config', 'branch.master.merge'],), - 'refs/heads/master'), - ((['git', 'config', 'branch.master.remote'],), - 'origin'), + ((['git', 'config', 'branch.master.merge'],), 'refs/heads/master'), + ((['git', 'config', 'branch.master.remote'],), 'origin'), + ] + + if custom_cl_base is None: + calls += [ ((['get_or_create_merge_base', 'master', 'refs/remotes/origin/master'],), 'origin/master'), - ((['git', 'rev-parse', 'HEAD:'],), - '0123456789abcdef'), - ((['git', 'commit-tree', '0123456789abcdef', '-p', - 'origin/master', '-m', description],), - ref_to_push), - ] + ] + parent = 'origin/master' + else: + calls += [ + ((['git', 'merge-base', '--is-ancestor', custom_cl_base, + 'refs/remotes/origin/master'],), + 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. + '0123456789abcdef'), + ((['git', 'commit-tree', '0123456789abcdef', '-p', parent, + '-m', description],), + ref_to_push), + ] else: ref_to_push = 'HEAD' calls += [ - ((['git', 'rev-list', - expected_upstream_ref + '..' + ref_to_push],), ''), - ] + ((['git', 'rev-list', + (custom_cl_base if custom_cl_base else expected_upstream_ref) + '..' + + ref_to_push],), + '1hashPerLine\n'), + ] if title: if ref_suffix: @@ -1565,7 +1593,8 @@ class TestGitCl(TestCase): cc=None, git_mirror=None, fetched_status=None, - other_cl_owner=None): + other_cl_owner=None, + custom_cl_base=None): """Generic gerrit upload test framework.""" if squash_mode is None: if '--no-squash' in upload_args: @@ -1585,7 +1614,8 @@ class TestGitCl(TestCase): lambda _, offer_removal: None) self.mock(git_cl.gclient_utils, 'RunEditor', lambda *_, **__: self._mocked_call(['RunEditor'])) - self.mock(git_cl, 'DownloadGerritHook', self._mocked_call) + self.mock(git_cl, 'DownloadGerritHook', lambda force: self._mocked_call( + 'DownloadGerritHook', force)) original_os_path_isdir = os.path.isdir def selective_os_path_isdir_mock(path): @@ -1598,7 +1628,8 @@ class TestGitCl(TestCase): issue=issue, fetched_description=description, fetched_status=fetched_status, - other_cl_owner=other_cl_owner) + other_cl_owner=other_cl_owner, + custom_cl_base=custom_cl_base) if fetched_status != 'ABANDONED': self.calls += self._gerrit_upload_calls( description, reviewers, squash, @@ -1606,7 +1637,8 @@ class TestGitCl(TestCase): expected_upstream_ref=expected_upstream_ref, ref_suffix=ref_suffix, title=title, notify=notify, post_amend_description=post_amend_description, - issue=issue, cc=cc, git_mirror=git_mirror) + issue=issue, cc=cc, git_mirror=git_mirror, + custom_cl_base=custom_cl_base) # Uncomment when debugging. # print '\n'.join(map(lambda x: '%2i: %s' % x, enumerate(self.calls))) git_cl.main(['upload'] + upload_args) @@ -1620,7 +1652,6 @@ class TestGitCl(TestCase): post_amend_description='desc\n\nBUG=\n\nChange-Id: Ixxx') def test_gerrit_upload_without_change_id_override_nosquash(self): - self.mock(git_cl, 'DownloadGerritHook', self._mocked_call) self._run_gerrit_upload_test( [], 'desc\n\nBUG=\n', @@ -1687,6 +1718,19 @@ class TestGitCl(TestCase): squash=True, expected_upstream_ref='origin/master') + def test_gerrit_upload_squash_first_against_rev(self): + custom_cl_base = 'custom_cl_base_rev_or_branch' + self._run_gerrit_upload_test( + ['--squash', custom_cl_base], + 'desc\nBUG=\n\nChange-Id: 123456789', + [], + squash=True, + expected_upstream_ref='origin/master', + custom_cl_base=custom_cl_base) + self.assertIn( + 'If you proceed with upload, more than 1 CL may be created by Gerrit', + sys.stdout.getvalue()) + def test_gerrit_upload_squash_first_with_mirror(self): self._run_gerrit_upload_test( ['--squash'],