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

Revert "git cl upload for Gerrit: use push options instead of refspec."

Browse Source 
This reverts commit 0267fd2c29.

Reason for revert: seems to have changed some behavior as reported by SKIA.

Original change's description:
> git cl upload for Gerrit: use push options instead of refspec.
> 
> This removes limitation of no special chars in patchset titles.
> 
> BUG=chromium:663787,chromium:707963,gerrit:5184
> R=​sergiyb@chromium.org,agable@chromium.org
> TEST=uploaded this CL using depot_tools with this patch :)
> 
> Change-Id: I5d684d0a0aa286a45ff99cca6d57aefa8436cd0f
> Reviewed-on: https://chromium-review.googlesource.com/468926
> Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
> Reviewed-by: Sergiy Byelozyorov <sergiyb@google.com>
> 

TBR=agable@chromium.org,sergiyb@google.com,tandrii@chromium.org,sergiyb@chromium.org,borenet@chromium.org,chromium-reviews@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=chromium:663787,chromium:707963,gerrit:5184

Change-Id: I3306091b14b97a200150389d0480b69120af8c61
Reviewed-on: https://chromium-review.googlesource.com/469006
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
changes/06/469006/2
Andrii Shyshkalov 8 years ago committed by Commit Bot
parent 0267fd2c29
commit febbae9d2e
2 changed files with 79 additions and 43 deletions
Show Stats Download Patch File Download Diff File

47
git_cl.py
Unescape Escape View File

@ -2815,6 +2815,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
# This may be None; default fallback value is determined in logic below. # This may be None; default fallback value is determined in logic below.
title = options.title title = options.title
automatic_title = False
if options.squash: if options.squash:
self._GerritCommitMsgHookCheck(offer_removal=not options.force) self._GerritCommitMsgHookCheck(offer_removal=not options.force)
@ -2829,6 +2830,8 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
default_title = RunGit(['show', '-s', '--format=%s', 'HEAD']).strip() default_title = RunGit(['show', '-s', '--format=%s', 'HEAD']).strip()
title = ask_for_data( title = ask_for_data(
'Title for patchset [%s]: ' % default_title) or default_title 'Title for patchset [%s]: ' % default_title) or default_title
if title == default_title:
automatic_title = True
change_id = self._GetChangeDetail()['change_id'] change_id = self._GetChangeDetail()['change_id']
while True: while True:
footer_change_ids = git_footers.get_footer_change_id(message) footer_change_ids = git_footers.get_footer_change_id(message)
@ -2877,6 +2880,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
# On first upload, patchset title is always this string, while # On first upload, patchset title is always this string, while
# --title flag gets converted to first line of message. # --title flag gets converted to first line of message.
title = 'Initial upload' title = 'Initial upload'
automatic_title = True
if not change_desc.description: if not change_desc.description:
DieWithError("Description is empty. Aborting...") DieWithError("Description is empty. Aborting...")
message = change_desc.description message = change_desc.description
@ -2928,22 +2932,29 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
# Extra options that can be specified at push time. Doc: # Extra options that can be specified at push time. Doc:
# https://gerrit-review.googlesource.com/Documentation/user-upload.html # https://gerrit-review.googlesource.com/Documentation/user-upload.html
# This can be done either through use of refspec OR by using --push-option refspec_opts = []
# as documented here: https://git-scm.com/docs/git-push#git-push--o .
push_opts = []
if change_desc.get_reviewers(tbr_only=True): if change_desc.get_reviewers(tbr_only=True):
print('Adding self-LGTM (Code-Review +1) because of TBRs') print('Adding self-LGTM (Code-Review +1) because of TBRs')
push_opts.append('l=Code-Review+1') refspec_opts.append('l=Code-Review+1')
if title: if title:
push_opts.append('m=' + title) if not re.match(r'^[\w ]+$', title):
title = re.sub(r'[^\w ]', '', title)
if not automatic_title:
print('WARNING: Patchset title may only contain alphanumeric chars '
'and spaces. You can edit it in the UI. '
'See https://crbug.com/663787.\n'
'Cleaned up title: %s' % title)
# Per doc, spaces must be converted to underscores, and Gerrit will do the
# reverse on its side.
refspec_opts.append('m=' + title.replace(' ', '_'))
if options.send_mail: if options.send_mail:
if not change_desc.get_reviewers(): if not change_desc.get_reviewers():
DieWithError('Must specify reviewers to send email.', change_desc) DieWithError('Must specify reviewers to send email.', change_desc)
push_opts.append('notify=ALL') refspec_opts.append('notify=ALL')
else: else:
push_opts.append('notify=NONE') refspec_opts.append('notify=NONE')
reviewers = change_desc.get_reviewers() reviewers = change_desc.get_reviewers()
if reviewers: if reviewers:
@ -2951,24 +2962,28 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
# side for real (b/34702620). # side for real (b/34702620).
def clean_invisible_chars(email): def clean_invisible_chars(email):
return email.decode('unicode_escape').encode('ascii', 'ignore') return email.decode('unicode_escape').encode('ascii', 'ignore')
push_opts.extend('r=' + clean_invisible_chars(email).strip() refspec_opts.extend('r=' + clean_invisible_chars(email).strip()
for email in reviewers) for email in reviewers)
if options.private: if options.private:
push_opts.append('draft') refspec_opts.append('draft')
if options.topic: if options.topic:
# Documentation on Gerrit topics is here: # Documentation on Gerrit topics is here:
# https://gerrit-review.googlesource.com/Documentation/user-upload.html#topic # https://gerrit-review.googlesource.com/Documentation/user-upload.html#topic
push_opts.append('topic=%s' % options.topic) refspec_opts.append('topic=%s' % options.topic)
refspec_suffix = ''
if refspec_opts:
refspec_suffix = '%' + ','.join(refspec_opts)
assert ' ' not in refspec_suffix, (
'spaces not allowed in refspec: "%s"' % refspec_suffix)
refspec = '%s:refs/for/%s%s' % (ref_to_push, branch, refspec_suffix)
push_cmd = ['git', 'push']
for opt in sorted(push_opts): # Sort to make tests deterministic.
push_cmd += ['-o', opt]
push_cmd += [self.GetRemoteUrl(), '%s:refs/for/%s' % (ref_to_push, branch)]
try: try:
push_stdout = gclient_utils.CheckCallAndFilter( push_stdout = gclient_utils.CheckCallAndFilter(
push_cmd, print_stdout=True, ['git', 'push', self.GetRemoteUrl(), refspec],
print_stdout=True,
# Flush after every line: useful for seeing progress when running as # Flush after every line: useful for seeing progress when running as
# recipe. # recipe.
filter_fn=lambda _: sys.stdout.flush()) filter_fn=lambda _: sys.stdout.flush())

75
tests/git_cl_test.py
Unescape Escape View File

@ -7,7 +7,6 @@
import contextlib import contextlib
import datetime import datetime
import itertools
import json import json
import logging import logging
import os import os
@ -1416,14 +1415,13 @@ class TestGitCl(TestCase):
def _gerrit_upload_calls(cls, description, reviewers, squash, def _gerrit_upload_calls(cls, description, reviewers, squash,
squash_mode='default', squash_mode='default',
expected_upstream_ref='origin/refs/heads/master', expected_upstream_ref='origin/refs/heads/master',
push_opts=None, title=None, notify=False, ref_suffix='', title=None, notify=False,
post_amend_description=None, issue=None, cc=None, post_amend_description=None, issue=None, cc=None,
git_mirror=None): git_mirror=None):
if post_amend_description is None: if post_amend_description is None:
post_amend_description = description post_amend_description = description
calls = [] calls = []
cc = cc or [] cc = cc or []
push_opts = push_opts or []
if squash_mode == 'default': if squash_mode == 'default':
calls.extend([ calls.extend([
@ -1445,14 +1443,14 @@ class TestGitCl(TestCase):
'fake_ancestor_sha..HEAD'],), 'fake_ancestor_sha..HEAD'],),
description)] description)]
if squash: if squash:
title = 'Initial upload' title = 'Initial_upload'
else: else:
if not title: if not title:
calls += [ calls += [
((['git', 'show', '-s', '--format=%s', 'HEAD'],), ''), ((['git', 'show', '-s', '--format=%s', 'HEAD'],), ''),
(('ask_for_data', 'Title for patchset []: '), 'User input'), (('ask_for_data', 'Title for patchset []: '), 'User input'),
] ]
title = 'User input' title = 'User_input'
if not git_footers.get_footer_change_id(description) and not squash: if not git_footers.get_footer_change_id(description) and not squash:
calls += [ calls += [
# DownloadGerritHook(False) # DownloadGerritHook(False)
@ -1499,10 +1497,20 @@ class TestGitCl(TestCase):
] ]
if title: if title:
push_opts += ['m=' + title] if ref_suffix:
ref_suffix += ',m=' + title
else:
ref_suffix = '%m=' + title
notify_suffix = 'notify=%s' % ('ALL' if notify else 'NONE')
if ref_suffix:
ref_suffix += ',' + notify_suffix
else:
ref_suffix = '%' + notify_suffix
push_opts += ['notify=%s' % ('ALL' if notify else 'NONE')] if reviewers:
push_opts += ['r=%s' % email for email in sorted(reviewers or [])] ref_suffix += ',' + ','.join('r=%s' % email
for email in sorted(reviewers))
if git_mirror is None: if git_mirror is None:
calls += [ calls += [
@ -1521,23 +1529,22 @@ class TestGitCl(TestCase):
] ]
calls += [ calls += [
((['git', 'push'] + ((['git', 'push',
list(itertools.chain(*(['-o', opt] for opt in sorted(push_opts)))) + 'https://chromium.googlesource.com/yyy/zzz',
['https://chromium.googlesource.com/yyy/zzz', ref_to_push + ':refs/for/refs/heads/master' + ref_suffix],),
ref_to_push + ':refs/for/refs/heads/master'],),
('remote:\n' ('remote:\n'
'remote: Processing changes: (\)\n' 'remote: Processing changes: (\)\n'
'remote: Processing changes: (|)\n' 'remote: Processing changes: (|)\n'
'remote: Processing changes: (/)\n' 'remote: Processing changes: (/)\n'
'remote: Processing changes: (-)\n' 'remote: Processing changes: (-)\n'
'remote: Processing changes: new: 1 (/)\n' 'remote: Processing changes: new: 1 (/)\n'
'remote: Processing changes: new: 1, done\n' 'remote: Processing changes: new: 1, done\n'
'remote:\n' 'remote:\n'
'remote: New Changes:\n' 'remote: New Changes:\n'
'remote: https://chromium-review.googlesource.com/123456 XXX.\n' 'remote: https://chromium-review.googlesource.com/123456 XXX.\n'
'remote:\n' 'remote:\n'
'To https://chromium.googlesource.com/yyy/zzz\n' 'To https://chromium.googlesource.com/yyy/zzz\n'
' * [new branch] hhhh -> refs/for/refs/heads/master\n')), ' * [new branch] hhhh -> refs/for/refs/heads/master\n')),
] ]
if squash: if squash:
calls += [ calls += [
@ -1565,7 +1572,7 @@ class TestGitCl(TestCase):
squash=True, squash=True,
squash_mode=None, squash_mode=None,
expected_upstream_ref='origin/refs/heads/master', expected_upstream_ref='origin/refs/heads/master',
push_opts=None, ref_suffix='',
title=None, title=None,
notify=False, notify=False,
post_amend_description=None, post_amend_description=None,
@ -1612,7 +1619,7 @@ class TestGitCl(TestCase):
description, reviewers, squash, description, reviewers, squash,
squash_mode=squash_mode, squash_mode=squash_mode,
expected_upstream_ref=expected_upstream_ref, expected_upstream_ref=expected_upstream_ref,
push_opts=push_opts, title=title, notify=notify, ref_suffix=ref_suffix, title=title, notify=notify,
post_amend_description=post_amend_description, post_amend_description=post_amend_description,
issue=issue, cc=cc, git_mirror=git_mirror) issue=issue, cc=cc, git_mirror=git_mirror)
# Uncomment when debugging. # Uncomment when debugging.
@ -1645,6 +1652,20 @@ class TestGitCl(TestCase):
squash=False, squash=False,
squash_mode='override_nosquash') squash_mode='override_nosquash')
def test_gerrit_patch_bad_chars(self):
self.mock(git_cl.sys, 'stdout', StringIO.StringIO())
self._run_gerrit_upload_test(
['-f', '-t', 'Don\'t put bad cha,.rs'],
'desc\n\nBUG=\n\nChange-Id: I123456789',
squash=False,
squash_mode='override_nosquash',
title='Dont_put_bad_chars')
self.assertIn(
'WARNING: Patchset title may only contain alphanumeric chars '
'and spaces. You can edit it in the UI. See https://crbug.com/663787.\n'
'Cleaned up title: Dont put bad chars\n',
git_cl.sys.stdout.getvalue())
def test_gerrit_reviewers_cmd_line(self): def test_gerrit_reviewers_cmd_line(self):
self._run_gerrit_upload_test( self._run_gerrit_upload_test(
['-r', 'foo@example.com', '--send-mail'], ['-r', 'foo@example.com', '--send-mail'],
@ -1663,7 +1684,7 @@ class TestGitCl(TestCase):
['reviewer@example.com', 'another@example.com'], ['reviewer@example.com', 'another@example.com'],
squash=False, squash=False,
squash_mode='override_nosquash', squash_mode='override_nosquash',
push_opts=['l=Code-Review+1'], ref_suffix='%l=Code-Review+1',
cc=['more@example.com', 'people@example.com']) cc=['more@example.com', 'people@example.com'])
def test_gerrit_upload_squash_first_is_default(self): def test_gerrit_upload_squash_first_is_default(self):

Write Preview
Loading…
Cancel
Save