From 6d571c0070161b1b96049410f8584c0955d73536 Mon Sep 17 00:00:00 2001 From: Michael D Kinney Date: Sun, 18 Feb 2024 12:59:51 -0800 Subject: BaseTools/Scripts/PatchCheck: Error if commit modifies multiple packages REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4679 Update PatchCheck.py to evaluate all the files modified in each commit and generate an error if: * A commit adds/modifies files in multiple package directories * A commit adds/modifies files in multiple non-package directories * A commit adds/modifies files in both a package and a non-package directory * A commit deletes files from multiple package directories * A commit deletes files from multiple non-package directories * A commit deletes files from both a package and a non-package directory Modifications to files in the root of the repository are not evaluated. This check is skipped if PatchCheck.py is run on a patch file or input from stdin because this multiple package commit check depends on information from a git repository. If --ignore-multi-package option is set, then reduce the multiple package commit check from an error to a warning for all commits in the commit range provided to PatchCheck.py. Add check for a 'Continuous-integration-options:' commit message tag that allows one or more options to be specified at the individual commit scope to enable/disable continuous integration checks. This tag must start at the beginning of a commit message line and may appear more than once in a commit message. Add support for a Continuous-integration-options tag value of 'PatchCheck.ignore-multi-package' that reduces the multiple package commit check from an error to a warning for the specific commits that specify this option. Example: Continuous-integration-options: PatchCheck.ignore-multi-package The set of packages are found by searching for DEC files in a git repository. The list of DEC files in a git repository is collected with the following git command: git ls-files *.dec The set of files added/modified by each commit is found using the following git command: git diff-tree --no-commit-id --name-only --diff-filter=AM -r The set of files deleted by each commit is found using the following git command: git diff-tree --no-commit-id --name-only --diff-filter=D -r Cc: Rebecca Cran Cc: Liming Gao Cc: Bob Feng Cc: Yuwei Chen Cc: Michael Kubacki Cc: Ard Biesheuvel Cc: Leif Lindholm Signed-off-by: Michael D Kinney Reviewed-by: Michael Kubacki Reviewed-by: Rebecca Cran --- BaseTools/Scripts/PatchCheck.py | 77 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 76 insertions(+), 1 deletion(-) diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py index 415198e382..a3b812fb73 100755 --- a/BaseTools/Scripts/PatchCheck.py +++ b/BaseTools/Scripts/PatchCheck.py @@ -28,6 +28,7 @@ class Verbose: class PatchCheckConf: ignore_change_id = False + ignore_multi_package = False class EmailAddressCheck: """Checks an email address.""" @@ -98,6 +99,7 @@ class CommitMessageCheck: def __init__(self, subject, message, author_email): self.ok = True + self.ignore_multi_package = False if subject is None and message is None: self.error('Commit message is missing!') @@ -120,6 +122,7 @@ class CommitMessageCheck: self.check_overall_format() if not PatchCheckConf.ignore_change_id: self.check_change_id_format() + self.check_ci_options_format() self.report_message_result() url = 'https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format' @@ -324,6 +327,15 @@ class CommitMessageCheck: self.error('\"%s\" found in commit message:' % cid) return + def check_ci_options_format(self): + cio='Continuous-integration-options:' + for line in self.msg.splitlines(): + if not line.startswith(cio): + continue + options = line.split(':', 1)[1].split() + if 'PatchCheck.ignore-multi-package' in options: + self.ignore_multi_package = True + (START, PRE_PATCH, PATCH) = range(3) class GitDiffCheck: @@ -561,6 +573,7 @@ class CheckOnePatch: msg_check = CommitMessageCheck(self.commit_subject, self.commit_msg, self.author_email) msg_ok = msg_check.ok + self.ignore_multi_package = msg_check.ignore_multi_package diff_ok = True if self.diff is not None: @@ -671,6 +684,7 @@ class CheckGitCommits: """ def __init__(self, rev_spec, max_count): + dec_files = self.read_dec_files_from_git() commits = self.read_commit_list_from_git(rev_spec, max_count) if len(commits) == 1 and Verbose.level > Verbose.ONELINE: commits = [ rev_spec ] @@ -686,10 +700,66 @@ class CheckGitCommits: email = self.read_committer_email_address_from_git(commit) self.ok &= EmailAddressCheck(email, 'Committer').ok patch = self.read_patch_from_git(commit) - self.ok &= CheckOnePatch(commit, patch).ok + check_patch = CheckOnePatch(commit, patch) + self.ok &= check_patch.ok + ignore_multi_package = check_patch.ignore_multi_package + if PatchCheckConf.ignore_multi_package: + ignore_multi_package = True + prefix = 'WARNING: ' if ignore_multi_package else '' + check_parent = self.check_parent_packages (dec_files, commit, prefix) + if not ignore_multi_package: + self.ok &= check_parent + if not commits: print("Couldn't find commit matching: '{}'".format(rev_spec)) + def check_parent_packages(self, dec_files, commit, prefix): + ok = True + modified = self.get_parent_packages (dec_files, commit, 'AM') + if len (modified) > 1: + print("{}The commit adds/modifies files in multiple packages:".format(prefix)) + print(" *", '\n * '.join(modified)) + ok = False + deleted = self.get_parent_packages (dec_files, commit, 'D') + if len (deleted) > 1: + print("{}The commit deletes files from multiple packages:".format(prefix)) + print(" *", '\n * '.join(deleted)) + ok = False + return ok + + def get_parent_packages(self, dec_files, commit, filter): + filelist = self.read_files_modified_from_git (commit, filter) + parents = set() + for file in filelist: + dec_found = False + for dec_file in dec_files: + if os.path.commonpath([dec_file, file]): + dec_found = True + parents.add(dec_file) + if not dec_found and os.path.dirname (file): + # No DEC file found and file is in a subdir + # Covers BaseTools, .github, .azurepipelines, .pytool + parents.add(file.split('/')[0]) + return list(parents) + + def read_dec_files_from_git(self): + # run git ls-files *.dec + out = self.run_git('ls-files', '*.dec') + # return list of .dec files + try: + return out.split() + except: + return [] + + def read_files_modified_from_git(self, commit, filter): + # run git diff-tree --no-commit-id --name-only -r + out = self.run_git('diff-tree', '--no-commit-id', '--name-only', + '--diff-filter=' + filter, '-r', commit) + try: + return out.split() + except: + return [] + def read_commit_list_from_git(self, rev_spec, max_count): # Run git to get the commit patch cmd = [ 'rev-list', '--abbrev-commit', '--no-walk' ] @@ -800,6 +870,9 @@ class PatchCheckApp: group.add_argument("--ignore-change-id", action="store_true", help="Ignore the presence of 'Change-Id:' tags in commit message") + group.add_argument("--ignore-multi-package", + action="store_true", + help="Ignore if commit modifies files in multiple packages") self.args = parser.parse_args() if self.args.oneline: Verbose.level = Verbose.ONELINE @@ -807,6 +880,8 @@ class PatchCheckApp: Verbose.level = Verbose.SILENT if self.args.ignore_change_id: PatchCheckConf.ignore_change_id = True + if self.args.ignore_multi_package: + PatchCheckConf.ignore_multi_package = True if __name__ == "__main__": sys.exit(PatchCheckApp().retval) -- cgit v1.2.3