diff --git a/split_cl.py b/split_cl.py index d0d9b1917..ed34a14d0 100644 --- a/split_cl.py +++ b/split_cl.py @@ -32,29 +32,45 @@ CL_SPLIT_FORCE_LIMIT = 10 # will be listed. CL_SPLIT_TOP_REVIEWERS = 5 +FilesAndOwnersDirectory = collections.namedtuple("FilesAndOwnersDirectory", + "files owners_directories") + def EnsureInGitRepository(): """Throws an exception if the current directory is not a git repository.""" git.run('rev-parse') -def CreateBranchForDirectory(prefix, directory, upstream): - """Creates a branch named |prefix| + "_" + |directory| + "_split". +def CreateBranchForDirectories(prefix, directories, upstream): + """Creates a branch named |prefix| + "_" + |directories[0]| + "_split". Return false if the branch already exists. |upstream| is used as upstream for the created branch. """ existing_branches = set(git.branches(use_limit = False)) - branch_name = prefix + '_' + directory + '_split' + branch_name = prefix + '_' + directories[0] + '_split' if branch_name in existing_branches: return False git.run('checkout', '-t', upstream, '-b', branch_name) return True -def FormatDescriptionOrComment(txt, directory): - """Replaces $directory with |directory| in |txt|.""" - return txt.replace('$directory', '/' + directory) +def FormatDirectoriesForPrinting(directories, prefix=None): + """Formats directory list for printing + + Uses dedicated format for single-item list.""" + + prefixed = directories + if prefix: + prefixed = [(prefix + d) for d in directories] + + return str(prefixed) if len(prefixed) > 1 else str(prefixed[0]) + + +def FormatDescriptionOrComment(txt, directories): + """Replaces $directory with |directories| in |txt|.""" + to_insert = FormatDirectoriesForPrinting(directories, prefix='/') + return txt.replace('$directory', to_insert) def AddUploadedByGitClSplitToDescription(description): @@ -73,7 +89,7 @@ def AddUploadedByGitClSplitToDescription(description): return '\n'.join(lines) -def UploadCl(refactor_branch, refactor_branch_upstream, directory, files, +def UploadCl(refactor_branch, refactor_branch_upstream, directories, files, description, comment, reviewers, changelist, cmd_upload, cq_dry_run, enable_auto_submit, topic, repository_root): """Uploads a CL with all changes to |files| in |refactor_branch|. @@ -81,8 +97,8 @@ def UploadCl(refactor_branch, refactor_branch_upstream, directory, files, Args: refactor_branch: Name of the branch that contains the changes to upload. refactor_branch_upstream: Name of the upstream of |refactor_branch|. - directory: Path to the directory that contains the OWNERS file for which - to upload a CL. + directories: Paths to the directories that contain the OWNERS files for + which to upload a CL. files: List of AffectedFile instances to include in the uploaded CL. description: Description of the uploaded CL. comment: Comment to post on the uploaded CL. @@ -94,9 +110,10 @@ def UploadCl(refactor_branch, refactor_branch_upstream, directory, files, topic: Topic to associate with uploaded CLs. """ # Create a branch. - if not CreateBranchForDirectory( - refactor_branch, directory, refactor_branch_upstream): - print('Skipping ' + directory + ' for which a branch already exists.') + if not CreateBranchForDirectories(refactor_branch, directories, + refactor_branch_upstream): + print('Skipping ' + FormatDirectoriesForPrinting(directories) + + ' for which a branch already exists.') return # Checkout all changes to files in |files|. @@ -119,7 +136,7 @@ def UploadCl(refactor_branch, refactor_branch_upstream, directory, files, # when it is closed. with gclient_utils.temporary_file() as tmp_file: gclient_utils.FileWrite( - tmp_file, FormatDescriptionOrComment(description, directory)) + tmp_file, FormatDescriptionOrComment(description, directories)) git.run('commit', '-F', tmp_file) # Upload a CL. @@ -134,17 +151,18 @@ def UploadCl(refactor_branch, refactor_branch_upstream, directory, files, upload_args.append('--enable-auto-submit') if topic: upload_args.append('--topic={}'.format(topic)) - print('Uploading CL for ' + directory + '...') + print('Uploading CL for ' + FormatDirectoriesForPrinting(directories) + '...') ret = cmd_upload(upload_args) if ret != 0: - print('Uploading failed for ' + directory + '.') + print('Uploading failed for ' + FormatDirectoriesForPrinting(directories) + + '.') print('Note: git cl split has built-in resume capabilities.') print('Delete ' + git.current_branch() + ' then run git cl split again to resume uploading.') if comment: - changelist().AddComment(FormatDescriptionOrComment(comment, directory), + changelist().AddComment(FormatDescriptionOrComment(comment, directories), publish=True) @@ -172,14 +190,14 @@ def GetFilesSplitByOwners(files, max_depth): return files_split_by_owners -def PrintClInfo(cl_index, num_cls, directory, file_paths, description, +def PrintClInfo(cl_index, num_cls, directories, file_paths, description, reviewers, enable_auto_submit, topic): """Prints info about a CL. Args: cl_index: The index of this CL in the list of CLs to upload. num_cls: The total number of CLs that will be uploaded. - directory: Path to the directory that contains the OWNERS file for which + directories: Paths to directories that contains the OWNERS files for which to upload a CL. file_paths: A list of files in this CL. description: The CL description. @@ -188,11 +206,11 @@ def PrintClInfo(cl_index, num_cls, directory, file_paths, description, topic: Topic to set for this CL. """ description_lines = FormatDescriptionOrComment(description, - directory).splitlines() + directories).splitlines() indented_description = '\n'.join([' ' + l for l in description_lines]) print('CL {}/{}'.format(cl_index, num_cls)) - print('Path: {}'.format(directory)) + print('Paths: {}'.format(FormatDirectoriesForPrinting(directories))) print('Reviewers: {}'.format(', '.join(reviewers))) print('Auto-Submit: {}'.format(enable_auto_submit)) print('Topic: {}'.format(topic)) @@ -245,9 +263,22 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, assert refactor_branch_upstream, \ "Branch %s must have an upstream." % refactor_branch - files_split_by_owners = GetFilesSplitByOwners(files, max_depth) + # Verify that the description contains a bug link. Examples: + # Bug: 123 + # Bug: chromium:456 + bug_pattern = re.compile(r"^Bug:\s*(?:[a-zA-Z]+:)?[0-9]+", re.MULTILINE) + matches = re.findall(bug_pattern, description) + answer = 'y' + if not matches: + answer = gclient_utils.AskForData( + 'Description does not include a bug link. Proceed? (y/n):') + if answer.lower() != 'y': + return 0 + + files_split_by_reviewers = SelectReviewersForFiles(cl, author, files, + max_depth) - num_cls = len(files_split_by_owners) + num_cls = len(files_split_by_reviewers) print('Will split current branch (' + refactor_branch + ') into ' + str(num_cls) + ' CLs.\n') if cq_dry_run and num_cls > CL_SPLIT_FORCE_LIMIT: @@ -261,34 +292,20 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, if answer.lower() != 'y': return 0 - # Verify that the description contains a bug link. Examples: - # Bug: 123 - # Bug: chromium:456 - bug_pattern = re.compile(r"^Bug:\s*(?:[a-zA-Z]+:)?[0-9]+", re.MULTILINE) - matches = re.findall(bug_pattern, description) - answer = 'y' - if not matches: - answer = gclient_utils.AskForData( - 'Description does not include a bug link. Proceed? (y/n):') - if answer.lower() != 'y': - return 0 - cls_per_reviewer = collections.defaultdict(int) - for cl_index, (directory, files) in \ - enumerate(files_split_by_owners.items(), 1): - # Use '/' as a path separator in the branch name and the CL description - # and comment. - directory = directory.replace(os.path.sep, '/') - file_paths = [f for _, f in files] - reviewers = cl.owners_client.SuggestOwners( - file_paths, exclude=[author, cl.owners_client.EVERYONE]) + for cl_index, (reviewers, cl_info) in \ + enumerate(files_split_by_reviewers.items(), 1): + # Convert reviewers from tuple to set. + reviewer_set = set(reviewers) if dry_run: - PrintClInfo(cl_index, num_cls, directory, file_paths, description, - reviewers, enable_auto_submit, topic) + file_paths = [f for _, f in cl_info.files] + PrintClInfo(cl_index, num_cls, cl_info.owners_directories, file_paths, + description, reviewer_set, enable_auto_submit, topic) else: - UploadCl(refactor_branch, refactor_branch_upstream, directory, files, - description, comment, reviewers, changelist, cmd_upload, - cq_dry_run, enable_auto_submit, topic, repository_root) + UploadCl(refactor_branch, refactor_branch_upstream, + cl_info.owners_directories, cl_info.files, description, + comment, reviewer_set, changelist, cmd_upload, cq_dry_run, + enable_auto_submit, topic, repository_root) for reviewer in reviewers: cls_per_reviewer[reviewer] += 1 @@ -309,3 +326,36 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, sys.stderr.write(cpe.stderr) return 1 return 0 + + +def SelectReviewersForFiles(cl, author, files, max_depth): + """Selects reviewers for passed-in files + + Args: + cl: Changelist class instance + author: Email of person running 'git cl split' + files: List of files + max_depth: The maximum directory depth to search for OWNERS files. A value + less than 1 means no limit. + """ + info_split_by_owners = GetFilesSplitByOwners(files, max_depth) + + info_split_by_reviewers = {} + + for (directory, split_files) in info_split_by_owners.items(): + # Use '/' as a path separator in the branch name and the CL description + # and comment. + directory = directory.replace(os.path.sep, '/') + file_paths = [f for _, f in split_files] + # Convert reviewers list to tuple in order to use reviewers as key to + # dictionary. + reviewers = tuple( + cl.owners_client.SuggestOwners( + file_paths, exclude=[author, cl.owners_client.EVERYONE])) + + if not reviewers in info_split_by_reviewers: + info_split_by_reviewers[reviewers] = FilesAndOwnersDirectory([], []) + info_split_by_reviewers[reviewers].files.extend(split_files) + info_split_by_reviewers[reviewers].owners_directories.append(directory) + + return info_split_by_reviewers diff --git a/tests/split_cl_test.py b/tests/split_cl_test.py index 083ba6083..6e78bf1be 100755 --- a/tests/split_cl_test.py +++ b/tests/split_cl_test.py @@ -4,6 +4,7 @@ import os import sys import unittest +from unittest import mock sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) @@ -11,26 +12,78 @@ import split_cl class SplitClTest(unittest.TestCase): - _description = """Convert use of X to Y in $directory + def testAddUploadedByGitClSplitToDescription(self): + description = """Convert use of X to Y in $directory """ + footers = 'Bug: 12345' - _footers = 'Bug: 12345' - - def testAddUploadedByGitClSplitToDescription(self): added_line = 'This CL was uploaded by git cl split.' # Description without footers - self.assertEqual( - split_cl.AddUploadedByGitClSplitToDescription(self._description), - self._description + added_line) + self.assertEqual(split_cl.AddUploadedByGitClSplitToDescription(description), + description + added_line) # Description with footers self.assertEqual( - split_cl.AddUploadedByGitClSplitToDescription(self._description + - self._footers), - self._description + added_line + '\n\n' + self._footers) + split_cl.AddUploadedByGitClSplitToDescription(description + footers), + description + added_line + '\n\n' + footers) + + def testFormatDescriptionOrComment(self): + description = "Converted use of X to Y in $directory." + + # One directory + self.assertEqual(split_cl.FormatDescriptionOrComment(description, ["foo"]), + "Converted use of X to Y in /foo.") + + # Many directories + self.assertEqual( + split_cl.FormatDescriptionOrComment(description, ["foo", "bar"]), + "Converted use of X to Y in ['/foo', '/bar'].") + + def GetDirectoryBaseName(self, file_path): + return os.path.basename(os.path.dirname(file_path)) + + def MockSuggestOwners(self, paths, exclude=None): + if not paths: + return ["superowner"] + return self.GetDirectoryBaseName(paths[0]).split(",") + + def MockIsFile(self, file_path): + if os.path.basename(file_path) == "OWNERS": + return "owner" in self.GetDirectoryBaseName(file_path) + + return True + + @mock.patch("os.path.isfile") + def testSelectReviewersForFiles(self, mock_is_file): + mock_is_file.side_effect = self.MockIsFile + + owners_client = mock.Mock(SuggestOwners=self.MockSuggestOwners, + EVERYONE="*") + cl = mock.Mock(owners_client=owners_client) + + files = [("M", "foo/owner1,owner2/a.txt"), ("M", "foo/owner1,owner2/b.txt"), + ("M", "bar/owner1,owner2/c.txt"), ("M", "bax/owner2/d.txt"), + ("M", "baz/owner3/e.txt")] + + files_split_by_reviewers = split_cl.SelectReviewersForFiles( + cl, "author", files, 0) + + self.assertEqual(3, len(files_split_by_reviewers.keys())) + info1 = files_split_by_reviewers[tuple(["owner1", "owner2"])] + self.assertEqual(info1.files, [("M", "foo/owner1,owner2/a.txt"), + ("M", "foo/owner1,owner2/b.txt"), + ("M", "bar/owner1,owner2/c.txt")]) + self.assertEqual(info1.owners_directories, + ["foo/owner1,owner2", "bar/owner1,owner2"]) + info2 = files_split_by_reviewers[tuple(["owner2"])] + self.assertEqual(info2.files, [("M", "bax/owner2/d.txt")]) + self.assertEqual(info2.owners_directories, ["bax/owner2"]) + info3 = files_split_by_reviewers[tuple(["owner3"])] + self.assertEqual(info3.files, [("M", "baz/owner3/e.txt")]) + self.assertEqual(info3.owners_directories, ["baz/owner3"]) if __name__ == '__main__':