diff --git a/git_cl.py b/git_cl.py index cc1317e2c..e7e3253d5 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1447,6 +1447,62 @@ class Changelist(object): p = subprocess2.Popen(['vpython', PRESUBMIT_SUPPORT] + args) p.wait() + def _GetDescriptionForUpload(self, options, git_diff_args, files): + # Get description message for upload. + if self.GetIssue(): + description = self.FetchDescription() + elif options.message: + description = options.message + else: + description = _create_description_from_log(git_diff_args) + if options.title and options.squash: + description = options.title + '\n\n' + message + + # Extract bug number from branch name. + bug = options.bug + fixed = options.fixed + match = re.match(r'(?Pbug|fix(?:e[sd])?)[_-]?(?P\d+)', + self.GetBranch()) + if not bug and not fixed and match: + if match.group('type') == 'bug': + bug = match.group('bugnum') + else: + fixed = match.group('bugnum') + + change_description = ChangeDescription(description, bug, fixed) + + # Set the reviewer list now so that presubmit checks can access it. + if options.reviewers or options.tbrs or options.add_owners_to: + change_description.update_reviewers( + options.reviewers, options.tbrs, options.add_owners_to, files, + self.GetAuthor()) + + return change_description + + def _GetTitleForUpload(self, options): + # When not squashing, just return options.title. + if not options.squash: + return options.title + + # On first upload, patchset title is always this string, while options.title + # gets converted to first line of message. + if not self.GetIssue(): + return 'Initial upload' + + # When uploading subsequent patchsets, options.message is taken as the title + # if options.title is not provided. + if options.title: + return options.title + if options.message: + return options.message.strip() + + # Use the subject of the last commit as title by default. + title = RunGit( + ['show', '-s', '--format=%s', 'HEAD']).strip() + if options.force: + return title + return ask_for_data('Title for patchset [%s]: ' % title) or title + def CMDUpload(self, options, git_diff_args, orig_args): """Uploads a change to codereview.""" custom_cl_base = None @@ -1466,30 +1522,13 @@ class Changelist(object): self.EnsureAuthenticated(force=options.force) self.EnsureCanUploadPatchset(force=options.force) - # Get description message for upload. - if self.GetIssue(): - description = self.FetchDescription() - elif options.message: - description = options.message - else: - description = _create_description_from_log(git_diff_args) - if options.title and options.squash: - description = options.title + '\n\n' + message - # Apply watchlists on upload. watchlist = watchlists.Watchlists(settings.GetRoot()) files = self.GetAffectedFiles(base_branch) if not options.bypass_watchlists: self.ExtendCC(watchlist.GetWatchersForPaths(files)) - if options.reviewers or options.tbrs or options.add_owners_to: - # Set the reviewer list now so that presubmit checks can access it. - change_description = ChangeDescription(description) - change_description.update_reviewers( - options.reviewers, options.tbrs, options.add_owners_to, files, - self.GetAuthor()) - description = change_description.description - + change_desc = self._GetDescriptionForUpload(options, git_diff_args, files) if not options.bypass_hooks: hook_results = self.RunHook( committing=False, @@ -1497,19 +1536,20 @@ class Changelist(object): verbose=options.verbose, parallel=options.parallel, upstream=base_branch, - description=description, + description=change_desc.description, all_files=False) self.ExtendCC(hook_results['more_cc']) print_stats(git_diff_args) ret = self.CMDUploadChange( - options, git_diff_args, custom_cl_base, description) + options, git_diff_args, custom_cl_base, change_desc) if not ret: self._GitSetBranchConfigValue( 'last-upload-hash', RunGit(['rev-parse', 'HEAD']).strip()) # Run post upload hooks, if specified. if settings.GetRunPostUploadHook(): - self.RunPostUploadHook(options.verbose, base_branch, description) + self.RunPostUploadHook( + options.verbose, base_branch, change_desc.description) # Upload all dependencies if specified. if options.dependencies: @@ -2237,7 +2277,7 @@ class Changelist(object): return push_stdout def CMDUploadChange( - self, options, git_diff_args, custom_cl_base, message): + self, options, git_diff_args, custom_cl_base, change_desc): """Upload the current branch to Gerrit.""" if options.squash is None: # Load default for user, repo, squash=true, in this order. @@ -2245,101 +2285,42 @@ class Changelist(object): remote, remote_branch = self.GetRemoteBranch() branch = GetTargetRef(remote, remote_branch, options.target_branch) - # This may be None; default fallback value is determined in logic below. - title = options.title - - # Extract bug number from branch name. - bug = options.bug - fixed = options.fixed - match = re.match(r'(?Pbug|fix(?:e[sd])?)[_-]?(?P\d+)', - self.GetBranch()) - if not bug and not fixed and match: - if match.group('type') == 'bug': - bug = match.group('bugnum') - else: - fixed = match.group('bugnum') if options.squash: self._GerritCommitMsgHookCheck(offer_removal=not options.force) if self.GetIssue(): - if not title: - if options.message: - # When uploading a subsequent patchset, -m|--message is taken - # as the patchset title if --title was not provided. - title = options.message.strip() - else: - default_title = RunGit( - ['show', '-s', '--format=%s', 'HEAD']).strip() - if options.force: - title = default_title - else: - title = ask_for_data( - 'Title for patchset [%s]: ' % default_title) or default_title - # User requested to change description if options.edit_description: - change_desc = ChangeDescription(message, bug=bug, fixed=fixed) change_desc.prompt() - message = change_desc.description - change_id = self._GetChangeDetail()['change_id'] - - # Make sure that the Change-Id in the description matches the one - # fetched from Gerrit. - footer_change_ids = git_footers.get_footer_change_id(message) - if footer_change_ids != [change_id]: - if footer_change_ids: - # Remove any existing Change-Id footers since they don't match the - # expected change_id footer. - message = git_footers.remove_footer(message, 'Change-Id') - # Add the expected Change-Id footer. - message = git_footers.add_footer_change_id(message, change_id) - print('WARNING: Change-Id has been set to Change-Id fetched from ' - 'Gerrit. Use `git cl issue 0` if you want to clear it and ' - 'set a new one.') - change_desc = ChangeDescription(message, bug=bug, fixed=fixed) + change_desc.ensure_change_id(change_id) else: # if not self.GetIssue() - change_desc = ChangeDescription(message, bug=bug, fixed=fixed) if not options.force: change_desc.prompt() - - # On first upload, patchset title is always this string, while - # --title flag gets converted to first line of message. - title = 'Initial upload' - if not change_desc.description: - DieWithError("Description is empty. Aborting...") change_ids = git_footers.get_footer_change_id(change_desc.description) - if len(change_ids) > 1: - DieWithError('too many Change-Id footers, at most 1 allowed.') - if not change_ids: - # Generate the Change-Id automatically. - change_desc.set_description(git_footers.add_footer_change_id( - change_desc.description, - GenerateGerritChangeId(change_desc.description))) - change_ids = git_footers.get_footer_change_id(change_desc.description) - assert len(change_ids) == 1 - change_id = change_ids[0] + if len(change_ids) == 1: + change_id = change_ids[0] + else: + change_id = GenerateGerritChangeId(change_desc.description) + change_desc.ensure_change_id(change_id) if options.preserve_tryjobs: change_desc.set_preserve_tryjobs() remote, upstream_branch = self.FetchUpstreamTuple(self.GetBranch()) - parent = self._ComputeParent(remote, upstream_branch, custom_cl_base, - options.force, change_desc) + parent = self._ComputeParent( + remote, upstream_branch, custom_cl_base, options.force, change_desc) tree = RunGit(['rev-parse', 'HEAD:']).strip() with gclient_utils.temporary_file() as desc_tempfile: gclient_utils.FileWrite(desc_tempfile, change_desc.description) ref_to_push = RunGit( ['commit-tree', tree, '-p', parent, '-F', desc_tempfile]).strip() else: # if not options.squash - change_desc = ChangeDescription(message) - 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(message, git_diff_args)) + self._AddChangeIdToCommitMessage( + change_desc.description, git_diff_args)) ref_to_push = 'HEAD' # For no-squash mode, we assume the remote called "origin" is the one we # want. It is not worthwhile to support different workflows for @@ -2347,7 +2328,6 @@ class Changelist(object): parent = 'origin/%s' % branch change_id = git_footers.get_footer_change_id(change_desc.description)[0] - assert change_desc SaveDescriptionBackup(change_desc) commits = RunGitSilent(['rev-list', '%s..%s' % (parent, ref_to_push)]).splitlines() @@ -2399,6 +2379,7 @@ class Changelist(object): # TODO(tandrii): options.message should be posted as a comment # if --send-mail is set on non-initial upload as Rietveld used to do it. + title = self._GetTitleForUpload(options) if title: # Punctuation and whitespace in |title| must be percent-encoded. refspec_opts.append('m=' + gerrit_util.PercentEncodeForGitRef(title)) @@ -2673,6 +2654,21 @@ class ChangeDescription(object): lines.pop(-1) self._description_lines = lines + def ensure_change_id(self, change_id): + description = self.description + footer_change_ids = git_footers.get_footer_change_id(description) + # Make sure that the Change-Id in the description matches the given one. + if footer_change_ids != [change_id]: + if footer_change_ids: + # Remove any existing Change-Id footers since they don't match the + # expected change_id footer. + description = git_footers.remove_footer(description, 'Change-Id') + print('WARNING: Change-Id has been set to %s. Use `git cl issue 0` ' + 'if you want to set a new one.') + # Add the expected Change-Id footer. + description = git_footers.add_footer_change_id(description, change_id) + self.set_description(description) + def update_reviewers( self, reviewers, tbrs, add_owners_to, affected_files, author_email): """Rewrites the R=/TBR= line(s) as a single line each. diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 9b3cdb068..05d51465b 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -850,12 +850,11 @@ class TestGitCl(unittest.TestCase): def _gerrit_upload_calls(self, description, reviewers, squash, squash_mode='default', - expected_upstream_ref='origin/refs/heads/master', title=None, notify=False, post_amend_description=None, issue=None, cc=None, custom_cl_base=None, tbr=None, short_hostname='chromium', - labels=None, change_id=None, original_title=None, + labels=None, change_id=None, final_description=None, gitcookies_exists=True, force=False, edit_description=None): if post_amend_description is None: @@ -868,23 +867,12 @@ class TestGitCl(unittest.TestCase): self.mockGit.config['gerrit.override-squash-uploads'] = ( 'true' if squash_mode == 'override_squash' else 'false') - # If issue is given, then description is fetched from Gerrit instead. - if issue is None: - if squash: - 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' if not git_footers.get_footer_change_id(description) and not squash: calls += [ (('DownloadGerritHook', False), ''), ] if squash: - if not force and not issue: + if not issue and not force: calls += [ ((['RunEditor'],), description), ] @@ -923,13 +911,11 @@ class TestGitCl(unittest.TestCase): ] else: ref_to_push = 'HEAD' + parent = 'origin/refs/heads/master' calls += [ (('SaveDescriptionBackup',), None), - ((['git', 'rev-list', - (custom_cl_base if custom_cl_base else expected_upstream_ref) + '..' + - ref_to_push],), - '1hashPerLine\n'), + ((['git', 'rev-list', parent + '..' + ref_to_push],),'1hashPerLine\n'), ] metrics_arguments = [] @@ -945,8 +931,19 @@ class TestGitCl(unittest.TestCase): ref_suffix = '%notify=NONE' metrics_arguments.append('notify=NONE') + # If issue is given, then description is fetched from Gerrit instead. + if issue is None: + if squash: + 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' if title: - ref_suffix += ',m=' + title + ref_suffix += ',m=' + gerrit_util.PercentEncodeForGitRef(title) metrics_arguments.append('m') if short_hostname == 'chromium': @@ -1029,7 +1026,6 @@ class TestGitCl(unittest.TestCase): ] final_description = final_description or post_amend_description.strip() - original_title = original_title or title or '' # Trace-related calls calls += [ # Write a description with context for the current trace. @@ -1045,7 +1041,7 @@ class TestGitCl(unittest.TestCase): 'short_hostname': short_hostname, 'change_id': change_id, 'description': final_description, - 'title': original_title, + 'title': title or '', 'trace_name': 'TRACES_DIR/20170316T200041.000000', }],), None, @@ -1118,7 +1114,6 @@ class TestGitCl(unittest.TestCase): reviewers=None, squash=True, squash_mode=None, - expected_upstream_ref='origin/refs/heads/master', title=None, notify=False, post_amend_description=None, @@ -1131,7 +1126,6 @@ class TestGitCl(unittest.TestCase): short_hostname='chromium', labels=None, change_id=None, - original_title=None, final_description=None, gitcookies_exists=True, force=False, @@ -1186,6 +1180,8 @@ class TestGitCl(unittest.TestCase): mock.patch( 'git_cl.Changelist._AddChangeIdToCommitMessage', return_value=post_amend_description or description).start() + mock.patch( + 'git_cl.GenerateGerritChangeId', return_value=change_id).start() mock.patch( 'git_cl.ask_for_data', lambda prompt: self._mocked_call('ask_for_data', prompt)).start() @@ -1212,7 +1208,6 @@ class TestGitCl(unittest.TestCase): self.calls += self._gerrit_upload_calls( description, reviewers, squash, squash_mode=squash_mode, - expected_upstream_ref=expected_upstream_ref, title=title, notify=notify, post_amend_description=post_amend_description, issue=issue, cc=cc, @@ -1220,7 +1215,6 @@ class TestGitCl(unittest.TestCase): short_hostname=short_hostname, labels=labels, change_id=change_id, - original_title=original_title, final_description=final_description, gitcookies_exists=gitcookies_exists, force=force, @@ -1245,6 +1239,13 @@ class TestGitCl(unittest.TestCase): gitcookies_exists=False) def test_gerrit_upload_without_change_id(self): + self._run_gerrit_upload_test( + [], + 'desc ✔\n\nBUG=\n\nChange-Id: Ixxx', + [], + change_id='Ixxx') + + def test_gerrit_upload_without_change_id_nosquash(self): self._run_gerrit_upload_test( ['--no-squash'], 'desc ✔\n\nBUG=\n', @@ -1289,15 +1290,14 @@ class TestGitCl(unittest.TestCase): 'desc ✔\n\nBUG=\n\nChange-Id: I123456789', squash=False, squash_mode='override_nosquash', - title='We%27ll_escape_%5E%5F_%5E_special_chars%2E%2E%2E%40%7Bu%7D', change_id='I123456789', - original_title='We\'ll escape ^_ ^ special chars...@{u}') + title='We\'ll escape ^_ ^ special chars...@{u}') def test_gerrit_reviewers_cmd_line(self): self._run_gerrit_upload_test( ['-r', 'foo@example.com', '--send-mail'], 'desc ✔\n\nBUG=\n\nChange-Id: I123456789', - ['foo@example.com'], + reviewers=['foo@example.com'], squash=False, squash_mode='override_nosquash', notify=True, @@ -1311,9 +1311,7 @@ class TestGitCl(unittest.TestCase): u'desc=\n\nBug: 10000\nChange-Id: Ixxx', [], force=True, - expected_upstream_ref='origin/master', fetched_description='desc=\n\nChange-Id: Ixxx', - original_title='Initial upload', change_id='Ixxx') def test_gerrit_upload_corrects_wrong_change_id(self): @@ -1322,10 +1320,8 @@ class TestGitCl(unittest.TestCase): u'desc=\n\nBug: 10000\nChange-Id: Ixxxx', [], issue='123456', - expected_upstream_ref='origin/master', edit_description='desc=\n\nBug: 10000\nChange-Id: Izzzz', fetched_description='desc=\n\nChange-Id: Ixxxx', - original_title='Title', title='Title', change_id='Ixxxx') @@ -1335,9 +1331,7 @@ class TestGitCl(unittest.TestCase): u'desc=\n\nFixed: 10000\nChange-Id: Ixxx', [], force=True, - expected_upstream_ref='origin/master', fetched_description='desc=\n\nChange-Id: Ixxx', - original_title='Initial upload', change_id='Ixxx') def test_gerrit_reviewer_multiple(self): @@ -1349,21 +1343,17 @@ class TestGitCl(unittest.TestCase): 'CC=more@example.com,people@example.com\n\n' 'Change-Id: 123456789', ['reviewer@example.com', 'another@example.com'], - expected_upstream_ref='origin/master', cc=['more@example.com', 'people@example.com'], tbr='reviewer@example.com', labels={'Code-Review': 2}, - change_id='123456789', - original_title='Initial upload') + change_id='123456789') def test_gerrit_upload_squash_first_is_default(self): self._run_gerrit_upload_test( [], 'desc ✔\nBUG=\n\nChange-Id: 123456789', [], - expected_upstream_ref='origin/master', - change_id='123456789', - original_title='Initial upload') + change_id='123456789') def test_gerrit_upload_squash_first(self): self._run_gerrit_upload_test( @@ -1371,9 +1361,7 @@ class TestGitCl(unittest.TestCase): 'desc ✔\nBUG=\n\nChange-Id: 123456789', [], squash=True, - expected_upstream_ref='origin/master', - change_id='123456789', - original_title='Initial upload') + change_id='123456789') def test_gerrit_upload_squash_first_with_labels(self): self._run_gerrit_upload_test( @@ -1381,10 +1369,8 @@ class TestGitCl(unittest.TestCase): 'desc ✔\nBUG=\n\nChange-Id: 123456789', [], squash=True, - expected_upstream_ref='origin/master', labels={'Commit-Queue': 1, 'Auto-Submit': 1}, - change_id='123456789', - original_title='Initial upload') + change_id='123456789') def test_gerrit_upload_squash_first_against_rev(self): custom_cl_base = 'custom_cl_base_rev_or_branch' @@ -1393,10 +1379,8 @@ class TestGitCl(unittest.TestCase): 'desc ✔\nBUG=\n\nChange-Id: 123456789', [], squash=True, - expected_upstream_ref='origin/master', custom_cl_base=custom_cl_base, - change_id='123456789', - original_title='Initial upload') + change_id='123456789') self.assertIn( 'If you proceed with upload, more than 1 CL may be created by Gerrit', sys.stdout.getvalue()) @@ -1408,10 +1392,8 @@ class TestGitCl(unittest.TestCase): description, [], squash=True, - expected_upstream_ref='origin/master', issue=123456, - change_id='123456789', - original_title='User input') + change_id='123456789') @mock.patch('sys.stderr', StringIO()) def test_gerrit_upload_squash_reupload_to_abandoned(self): @@ -1422,7 +1404,6 @@ class TestGitCl(unittest.TestCase): description, [], squash=True, - expected_upstream_ref='origin/master', issue=123456, fetched_status='ABANDONED', change_id='123456789') @@ -1441,11 +1422,9 @@ class TestGitCl(unittest.TestCase): description, [], squash=True, - expected_upstream_ref='origin/master', issue=123456, other_cl_owner='other@example.com', - change_id='123456789', - original_title='User input') + change_id='123456789') self.assertIn( 'WARNING: Change 123456 is owned by other@example.com, but you ' 'authenticate to Gerrit as yet-another@example.com.\n' @@ -1461,10 +1440,8 @@ class TestGitCl(unittest.TestCase): [], fetched_description=fetched_description, squash=True, - expected_upstream_ref='origin/master', issue=123456, change_id='123456789', - original_title='User input', edit_description=description) @mock.patch('git_cl.RunGit')