From febbae9d2edf9b7861dcee602ae6eaecb25d3e09 Mon Sep 17 00:00:00 2001 From: Andrii Shyshkalov Date: Wed, 5 Apr 2017 15:05:20 +0000 Subject: [PATCH] Revert "git cl upload for Gerrit: use push options instead of refspec." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 0267fd2c290a4ca2300d58abf53af32f8324e285. 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 > Reviewed-by: Sergiy Byelozyorov > 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 Commit-Queue: Andrii Shyshkalov --- git_cl.py | 47 +++++++++++++++++---------- tests/git_cl_test.py | 75 ++++++++++++++++++++++++++++---------------- 2 files changed, 79 insertions(+), 43 deletions(-) diff --git a/git_cl.py b/git_cl.py index e2732533c..f8c536a7e 100755 --- a/git_cl.py +++ b/git_cl.py @@ -2815,6 +2815,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): # This may be None; default fallback value is determined in logic below. title = options.title + automatic_title = False if options.squash: self._GerritCommitMsgHookCheck(offer_removal=not options.force) @@ -2829,6 +2830,8 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): default_title = RunGit(['show', '-s', '--format=%s', 'HEAD']).strip() title = ask_for_data( 'Title for patchset [%s]: ' % default_title) or default_title + if title == default_title: + automatic_title = True change_id = self._GetChangeDetail()['change_id'] while True: 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 # --title flag gets converted to first line of message. title = 'Initial upload' + automatic_title = True if not change_desc.description: DieWithError("Description is empty. Aborting...") message = change_desc.description @@ -2928,22 +2932,29 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): # Extra options that can be specified at push time. Doc: # https://gerrit-review.googlesource.com/Documentation/user-upload.html - # This can be done either through use of refspec OR by using --push-option - # as documented here: https://git-scm.com/docs/git-push#git-push--o . - push_opts = [] + refspec_opts = [] if change_desc.get_reviewers(tbr_only=True): 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: - 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 not change_desc.get_reviewers(): DieWithError('Must specify reviewers to send email.', change_desc) - push_opts.append('notify=ALL') + refspec_opts.append('notify=ALL') else: - push_opts.append('notify=NONE') + refspec_opts.append('notify=NONE') reviewers = change_desc.get_reviewers() if reviewers: @@ -2951,24 +2962,28 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): # side for real (b/34702620). def clean_invisible_chars(email): return email.decode('unicode_escape').encode('ascii', 'ignore') - push_opts.extend('r=' + clean_invisible_chars(email).strip() - for email in reviewers) + refspec_opts.extend('r=' + clean_invisible_chars(email).strip() + for email in reviewers) if options.private: - push_opts.append('draft') + refspec_opts.append('draft') if options.topic: # Documentation on Gerrit topics is here: # 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: 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 # recipe. filter_fn=lambda _: sys.stdout.flush()) diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 32c6c2905..cfae719a5 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -7,7 +7,6 @@ import contextlib import datetime -import itertools import json import logging import os @@ -1416,14 +1415,13 @@ class TestGitCl(TestCase): def _gerrit_upload_calls(cls, description, reviewers, squash, squash_mode='default', 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, git_mirror=None): if post_amend_description is None: post_amend_description = description calls = [] cc = cc or [] - push_opts = push_opts or [] if squash_mode == 'default': calls.extend([ @@ -1445,14 +1443,14 @@ class TestGitCl(TestCase): 'fake_ancestor_sha..HEAD'],), description)] if squash: - title = 'Initial upload' + title = 'Initial_upload' else: if not title: calls += [ ((['git', 'show', '-s', '--format=%s', 'HEAD'],), ''), (('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: calls += [ # DownloadGerritHook(False) @@ -1499,10 +1497,20 @@ class TestGitCl(TestCase): ] 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')] - push_opts += ['r=%s' % email for email in sorted(reviewers or [])] + if reviewers: + ref_suffix += ',' + ','.join('r=%s' % email + for email in sorted(reviewers)) if git_mirror is None: calls += [ @@ -1521,23 +1529,22 @@ class TestGitCl(TestCase): ] calls += [ - ((['git', 'push'] + - list(itertools.chain(*(['-o', opt] for opt in sorted(push_opts)))) + - ['https://chromium.googlesource.com/yyy/zzz', - ref_to_push + ':refs/for/refs/heads/master'],), + ((['git', 'push', + 'https://chromium.googlesource.com/yyy/zzz', + ref_to_push + ':refs/for/refs/heads/master' + ref_suffix],), ('remote:\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, done\n' - 'remote:\n' - 'remote: New Changes:\n' - 'remote: https://chromium-review.googlesource.com/123456 XXX.\n' - 'remote:\n' - 'To https://chromium.googlesource.com/yyy/zzz\n' - ' * [new branch] hhhh -> refs/for/refs/heads/master\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, done\n' + 'remote:\n' + 'remote: New Changes:\n' + 'remote: https://chromium-review.googlesource.com/123456 XXX.\n' + 'remote:\n' + 'To https://chromium.googlesource.com/yyy/zzz\n' + ' * [new branch] hhhh -> refs/for/refs/heads/master\n')), ] if squash: calls += [ @@ -1565,7 +1572,7 @@ class TestGitCl(TestCase): squash=True, squash_mode=None, expected_upstream_ref='origin/refs/heads/master', - push_opts=None, + ref_suffix='', title=None, notify=False, post_amend_description=None, @@ -1612,7 +1619,7 @@ class TestGitCl(TestCase): description, reviewers, squash, squash_mode=squash_mode, 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, issue=issue, cc=cc, git_mirror=git_mirror) # Uncomment when debugging. @@ -1645,6 +1652,20 @@ class TestGitCl(TestCase): squash=False, 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): self._run_gerrit_upload_test( ['-r', 'foo@example.com', '--send-mail'], @@ -1663,7 +1684,7 @@ class TestGitCl(TestCase): ['reviewer@example.com', 'another@example.com'], squash=False, squash_mode='override_nosquash', - push_opts=['l=Code-Review+1'], + ref_suffix='%l=Code-Review+1', cc=['more@example.com', 'people@example.com']) def test_gerrit_upload_squash_first_is_default(self):