aiden
/
depot_tools
mirror of https://chromium.googlesource.com/chromium/tools/depot_tools
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Gerrit git cl upload <diff_base>: respect diff_base argumnent.

Browse Source 
This CL propagates <diff_base> 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 <agable@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
changes/29/475229/4
Andrii Shyshkalov 8 years ago committed by Commit Bot
parent d0573ec067
commit 550e924d20
2 changed files with 164 additions and 91 deletions
Show Stats Download Patch File Download Diff File

69
git_cl.py
Unescape Escape View File

@ -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()

186
tests/git_cl_test.py
Unescape Escape View File

@ -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'],

Write Preview
Loading…
Cancel
Save