Commit 27386ddd authored by bauerb@chromium.org's avatar bauerb@chromium.org

Add a --squash option to Gerrit uploads.

This makes uploading to Gerrit with `git cl` more similar to uploading to Rietveld, by uploading a squashed commit containing the diff to the newest common ancestor.

Uploaded commits are stored in refs/heads/git_cl_uploads/<branch> (and their commit message is used for the squash commit), which allows amending the commit message to change it on the uploaded CL, and looking through the reflog to find old uploaded versions.

BUG=none

Review URL: https://codereview.chromium.org/200023004

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@294077 0039d316-1c4b-4281-b951-d872f2087c98
parent 088e33ec
...@@ -18,6 +18,7 @@ import Queue ...@@ -18,6 +18,7 @@ import Queue
import re import re
import stat import stat
import sys import sys
import tempfile
import textwrap import textwrap
import threading import threading
import urllib2 import urllib2
...@@ -113,6 +114,11 @@ def RunGitWithCode(args, suppress_stderr=False): ...@@ -113,6 +114,11 @@ def RunGitWithCode(args, suppress_stderr=False):
return 1, '' return 1, ''
def RunGitSilent(args):
"""Returns stdout, suppresses stderr and ingores the return code."""
return RunGitWithCode(args, suppress_stderr=True)[1]
def IsGitVersionAtLeast(min_version): def IsGitVersionAtLeast(min_version):
prefix = 'git version ' prefix = 'git version '
version = RunGit(['--version']).strip() version = RunGit(['--version']).strip()
...@@ -1633,7 +1639,7 @@ def GerritUpload(options, args, cl, change): ...@@ -1633,7 +1639,7 @@ def GerritUpload(options, args, cl, change):
"""upload the current branch to gerrit.""" """upload the current branch to gerrit."""
# We assume the remote called "origin" is the one we want. # We assume the remote called "origin" is the one we want.
# It is probably not worthwhile to support different workflows. # It is probably not worthwhile to support different workflows.
remote = 'origin' gerrit_remote = 'origin'
branch = 'master' branch = 'master'
if options.target_branch: if options.target_branch:
branch = options.target_branch branch = options.target_branch
...@@ -1643,15 +1649,72 @@ def GerritUpload(options, args, cl, change): ...@@ -1643,15 +1649,72 @@ def GerritUpload(options, args, cl, change):
if not change_desc.description: if not change_desc.description:
print "Description is empty; aborting." print "Description is empty; aborting."
return 1 return 1
if CHANGE_ID not in change_desc.description:
AddChangeIdToCommitMessage(options, args)
commits = RunGit(['rev-list', '%s/%s..' % (remote, branch)]).splitlines() if options.squash:
# Try to get the message from a previous upload.
shadow_branch = 'refs/heads/git_cl_uploads/' + cl.GetBranch()
message = RunGitSilent(['show', '--format=%s\n\n%b', '-s', shadow_branch])
if not message:
if not options.force:
change_desc.prompt()
if CHANGE_ID not in change_desc.description:
# Run the commit-msg hook without modifying the head commit by writing
# the commit message to a temporary file and running the hook over it,
# then reading the file back in.
commit_msg_hook = os.path.join(settings.GetRoot(), '.git', 'hooks',
'commit-msg')
file_handle, msg_file = tempfile.mkstemp(text=True,
prefix='commit_msg')
try:
try:
with os.fdopen(file_handle, 'w') as fileobj:
fileobj.write(change_desc.description)
finally:
os.close(file_handle)
RunCommand([commit_msg_hook, msg_file])
change_desc.set_description(gclient_utils.FileRead(msg_file))
finally:
os.remove(msg_file)
if not change_desc.description:
print "Description is empty; aborting."
return 1
message = change_desc.description
remote, upstream_branch = cl.FetchUpstreamTuple(cl.GetBranch())
if remote is '.':
# If our upstream branch is local, we base our squashed commit on its
# squashed version.
parent = ('refs/heads/git_cl_uploads/' +
scm.GIT.ShortBranchName(upstream_branch))
# Verify that the upstream branch has been uploaded too, otherwise Gerrit
# will create additional CLs when uploading.
if (RunGitSilent(['rev-parse', upstream_branch + ':']) !=
RunGitSilent(['rev-parse', parent + ':'])):
print 'Upload upstream branch ' + upstream_branch + ' first.'
return 1
else:
parent = cl.GetCommonAncestorWithUpstream()
tree = RunGit(['rev-parse', 'HEAD:']).strip()
ref_to_push = RunGit(['commit-tree', tree, '-p', parent,
'-m', message]).strip()
else:
if CHANGE_ID not in change_desc.description:
AddChangeIdToCommitMessage(options, args)
ref_to_push = 'HEAD'
parent = '%s/%s' % (gerrit_remote, branch)
commits = RunGitSilent(['rev-list', '%s..%s' % (parent,
ref_to_push)]).splitlines()
if len(commits) > 1: if len(commits) > 1:
print('WARNING: This will upload %d commits. Run the following command ' print('WARNING: This will upload %d commits. Run the following command '
'to see which commits will be uploaded: ' % len(commits)) 'to see which commits will be uploaded: ' % len(commits))
print('git log %s/%s..' % (remote, branch)) print('git log %s..%s' % (parent, ref_to_push))
print('You can also use `git squash-branch` to squash these into a single' print('You can also use `git squash-branch` to squash these into a single '
'commit.') 'commit.')
ask_for_data('About to upload; enter to confirm.') ask_for_data('About to upload; enter to confirm.')
...@@ -1673,8 +1736,13 @@ def GerritUpload(options, args, cl, change): ...@@ -1673,8 +1736,13 @@ def GerritUpload(options, args, cl, change):
if receive_options: if receive_options:
git_command.append('--receive-pack=git receive-pack %s' % git_command.append('--receive-pack=git receive-pack %s' %
' '.join(receive_options)) ' '.join(receive_options))
git_command += [remote, 'HEAD:refs/for/' + branch] git_command += [gerrit_remote, ref_to_push + ':refs/for/' + branch]
RunGit(git_command) RunGit(git_command)
if options.squash:
head = RunGit(['rev-parse', 'HEAD']).strip()
RunGit(['update-ref', '-m', 'Uploaded ' + head, shadow_branch, ref_to_push])
# TODO(ukai): parse Change-Id: and set issue number? # TODO(ukai): parse Change-Id: and set issue number?
return 0 return 0
...@@ -1690,7 +1758,7 @@ def GetTargetRef(remote, remote_branch, target_branch, pending_prefix): ...@@ -1690,7 +1758,7 @@ def GetTargetRef(remote, remote_branch, target_branch, pending_prefix):
""" """
if not (remote and remote_branch): if not (remote and remote_branch):
return None return None
if target_branch: if target_branch:
# Cannonicalize branch references to the equivalent local full symbolic # Cannonicalize branch references to the equivalent local full symbolic
# refs, which are then translated into the remote full symbolic refs # refs, which are then translated into the remote full symbolic refs
...@@ -1716,7 +1784,7 @@ def GetTargetRef(remote, remote_branch, target_branch, pending_prefix): ...@@ -1716,7 +1784,7 @@ def GetTargetRef(remote, remote_branch, target_branch, pending_prefix):
not remote_branch.startswith('refs/remotes/%s/refs' % remote)): not remote_branch.startswith('refs/remotes/%s/refs' % remote)):
# Default to master for refs that are not branches. # Default to master for refs that are not branches.
remote_branch = 'refs/remotes/%s/master' % remote remote_branch = 'refs/remotes/%s/master' % remote
# Create the true path to the remote branch. # Create the true path to the remote branch.
# Does the following translation: # Does the following translation:
# * refs/remotes/origin/refs/diff/test -> refs/diff/test # * refs/remotes/origin/refs/diff/test -> refs/diff/test
...@@ -1896,6 +1964,8 @@ def CMDupload(parser, args): ...@@ -1896,6 +1964,8 @@ def CMDupload(parser, args):
metavar='TARGET', metavar='TARGET',
help='Apply CL to remote ref TARGET. ' + help='Apply CL to remote ref TARGET. ' +
'Default: remote branch head, or master') 'Default: remote branch head, or master')
parser.add_option('--squash', action='store_true',
help='Squash multiple commits into one (Gerrit only)')
parser.add_option('--email', default=None, parser.add_option('--email', default=None,
help='email address to use to connect to Rietveld') help='email address to use to connect to Rietveld')
parser.add_option('--tbr-owners', dest='tbr_owners', action='store_true', parser.add_option('--tbr-owners', dest='tbr_owners', action='store_true',
......
...@@ -571,7 +571,7 @@ class TestGitCl(TestCase): ...@@ -571,7 +571,7 @@ class TestGitCl(TestCase):
] ]
@staticmethod @staticmethod
def _gerrit_upload_calls(description, reviewers): def _gerrit_upload_calls(description, reviewers, squash):
calls = [ calls = [
((['git', 'config', 'gerrit.host'],), ((['git', 'config', 'gerrit.host'],),
'gerrit.example.com'), 'gerrit.example.com'),
...@@ -590,8 +590,29 @@ class TestGitCl(TestCase): ...@@ -590,8 +590,29 @@ class TestGitCl(TestCase):
'fake_ancestor_sha..HEAD'],), 'fake_ancestor_sha..HEAD'],),
description) description)
] ]
if squash:
ref_to_push = 'abcdef0123456789'
calls += [
((['git', 'show', '--format=%s\n\n%b', '-s',
'refs/heads/git_cl_uploads/master'],),
(description, 0)),
((['git', 'config', 'branch.master.merge'],),
'refs/heads/master'),
((['git', 'config', 'branch.master.remote'],),
'origin'),
((['get_or_create_merge_base', 'master', 'master'],),
'origin/master'),
((['git', 'rev-parse', 'HEAD:'],),
'0123456789abcdef'),
((['git', 'commit-tree', '0123456789abcdef', '-p',
'origin/master', '-m', 'd'],),
ref_to_push),
]
else:
ref_to_push = 'HEAD'
calls += [ calls += [
((['git', 'rev-list', 'origin/master..'],), ''), ((['git', 'rev-list', 'origin/master..' + ref_to_push],), ''),
((['git', 'config', 'rietveld.cc'],), '') ((['git', 'config', 'rietveld.cc'],), '')
] ]
receive_pack = '--receive-pack=git receive-pack ' receive_pack = '--receive-pack=git receive-pack '
...@@ -603,19 +624,28 @@ class TestGitCl(TestCase): ...@@ -603,19 +624,28 @@ class TestGitCl(TestCase):
receive_pack += '' receive_pack += ''
calls += [ calls += [
((['git', ((['git',
'push', receive_pack, 'origin', 'HEAD:refs/for/master'],), 'push', receive_pack, 'origin', ref_to_push + ':refs/for/master'],),
'') '')
] ]
if squash:
calls += [
((['git', 'rev-parse', 'HEAD'],), 'abcdef0123456789'),
((['git', 'update-ref', '-m', 'Uploaded abcdef0123456789',
'refs/heads/git_cl_uploads/master', 'abcdef0123456789'],),
'')
]
return calls return calls
def _run_gerrit_upload_test( def _run_gerrit_upload_test(
self, self,
upload_args, upload_args,
description, description,
reviewers): reviewers,
squash=False):
"""Generic gerrit upload test framework.""" """Generic gerrit upload test framework."""
self.calls = self._gerrit_base_calls() self.calls = self._gerrit_base_calls()
self.calls += self._gerrit_upload_calls(description, reviewers) self.calls += self._gerrit_upload_calls(description, reviewers, squash)
git_cl.main(['upload'] + upload_args) git_cl.main(['upload'] + upload_args)
def test_gerrit_upload_without_change_id(self): def test_gerrit_upload_without_change_id(self):
...@@ -643,6 +673,12 @@ class TestGitCl(TestCase): ...@@ -643,6 +673,12 @@ class TestGitCl(TestCase):
'Change-Id:123456789\n', 'Change-Id:123456789\n',
['reviewer@example.com', 'another@example.com']) ['reviewer@example.com', 'another@example.com'])
def test_gerrit_upload_squash(self):
self._run_gerrit_upload_test(
['--squash'],
'desc\n\nBUG=\nChange-Id:123456789\n',
[],
squash=True)
def test_config_gerrit_download_hook(self): def test_config_gerrit_download_hook(self):
self.mock(git_cl, 'FindCodereviewSettingsFile', CodereviewSettingsFileMock) self.mock(git_cl, 'FindCodereviewSettingsFile', CodereviewSettingsFileMock)
...@@ -762,7 +798,7 @@ class TestGitCl(TestCase): ...@@ -762,7 +798,7 @@ class TestGitCl(TestCase):
self.assertEqual(None, git_cl.GetTargetRef(None, self.assertEqual(None, git_cl.GetTargetRef(None,
'refs/remotes/origin/master', 'refs/remotes/origin/master',
'master', None)) 'master', None))
# Check default target refs for branches. # Check default target refs for branches.
self.assertEqual('refs/heads/master', self.assertEqual('refs/heads/master',
git_cl.GetTargetRef('origin', 'refs/remotes/origin/master', git_cl.GetTargetRef('origin', 'refs/remotes/origin/master',
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment