From 4629c3474e0fe71ecf2861b9051c9db1408e72ca Mon Sep 17 00:00:00 2001 From: Devon Loehr Date: Mon, 24 Feb 2025 11:08:30 -0800 Subject: [PATCH] [git cl split]: Deprecate $directory in favor of $description `git cl split` allows users to parameterize their CL descriptions by including the string $directory, which is replaced by the list of directories covered by each generated CL. However, as we add more ways of generating splittings, a list of directories may not always be the most appropriate description. For example, the --from-file option lets users write arbitrary description strings. This CL renames the $directory string to $description, to reflect this new generality, and refactors all places in the code that expected a directory list instead of a string. Since users may beused to the current version, it does not remove $directory, but instead notes that it is deprecated and emits a warning when it is used. After some time, we can remove it entirely. Bug: 389069356 Change-Id: If8c947fdcbbb4897675b015a377cf21123e51467 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6299688 Reviewed-by: Josip Sokcevic Commit-Queue: Devon Loehr --- git_cl.py | 14 ++++--- split_cl.py | 90 ++++++++++++++++++++++-------------------- tests/split_cl_test.py | 44 +++++++++++++++------ 3 files changed, 88 insertions(+), 60 deletions(-) diff --git a/git_cl.py b/git_cl.py index bbdcb6393..28fcb5f16 100755 --- a/git_cl.py +++ b/git_cl.py @@ -5785,8 +5785,8 @@ def CMDsplit(parser, args): Creates a branch and uploads a CL for each group of files modified in the current branch that share a common OWNERS file. In the CL description and - comment, the string '$directory', is replaced with the directory containing - the shared OWNERS file. + comment, the string '$directory' or '$description', is replaced with the + directory containing the shared OWNERS file. """ if gclient_utils.IsEnvCog(): print('split command is not supported in non-git environment.', @@ -5796,9 +5796,13 @@ def CMDsplit(parser, args): parser.add_option('-d', '--description', dest='description_file', - help='A text file containing a CL description in which ' - '$directory will be replaced by each CL\'s directory. ' - 'Mandatory except in dry runs.') + help='A text file containing a CL description, in which ' + 'the string $description will be replaced by a ' + 'description of each generated CL (by default, the list ' + 'of directories that CL covers). ' + 'Mandatory except in dry runs.\n' + 'Usage of the string $directory instead of $description ' + 'is deprecated, and will be removed in the future.') parser.add_option('-c', '--comment', dest='comment_file', diff --git a/split_cl.py b/split_cl.py index 8e0023f0c..5dbc6378d 100644 --- a/split_cl.py +++ b/split_cl.py @@ -64,8 +64,8 @@ class CLInfo: - reviewers: the reviewers the CL will be sent to. - files: a list of , pairs in the CL. Has the same format as `git status`. - - directories: a string representing the directories containing the files - in this CL. This is only used for replacing $directory in + - description: a string describing the CL. Typically the list of affected + directories. Only used for replacing $description in the user-provided CL description. """ # Have to use default_factory because lists are mutable @@ -74,7 +74,7 @@ class CLInfo: # This is only used for formatting in the CL description, so it just # has to be convertible to string. - directories: Any = "" + description: Any = "" def FormatForPrinting(self) -> str: """ @@ -83,7 +83,7 @@ class CLInfo: # Don't quote the reviewer emails in the output reviewers_str = ", ".join(self.reviewers) lines = [ - f"Reviewers: [{reviewers_str}]", f"Description: {self.directories}" + f"Reviewers: [{reviewers_str}]", f"Description: {self.description}" ] + [f"{action}, {file}" for (action, file) in self.files] return "\n".join(lines) @@ -97,7 +97,8 @@ def CLInfoFromFilesAndOwnersDirectoriesDict( cl_infos = [] for (reviewers, fod) in d.items(): cl_infos.append( - CLInfo(set(reviewers), fod.files, fod.owners_directories)) + CLInfo(set(reviewers), fod.files, + FormatDirectoriesForPrinting(fod.owners_directories))) return cl_infos @@ -168,7 +169,9 @@ def ValidateExistingBranches(prefix: str, cl_infos: List[CLInfo]) -> bool: return False return True -def FormatDirectoriesForPrinting(directories, prefix=None): + +def FormatDirectoriesForPrinting(directories: List[str], + prefix: str = None) -> str: """Formats directory list for printing Uses dedicated format for single-item list.""" @@ -177,13 +180,20 @@ def FormatDirectoriesForPrinting(directories, prefix=None): if prefix: prefixed = [(prefix + d) for d in directories] - return str(prefixed) if len(prefixed) > 1 else str(prefixed[0]) + return str(prefixed[0]) if len(prefixed) == 1 else str(prefixed) -def FormatDescriptionOrComment(txt, directories): - """Replaces $directory with |directories| in |txt|.""" - to_insert = FormatDirectoriesForPrinting(directories) - return txt.replace('$directory', to_insert) +def FormatDescriptionOrComment(txt, desc): + """Replaces $description with |desc| in |txt|.""" + # TODO(389069356): Remove support for $directory entirely once it's been + # deprecated for a while. + replaced_txt = txt.replace('$directory', desc) + if txt != replaced_txt: + EmitWarning('Usage of $directory is deprecated and will be removed ' + 'in a future update. Please use $description instead, ' + 'which has the same behavior by default.\n\n') + replaced_txt = replaced_txt.replace('$description', desc) + return replaced_txt def AddUploadedByGitClSplitToDescription(description): @@ -202,19 +212,19 @@ def AddUploadedByGitClSplitToDescription(description): return '\n'.join(lines) -def UploadCl(refactor_branch, refactor_branch_upstream, directories, files, - description, saved_splitting_file, comment, reviewers, changelist, - cmd_upload, cq_dry_run, enable_auto_submit, topic, +def UploadCl(refactor_branch, refactor_branch_upstream, cl_description, files, + user_description, saved_splitting_file, 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|. Args: refactor_branch: Name of the branch that contains the changes to upload. refactor_branch_upstream: Name of the upstream of |refactor_branch|. - directories: Paths to the directories that contain the OWNERS files for - which to upload a CL. + cl_description: Description of this specific CL, e.g. the list of + affected directories. files: List of AffectedFile instances to include in the uploaded CL. - description: Description of the uploaded CL. + user_description: Description provided by user. comment: Comment to post on the uploaded CL. reviewers: A set of reviewers for the CL. changelist: The Changelist class. @@ -226,8 +236,9 @@ def UploadCl(refactor_branch, refactor_branch_upstream, directories, files, # Create a branch. if not CreateBranchForOneCL(refactor_branch, files, refactor_branch_upstream): - Emit('Skipping ' + FormatDirectoriesForPrinting(directories) + - ' for which a branch already exists.') + Emit( + f'Skipping existing branch for CL with description: {cl_description}' + ) return # Checkout all changes to files in |files|. @@ -250,7 +261,8 @@ def UploadCl(refactor_branch, refactor_branch_upstream, directories, files, # when it is closed. with gclient_utils.temporary_file() as tmp_file: gclient_utils.FileWrite( - tmp_file, FormatDescriptionOrComment(description, directories)) + tmp_file, + FormatDescriptionOrComment(user_description, cl_description)) git.run('commit', '-F', tmp_file) # Upload a CL. @@ -265,8 +277,7 @@ def UploadCl(refactor_branch, refactor_branch_upstream, directories, files, upload_args.append('--enable-auto-submit') if topic: upload_args.append('--topic={}'.format(topic)) - Emit('Uploading CL for ' + FormatDirectoriesForPrinting(directories) + - '...') + Emit(f'Uploading CL with description: {cl_description} ...') ret = cmd_upload(upload_args) if ret != 0: @@ -278,7 +289,7 @@ def UploadCl(refactor_branch, refactor_branch_upstream, directories, files, if comment: changelist().AddComment(FormatDescriptionOrComment( - comment, directories), + comment, cl_description), publish=True) @@ -307,28 +318,28 @@ def GetFilesSplitByOwners(files, max_depth): return files_split_by_owners -def PrintClInfo(cl_index, num_cls, directories, file_paths, description, +def PrintClInfo(cl_index, num_cls, cl_description, file_paths, user_description, reviewers, cq_dry_run, 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. - directories: Paths to directories that contains the OWNERS files for - which to upload a CL. + cl_description: Description of this specific CL, e.g. the list of + affected directories. file_paths: A list of files in this CL. - description: The CL description. + user_description: Description provided by user. reviewers: A set of reviewers for this CL. cq_dry_run: If the CL should also be sent to CQ dry run. enable_auto_submit: If the CL should also have auto submit enabled. topic: Topic to set for this CL. """ - description_lines = FormatDescriptionOrComment(description, - directories).splitlines() + description_lines = FormatDescriptionOrComment(user_description, + cl_description).splitlines() indented_description = '\n'.join([' ' + l for l in description_lines]) Emit('CL {}/{}'.format(cl_index, num_cls)) - Emit('Paths: {}'.format(FormatDirectoriesForPrinting(directories))) + Emit('Paths: {}'.format(cl_description)) Emit('Reviewers: {}'.format(', '.join(reviewers))) Emit('Auto-Submit: {}'.format(enable_auto_submit)) Emit('CQ Dry Run: {}'.format(cq_dry_run)) @@ -345,7 +356,7 @@ def LoadDescription(description_file, dry_run): raise ValueError( "Must provide a description file except during dry runs") return ('Dummy description for dry run.\n' - 'directory = $directory') + 'description = $description') return gclient_utils.FileRead(description_file) @@ -360,7 +371,7 @@ def PrintSummary(cl_infos, refactor_branch): """ for info in cl_infos: Emit(f'Reviewers: {info.reviewers}, files: {len(info.files)}, ', - f'directories: {info.directories}') + f'description: {info.description}') num_cls = len(cl_infos) Emit(f'\nWill split branch {refactor_branch} into {num_cls} CLs. ' @@ -462,12 +473,12 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, pass elif dry_run: file_paths = [f for _, f in cl_info.files] - PrintClInfo(cl_index, len(cl_infos), cl_info.directories, + PrintClInfo(cl_index, len(cl_infos), cl_info.description, file_paths, description, cl_info.reviewers, cq_dry_run, enable_auto_submit, topic) else: UploadCl(refactor_branch, refactor_branch_upstream, - cl_info.directories, cl_info.files, description, + cl_info.description, cl_info.files, description, saved_splitting_file, comment, cl_info.reviewers, changelist, cmd_upload, cq_dry_run, enable_auto_submit, topic, repository_root) @@ -591,11 +602,6 @@ description_re = re.compile(r'Description:\s*(.+)') # must be a valid code (either 1 or 2 letters) file_re = re.compile(r'([MTADRC]{1,2}),\s*(.+)') -# TODO(crbug.com/389069356): Replace the "Description" line with an optional -# "Description" line, and adjust the description variables accordingly, as well -# as all the places in the code that expect to get a directory list. - - # We use regex parsing instead of e.g. json because it lets us use a much more # human-readable format, similar to the summary printed in dry runs def ParseSplittings(lines: List[str]) -> List[CLInfo]: @@ -644,11 +650,11 @@ def ParseSplittings(lines: List[str]) -> List[CLInfo]: # Description is just used as a description, so any string is fine m = re.fullmatch(description_re, line) if m: - if current_cl_info.directories: + if current_cl_info.description: raise ClSplitParseError( - f"Error parsing line: CL already has a directories entry\n{line}" + f"Error parsing line: CL already has a description entry\n{line}" ) - current_cl_info.directories = m.group(1).strip() + current_cl_info.description = m.group(1).strip() continue # Any other line is presumed to be an ', ' pair diff --git a/tests/split_cl_test.py b/tests/split_cl_test.py index 6133f2c5c..001d44b7e 100755 --- a/tests/split_cl_test.py +++ b/tests/split_cl_test.py @@ -25,7 +25,7 @@ class SplitClTest(unittest.TestCase): return path def testAddUploadedByGitClSplitToDescription(self): - description = """Convert use of X to Y in $directory + description = """Convert use of X to Y in $description @@ -44,21 +44,39 @@ class SplitClTest(unittest.TestCase): footers), description + added_line + '\n\n' + footers) - def testFormatDescriptionOrComment(self): - description = "Converted use of X to Y in $directory." + @mock.patch("split_cl.EmitWarning") + def testFormatDescriptionOrComment(self, mock_emit_warning): + description = "Converted use of X to Y in $description." # One directory self.assertEqual( - split_cl.FormatDescriptionOrComment(description, ["foo"]), + split_cl.FormatDescriptionOrComment( + description, split_cl.FormatDirectoriesForPrinting(["foo"])), "Converted use of X to Y in foo.", ) # Many directories self.assertEqual( - split_cl.FormatDescriptionOrComment(description, ["foo", "bar"]), + split_cl.FormatDescriptionOrComment( + description, + split_cl.FormatDirectoriesForPrinting(["foo", "bar"])), "Converted use of X to Y in ['foo', 'bar'].", ) + mock_emit_warning.assert_not_called() + + description_deprecated = "Converted use of X to Y in $directory." + + # Make sure we emit a deprecation warning if the old format is used + self.assertEqual( + split_cl.FormatDescriptionOrComment( + description_deprecated, + split_cl.FormatDirectoriesForPrinting([])), + "Converted use of X to Y in [].", + ) + + mock_emit_warning.assert_called_once() + def GetDirectoryBaseName(self, file_path): return os.path.basename(os.path.dirname(file_path)) @@ -130,9 +148,9 @@ class SplitClTest(unittest.TestCase): test.addCleanup(patcher.stop) return patcher.start() - def DoUploadCl(self, directories, files, reviewers, cmd_upload): + def DoUploadCl(self, description, files, reviewers, cmd_upload): split_cl.UploadCl("branch_to_upload", "upstream_branch", - directories, files, "description", + description, files, "description", "splitting_file.txt", None, reviewers, mock.Mock(), cmd_upload, True, True, "topic", os.path.sep) @@ -142,12 +160,12 @@ class SplitClTest(unittest.TestCase): upload_cl_tester = self.UploadClTester(self) - directories = ["dir0"] + description = split_cl.FormatDirectoriesForPrinting(["dir0"]) files = [("M", os.path.join("bar", "a.cc")), ("D", os.path.join("foo", "b.cc"))] reviewers = {"reviewer1@gmail.com", "reviewer2@gmail.com"} mock_cmd_upload = mock.Mock() - upload_cl_tester.DoUploadCl(directories, files, reviewers, + upload_cl_tester.DoUploadCl(description, files, reviewers, mock_cmd_upload) abs_repository_path = os.path.abspath(os.path.sep) @@ -174,7 +192,7 @@ class SplitClTest(unittest.TestCase): upload_cl_tester = self.UploadClTester(self) - directories = ["dir0"] + description = split_cl.FormatDirectoriesForPrinting(["dir0"]) files = [("M", os.path.join("bar", "a.cc")), ("D", os.path.join("foo", "b.cc"))] reviewers = {"reviewer1@gmail.com"} @@ -183,7 +201,7 @@ class SplitClTest(unittest.TestCase): "branch0", split_cl.CreateBranchName("branch_to_upload", files) ] - upload_cl_tester.DoUploadCl(directories, files, reviewers, + upload_cl_tester.DoUploadCl(description, files, reviewers, mock_cmd_upload) upload_cl_tester.mock_git_run.assert_not_called() @@ -384,14 +402,14 @@ class SplitClTest(unittest.TestCase): # Tests related to saving to and loading from files # Sample CLInfos for testing CLInfo_1 = split_cl.CLInfo(reviewers=["a@example.com"], - directories="['chrome/browser']", + description="['chrome/browser']", files=[ ("M", "chrome/browser/a.cc"), ("M", "chrome/browser/b.cc"), ]) CLInfo_2 = split_cl.CLInfo(reviewers=["a@example.com", "b@example.com"], - directories="['foo', 'bar/baz']", + description="['foo', 'bar/baz']", files=[("M", "foo/browser/a.cc"), ("M", "bar/baz/b.cc"), ("D", "foo/bar/c.h")])