From 78936cbc80517cb41896cb5004dc7253b428c83f Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Thu, 11 Apr 2013 00:17:52 +0000 Subject: [PATCH] Switch ChangeDescription usage to be stricter. Include all the preparatory work to eventually update the R= line to match the actual reviewers. The goal here is that ChangeDescription becomes the official implementation for handling and modifying commit messages accross git-cl, gcl and the commit queue. This change does slightly tweak the spacing between the hot lines. It is done on purpose to make them consistent. R=dpranke@chromium.org BUG= Review URL: https://chromiumcodereview.appspot.com/13741015 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@193514 0039d316-1c4b-4281-b951-d872f2087c98 --- git_cl.py | 180 ++++++++++++++++++++++++++----------------- tests/git_cl_test.py | 72 ++++++++++++----- 2 files changed, 160 insertions(+), 92 deletions(-) diff --git a/git_cl.py b/git_cl.py index 35cfea3b7d..64cd9e86f0 100755 --- a/git_cl.py +++ b/git_cl.py @@ -54,7 +54,8 @@ def DieWithError(message): def RunCommand(args, error_ok=False, error_message=None, **kwargs): try: return subprocess2.check_output(args, shell=False, **kwargs) - except subprocess2.CalledProcessError, e: + except subprocess2.CalledProcessError as e: + logging.debug('Failed running %s', args) if not error_ok: DieWithError( 'Command "%s" failed.\n%s' % ( @@ -797,44 +798,79 @@ def GetCodereviewSettingsInteractively(): class ChangeDescription(object): """Contains a parsed form of the change description.""" - def __init__(self, log_desc, reviewers): - self.log_desc = log_desc - self.reviewers = reviewers - self.description = self.log_desc - - def Prompt(self): - content = """# Enter a description of the change. -# This will displayed on the codereview site. -# The first line will also be used as the subject of the review. -""" - content += self.description - if ('\nR=' not in self.description and - '\nTBR=' not in self.description and - self.reviewers): - content += '\nR=' + self.reviewers - if '\nBUG=' not in self.description: - content += '\nBUG=' - content = content.rstrip('\n') + '\n' - content = gclient_utils.RunEditor(content, True) + R_LINE = r'^\s*(TBR|R)\s*=\s*(.+)\s*$' + + def __init__(self, description): + self._description = (description or '').strip() + + @property + def description(self): + return self._description + + def update_reviewers(self, reviewers): + """Rewrites the R=/TBR= line(s) as a single line.""" + assert isinstance(reviewers, list), reviewers + if not reviewers: + return + regexp = re.compile(self.R_LINE, re.MULTILINE) + matches = list(regexp.finditer(self._description)) + is_tbr = any(m.group(1) == 'TBR' for m in matches) + if len(matches) > 1: + # Erase all except the first one. + for i in xrange(len(matches) - 1, 0, -1): + self._description = ( + self._description[:matches[i].start()] + + self._description[matches[i].end()+1:]) + + if is_tbr: + new_r_line = 'TBR=' + ', '.join(reviewers) + else: + new_r_line = 'R=' + ', '.join(reviewers) + + if matches: + self._description = ( + self._description[:matches[0].start()] + new_r_line + + self._description[matches[0].end()+1:]) + else: + self.append_footer(new_r_line) + + def prompt(self): + """Asks the user to update the description.""" + self._description = ( + '# Enter a description of the change.\n' + '# This will displayed on the codereview site.\n' + '# The first line will also be used as the subject of the review.\n' + ) + self._description + + if '\nBUG=' not in self._description: + self.append_footer('BUG=') + content = gclient_utils.RunEditor(self._description, True) if not content: DieWithError('Running editor failed') + + # Strip off comments. content = re.compile(r'^#.*$', re.MULTILINE).sub('', content).strip() - if not content.strip(): + if not content: DieWithError('No CL description, aborting') - self.description = content + self._description = content - def ParseDescription(self): - """Updates the list of reviewers and subject from the description.""" - self.description = self.description.strip('\n') + '\n' - # Retrieves all reviewer lines - regexp = re.compile(r'^\s*(TBR|R)=(.+)$', re.MULTILINE) - reviewers = ', '.join( - i.group(2).strip() for i in regexp.finditer(self.description)) - if reviewers: - self.reviewers = reviewers + def append_footer(self, line): + # Adds a LF if the last line did not have 'FOO=BAR' or if the new one isn't. + if self._description: + if '\n' not in self._description: + self._description += '\n' + else: + last_line = self._description.rsplit('\n', 1)[1] + if (not presubmit_support.Change.TAG_LINE_RE.match(last_line) or + not presubmit_support.Change.TAG_LINE_RE.match(line)): + self._description += '\n' + self._description += '\n' + line - def IsEmpty(self): - return not self.description + def get_reviewers(self): + """Retrieves the list of reviewers.""" + regexp = re.compile(self.R_LINE, re.MULTILINE) + reviewers = [i.group(2) for i in regexp.finditer(self._description)] + return cleanup_list(reviewers) def FindCodereviewSettingsFile(filename='codereview.settings'): @@ -1102,16 +1138,15 @@ def GerritUpload(options, args, cl): if options.target_branch: branch = options.target_branch - log_desc = options.message or CreateDescriptionFromLog(args) - if CHANGE_ID not in log_desc: - AddChangeIdToCommitMessage(options, args) - if options.reviewers: - log_desc += '\nR=' + ', '.join(options.reviewers) - change_desc = ChangeDescription(log_desc, ', '.join(options.reviewers)) - change_desc.ParseDescription() - if change_desc.IsEmpty(): + change_desc = ChangeDescription( + options.message or CreateDescriptionFromLog(args)) + if not change_desc.description: print "Description is empty; aborting." return 1 + if CHANGE_ID not in change_desc.description: + AddChangeIdToCommitMessage(options, args) + if options.reviewers: + change_desc.update_reviewers(options.reviewers) receive_options = [] cc = cl.GetCCList().split(',') @@ -1120,11 +1155,9 @@ def GerritUpload(options, args, cl): cc = filter(None, cc) if cc: receive_options += ['--cc=' + email for email in cc] - if change_desc.reviewers: - reviewers = filter( - None, (r.strip() for r in change_desc.reviewers.split(','))) - if reviewers: - receive_options += ['--reviewer=' + email for email in reviewers] + if change_desc.get_reviewers(): + receive_options.extend( + '--reviewer=' + email for email in change_desc.get_reviewers()) git_command = ['push'] if receive_options: @@ -1160,24 +1193,21 @@ def RietveldUpload(options, args, cl): if options.title: upload_args.extend(['--title', options.title]) message = options.title or options.message or CreateDescriptionFromLog(args) - change_desc = ChangeDescription(message, ','.join(options.reviewers)) + change_desc = ChangeDescription(message) + if options.reviewers: + change_desc.update_reviewers(options.reviewers) if not options.force: - change_desc.Prompt() - change_desc.ParseDescription() + change_desc.prompt() - if change_desc.IsEmpty(): + if not change_desc.description: print "Description is empty; aborting." return 1 upload_args.extend(['--message', change_desc.description]) - if change_desc.reviewers: - upload_args.extend( - [ - '--reviewers', - ','.join(r.strip() for r in change_desc.reviewers.split(',')), - ]) + if change_desc.get_reviewers(): + upload_args.append('--reviewers=' + ','.join(change_desc.get_reviewers())) if options.send_mail: - if not change_desc.reviewers: + if not change_desc.get_reviewers(): DieWithError("Must specify reviewers to send email.") upload_args.append('--send_mail') cc = ','.join(filter(None, (cl.GetCCList(), ','.join(options.cc)))) @@ -1440,24 +1470,28 @@ def SendUpstream(parser, args, cmd): (cl.GetRietveldServer(), cl.GetIssue()), verbose=False) - description = options.message - if not description and cl.GetIssue(): - description = cl.GetDescription() + change_desc = ChangeDescription(options.message) + if not change_desc.description and cl.GetIssue(): + change_desc = ChangeDescription(cl.GetDescription()) - if not description: + if not change_desc.description: if not cl.GetIssue() and options.bypass_hooks: - description = CreateDescriptionFromLog([base_branch]) + change_desc = ChangeDescription(CreateDescriptionFromLog([base_branch])) else: print 'No description set.' print 'Visit %s/edit to set it.' % (cl.GetIssueURL()) return 1 + # Keep a separate copy for the commit message, because the commit message + # contains the link to the Rietveld issue, while the Rietveld message contains + # the commit viewvc url. + commit_desc = ChangeDescription(change_desc.description) if cl.GetIssue(): - description += "\n\nReview URL: %s" % cl.GetIssueURL() - + commit_desc.append_footer('Review URL: %s' % cl.GetIssueURL()) if options.contributor: - description += "\nPatch from %s." % options.contributor - print 'Description:', repr(description) + commit_desc.append_footer('Patch from %s.' % options.contributor) + + print 'Description:', repr(commit_desc.description) branches = [base_branch, cl.GetBranchRef()] if not options.force: @@ -1493,9 +1527,13 @@ def SendUpstream(parser, args, cmd): RunGit(['checkout', '-q', '-b', MERGE_BRANCH]) RunGit(['reset', '--soft', base_branch]) if options.contributor: - RunGit(['commit', '--author', options.contributor, '-m', description]) + RunGit( + [ + 'commit', '--author', options.contributor, + '-m', commit_desc.description, + ]) else: - RunGit(['commit', '-m', description]) + RunGit(['commit', '-m', commit_desc.description]) if base_has_submodules: cherry_pick_commit = RunGit(['rev-list', 'HEAD^!']).rstrip() RunGit(['branch', CHERRY_PICK_BRANCH, svn_head]) @@ -1534,12 +1572,12 @@ def SendUpstream(parser, args, cmd): return 1 viewvc_url = settings.GetViewVCUrl() if viewvc_url and revision: - cl.description += ('\n\nCommitted: ' + viewvc_url + revision) + change_desc.append_footer('Committed: ' + viewvc_url + revision) elif revision: - cl.description += ('\n\nCommitted: ' + revision) + change_desc.append_footer('Committed: ' + revision) print ('Closing issue ' '(you may be prompted for your codereview password)...') - cl.UpdateDescription(cl.description) + cl.UpdateDescription(change_desc.description) cl.CloseIssue() props = cl.RpcServer().get_issue_properties(cl.GetIssue(), False) patch_num = len(props['patchsets']) diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 23488ce516..58508bf452 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -30,10 +30,23 @@ class PresubmitMock(object): class RietveldMock(object): def __init__(self, *args, **kwargs): pass + @staticmethod def get_description(issue): return 'Issue: %d' % issue + @staticmethod + def get_issue_properties(_issue, _messages): + return { + 'reviewers': ['joe@chromium.org', 'john@chromium.org'], + 'messages': [ + { + 'approval': True, + 'sender': 'john@chromium.org', + }, + ], + } + class WatchlistsMock(object): def __init__(self, _): @@ -271,7 +284,8 @@ class TestGitCl(TestCase): ((['git', 'checkout', '-q', '-b', 'git-cl-commit'],), ''), ((['git', 'reset', '--soft', 'fake_ancestor_sha'],), ''), ((['git', 'commit', '-m', - 'Issue: 12345\n\nReview URL: https://codereview.example.com/12345'],), + 'Issue: 12345\n\n' + 'Review URL: https://codereview.example.com/12345'],), ''), ((['git', 'svn', 'dcommit', '-C50', '--no-rebase', '--rmdir'],), (('', None), 0)), @@ -334,68 +348,68 @@ class TestGitCl(TestCase): def test_no_reviewer(self): self._run_reviewer_test( [], - 'desc\n\nBUG=\n', - '# Blah blah comment.\ndesc\n\nBUG=\n', - 'desc\n\nBUG=\n', + 'desc\n\nBUG=', + '# Blah blah comment.\ndesc\n\nBUG=', + 'desc\n\nBUG=', []) def test_keep_similarity(self): self._run_reviewer_test( ['--similarity', '70'], - 'desc\n\nBUG=\n', - '# Blah blah comment.\ndesc\n\nBUG=\n', - 'desc\n\nBUG=\n', + 'desc\n\nBUG=', + '# Blah blah comment.\ndesc\n\nBUG=', + 'desc\n\nBUG=', []) def test_keep_find_copies(self): self._run_reviewer_test( ['--no-find-copies'], - 'desc\n\nBUG=\n', + 'desc\n\nBUG=', '# Blah blah comment.\ndesc\n\nBUG=\n', - 'desc\n\nBUG=\n', + 'desc\n\nBUG=', []) def test_reviewers_cmd_line(self): # Reviewer is passed as-is - description = 'desc\n\nR=foo@example.com\nBUG=\n' + description = 'desc\n\nR=foo@example.com\nBUG=' self._run_reviewer_test( ['-r' 'foo@example.com'], description, '\n%s\n' % description, description, - ['--reviewers', 'foo@example.com']) + ['--reviewers=foo@example.com']) def test_reviewer_tbr_overriden(self): # Reviewer is overriden with TBR # Also verifies the regexp work without a trailing LF - description = 'Foo Bar\nTBR=reviewer@example.com\n' + description = 'Foo Bar\n\nTBR=reviewer@example.com' self._run_reviewer_test( ['-r' 'foo@example.com'], - 'desc\n\nR=foo@example.com\nBUG=\n', + 'desc\n\nR=foo@example.com\nBUG=', description.strip('\n'), description, - ['--reviewers', 'reviewer@example.com']) + ['--reviewers=reviewer@example.com']) def test_reviewer_multiple(self): # Handles multiple R= or TBR= lines. description = ( - 'Foo Bar\nTBR=reviewer@example.com\nBUG=\nR=another@example.com\n') + 'Foo Bar\nTBR=reviewer@example.com\nBUG=\nR=another@example.com') self._run_reviewer_test( [], - 'desc\n\nBUG=\n', + 'desc\n\nBUG=', description, description, - ['--reviewers', 'reviewer@example.com,another@example.com']) + ['--reviewers=another@example.com,reviewer@example.com']) def test_reviewer_send_mail(self): # --send-mail can be used without -r if R= is used - description = 'Foo Bar\nR=reviewer@example.com\n' + description = 'Foo Bar\nR=reviewer@example.com' self._run_reviewer_test( ['--send-mail'], - 'desc\n\nBUG=\n', + 'desc\n\nBUG=', description.strip('\n'), description, - ['--reviewers', 'reviewer@example.com', '--send_mail']) + ['--reviewers=reviewer@example.com', '--send_mail']) def test_reviewer_send_mail_no_rev(self): # Fails without a reviewer. @@ -490,7 +504,8 @@ class TestGitCl(TestCase): receive_pack += '--cc=joe@example.com' # from watch list if reviewers: receive_pack += ' ' - receive_pack += ' '.join(['--reviewer=' + email for email in reviewers]) + receive_pack += ' '.join( + '--reviewer=' + email for email in sorted(reviewers)) receive_pack += '' calls += [ ((['git', 'push', receive_pack, 'origin', 'HEAD:refs/for/master'],), @@ -590,6 +605,21 @@ class TestGitCl(TestCase): ] git_cl.main(['config']) + def test_update_reviewers(self): + data = [ + ('foo', [], 'foo'), + ('foo', ['a@c'], 'foo\n\nR=a@c'), + ('foo\nBUG=', ['a@c'], 'foo\nBUG=\nR=a@c'), + ('foo\nR=xx\nTBR=yy\nR=bar', ['a@c'], 'foo\nTBR=a@c'), + ('foo', ['a@c', 'b@c'], 'foo\n\nR=a@c, b@c'), + ] + for orig, reviewers, expected in data: + obj = git_cl.ChangeDescription(orig) + obj.update_reviewers(reviewers) + self.assertEqual(expected, obj.description) + if __name__ == '__main__': + git_cl.logging.basicConfig( + level=git_cl.logging.DEBUG if '-v' in sys.argv else git_cl.logging.ERROR) unittest.main()