aiden
/
depot_tools
mirror of https://chromium.googlesource.com/chromium/tools/depot_tools
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

[git cl split]: Deprecate $directory in favor of $description

Browse Source 
`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 <sokcevic@chromium.org>
Commit-Queue: Devon Loehr <dloehr@google.com>
changes/88/6299688/2
Devon Loehr 4 days ago committed by LUCI CQ
parent 158b6837dc
commit 4629c3474e
3 changed files with 88 additions and 60 deletions
Show Stats Download Patch File Download Diff File

14
git_cl.py
Unescape Escape View File

@ -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',

90
split_cl.py
Unescape Escape View File

@ -64,8 +64,8 @@ class CLInfo:
- reviewers: the reviewers the CL will be sent to.
- files: a list of <action>, <file> 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*(.+)')
# <action> 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 '<action>, <file>' pair

44
tests/split_cl_test.py
Unescape Escape View File

@ -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
<add some background about this conversion for the reviewers>
@ -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")])

Write Preview
Loading…
Cancel
Save