From 2c62b334ea6446f9ebaad884c7192cf51b8f9f93 Mon Sep 17 00:00:00 2001 From: Edward Lemur Date: Thu, 12 Mar 2020 22:12:33 +0000 Subject: [PATCH] git-cl: Stop using Change class from presubmit support. Bug: 1042324 Change-Id: I72db082f086f69bf49256d0613c39dc92ae0a07f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2101471 Commit-Queue: Edward Lesmes Reviewed-by: Anthony Polito --- git_cl.py | 106 +++++++++++++++++-------------------------- split_cl.py | 36 +++++++++------ tests/git_cl_test.py | 7 +-- 3 files changed, 66 insertions(+), 83 deletions(-) diff --git a/git_cl.py b/git_cl.py index a563fd95d..cc1317e2c 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1348,15 +1348,9 @@ class Changelist(object): self.issue = None self.patchset = None - def GetChange(self, upstream_branch, description): - if not self.GitSanityChecks(upstream_branch): - DieWithError('\nGit sanity check failure') - - root = settings.GetRoot() - # We use the sha1 of HEAD as a name of this change. - name = RunGitWithCode(['rev-parse', 'HEAD'])[1].strip() + def GetAffectedFiles(self, upstream): try: - files = scm.GIT.CaptureStatus(root, upstream_branch) + return [f for _, f in scm.GIT.CaptureStatus(settings.GetRoot(), upstream)] except subprocess2.CalledProcessError: DieWithError( ('\nFailed to diff against upstream branch %s\n\n' @@ -1364,21 +1358,7 @@ class Changelist(object): 'tracking branch, please run\n' ' git branch --set-upstream-to origin/master %s\n' 'or replace origin/master with the relevant branch') % - (upstream_branch, self.GetBranch())) - - issue = self.GetIssue() - patchset = self.GetPatchset() - - author = self.GetAuthor() - return presubmit_support.GitChange( - name, - description, - root, - files, - issue, - patchset, - author, - upstream=upstream_branch) + (upstream, self.GetBranch())) def UpdateDescription(self, description, force=False): assert self.GetIssue(), 'issue is required to update description' @@ -1497,34 +1477,33 @@ class Changelist(object): description = options.title + '\n\n' + message # Apply watchlists on upload. - change = self.GetChange(base_branch, description) - watchlist = watchlists.Watchlists(change.RepositoryRoot()) - files = [f.LocalPath() for f in change.AffectedFiles()] + watchlist = watchlists.Watchlists(settings.GetRoot()) + files = self.GetAffectedFiles(base_branch) if not options.bypass_watchlists: self.ExtendCC(watchlist.GetWatchersForPaths(files)) if options.reviewers or options.tbrs or options.add_owners_to: # Set the reviewer list now so that presubmit checks can access it. change_description = ChangeDescription(description) - change_description.update_reviewers(options.reviewers, - options.tbrs, - options.add_owners_to, - change) + change_description.update_reviewers( + options.reviewers, options.tbrs, options.add_owners_to, files, + self.GetAuthor()) description = change_description.description if not options.bypass_hooks: - hook_results = self.RunHook(committing=False, - may_prompt=not options.force, - verbose=options.verbose, - parallel=options.parallel, - upstream=base_branch, - description=description, - all_files=False) + hook_results = self.RunHook( + committing=False, + may_prompt=not options.force, + verbose=options.verbose, + parallel=options.parallel, + upstream=base_branch, + description=description, + all_files=False) self.ExtendCC(hook_results['more_cc']) print_stats(git_diff_args) ret = self.CMDUploadChange( - options, git_diff_args, custom_cl_base, change, description) + options, git_diff_args, custom_cl_base, description) if not ret: self._GitSetBranchConfigValue( 'last-upload-hash', RunGit(['rev-parse', 'HEAD']).strip()) @@ -2258,7 +2237,7 @@ class Changelist(object): return push_stdout def CMDUploadChange( - self, options, git_diff_args, custom_cl_base, change, message): + self, options, git_diff_args, custom_cl_base, message): """Upload the current branch to Gerrit.""" if options.squash is None: # Load default for user, repo, squash=true, in this order. @@ -2341,9 +2320,6 @@ class Changelist(object): assert len(change_ids) == 1 change_id = change_ids[0] - if options.reviewers or options.tbrs or options.add_owners_to: - change_desc.update_reviewers(options.reviewers, options.tbrs, - options.add_owners_to, change) if options.preserve_tryjobs: change_desc.set_preserve_tryjobs() @@ -2364,9 +2340,6 @@ class Changelist(object): DownloadGerritHook(False) change_desc.set_description( self._AddChangeIdToCommitMessage(message, git_diff_args)) - if options.reviewers or options.tbrs or options.add_owners_to: - change_desc.update_reviewers(options.reviewers, options.tbrs, - options.add_owners_to, change) ref_to_push = 'HEAD' # For no-squash mode, we assume the remote called "origin" is the one we # want. It is not worthwhile to support different workflows for @@ -2386,10 +2359,6 @@ class Changelist(object): 'single commit.') confirm_or_exit(action='upload') - if options.reviewers or options.tbrs or options.add_owners_to: - change_desc.update_reviewers(options.reviewers, options.tbrs, - options.add_owners_to, change) - reviewers = sorted(change_desc.get_reviewers()) cc = [] # Add CCs from WATCHLISTS and rietveld.cc git config unless this is @@ -2704,7 +2673,8 @@ class ChangeDescription(object): lines.pop(-1) self._description_lines = lines - def update_reviewers(self, reviewers, tbrs, add_owners_to=None, change=None): + def update_reviewers( + self, reviewers, tbrs, add_owners_to, affected_files, author_email): """Rewrites the R=/TBR= line(s) as a single line each. Args: @@ -2719,7 +2689,7 @@ class ChangeDescription(object): assert isinstance(tbrs, list), tbrs assert add_owners_to in (None, 'TBR', 'R'), add_owners_to - assert not add_owners_to or change, add_owners_to + assert not add_owners_to or affected_files, add_owners_to if not reviewers and not tbrs and not add_owners_to: return @@ -2748,12 +2718,12 @@ class ChangeDescription(object): # Next, maybe fill in OWNERS coverage gaps to either tbrs/reviewers. if add_owners_to: - owners_db = owners.Database(change.RepositoryRoot(), + owners_db = owners.Database(settings.GetRoot(), fopen=open, os_path=os.path) - missing_files = owners_db.files_not_covered_by(change.LocalPaths(), + missing_files = owners_db.files_not_covered_by(affected_files, (tbrs | reviewers)) LOOKUP[add_owners_to].update( - owners_db.reviewers_for(missing_files, change.author_email)) + owners_db.reviewers_for(missing_files, author_email)) # If any folks ended up in both groups, remove them from tbrs. tbrs -= reviewers @@ -4034,8 +4004,7 @@ def CMDlint(parser, args): os.chdir(settings.GetRoot()) try: cl = Changelist() - change = cl.GetChange(cl.GetCommonAncestorWithUpstream(), '') - files = [f.LocalPath() for f in change.AffectedFiles()] + files = cl.GetAffectedFiles(cl.GetCommonAncestorWithUpstream()) if not files: print('Cannot lint an empty CL') return 1 @@ -4414,9 +4383,10 @@ def CMDsplit(parser, args): def WrappedCMDupload(args): return CMDupload(OptionParser(), args) - return split_cl.SplitCl(options.description_file, options.comment_file, - Changelist, WrappedCMDupload, options.dry_run, - options.cq_dry_run, options.enable_auto_submit) + return split_cl.SplitCl( + options.description_file, options.comment_file, Changelist, + WrappedCMDupload, options.dry_run, options.cq_dry_run, + options.enable_auto_submit, settings.GetRoot()) @subcommand.usage('DEPRECATED') @@ -4929,22 +4899,28 @@ def CMDowners(parser, args): # Default to diffing against the common ancestor of the upstream branch. base_branch = cl.GetCommonAncestorWithUpstream() - change = cl.GetChange(base_branch, '') - affected_files = [f.LocalPath() for f in change.AffectedFiles()] + root = settings.GetRoot() + affected_files = cl.GetAffectedFiles(base_branch) if options.batch: - db = owners.Database(change.RepositoryRoot(), open, os.path) + db = owners.Database(root, open, os.path) print('\n'.join(db.reviewers_for(affected_files, author))) return 0 + owner_files = [f for f in affected_files if 'OWNERS' in os.path.basename(f)] + original_owner_files = { + f: scm.GIT.GetOldContents(root, f, base_branch).splitlines() + for f in owner_files} + return owners_finder.OwnersFinder( affected_files, - change.RepositoryRoot(), + root, author, [] if options.ignore_current else cl.GetReviewers(), - fopen=open, os_path=os.path, + fopen=open, + os_path=os.path, disable_color=options.no_color, - override_files=change.OriginalOwnersFiles(), + override_files=original_owner_files, ignore_author=options.ignore_self).run() diff --git a/split_cl.py b/split_cl.py index a814eb300..800482612 100644 --- a/split_cl.py +++ b/split_cl.py @@ -78,7 +78,7 @@ def AddUploadedByGitClSplitToDescription(description): def UploadCl(refactor_branch, refactor_branch_upstream, directory, files, description, comment, reviewers, changelist, cmd_upload, - cq_dry_run, enable_auto_submit): + cq_dry_run, enable_auto_submit, repository_root): """Uploads a CL with all changes to |files| in |refactor_branch|. Args: @@ -102,10 +102,17 @@ def UploadCl(refactor_branch, refactor_branch_upstream, directory, files, return # Checkout all changes to files in |files|. - deleted_files = [f.AbsoluteLocalPath() for f in files if f.Action() == 'D'] + deleted_files = [] + modified_files = [] + for action, f in files: + abspath = os.path.abspath(os.path.join(repository_root, f)) + if action == 'D': + deleted_files.append(abspath) + else: + modified_files.append(abspath) + if deleted_files: git.run(*['rm'] + deleted_files) - modified_files = [f.AbsoluteLocalPath() for f in files if f.Action() != 'D'] if modified_files: git.run(*['checkout', refactor_branch, '--'] + modified_files) @@ -140,9 +147,9 @@ def GetFilesSplitByOwners(owners_database, files): values are lists of files sharing an OWNERS file. """ files_split_by_owners = collections.defaultdict(list) - for f in files: - files_split_by_owners[owners_database.enclosing_dir_with_owners( - f.LocalPath())].append(f) + for _, f in files: + enclosing_dir = owners_database.enclosing_dir_with_owners(f) + files_split_by_owners[enclosing_dir].append(f) return files_split_by_owners @@ -172,7 +179,7 @@ def PrintClInfo(cl_index, num_cls, directory, file_paths, description, def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, - cq_dry_run, enable_auto_submit): + cq_dry_run, enable_auto_submit, repository_root): """"Splits a branch into smaller branches and uploads CLs. Args: @@ -194,8 +201,11 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, EnsureInGitRepository() cl = changelist() - change = cl.GetChange(cl.GetCommonAncestorWithUpstream(), '') - files = change.AffectedFiles() + upstream = cl.GetCommonAncestorWithUpstream() + files = [ + (action.strip(), f) + for action, f in scm.GIT.CaptureStatus(repository_root, upstream) + ] if not files: print('Cannot split an empty CL.') @@ -208,8 +218,8 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, assert refactor_branch_upstream, \ "Branch %s must have an upstream." % refactor_branch - owners_database = owners.Database(change.RepositoryRoot(), open, os.path) - owners_database.load_data_needed_for([f.LocalPath() for f in files]) + owners_database = owners.Database(repository_root, open, os.path) + owners_database.load_data_needed_for([f for _, f in files]) files_split_by_owners = GetFilesSplitByOwners(owners_database, files) @@ -232,7 +242,7 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, # Use '/' as a path separator in the branch name and the CL description # and comment. directory = directory.replace(os.path.sep, '/') - file_paths = [f.LocalPath() for f in files] + file_paths = [f for _, f in files] reviewers = owners_database.reviewers_for(file_paths, author) if dry_run: @@ -241,7 +251,7 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, else: UploadCl(refactor_branch, refactor_branch_upstream, directory, files, description, comment, reviewers, changelist, cmd_upload, - cq_dry_run, enable_auto_submit) + cq_dry_run, enable_auto_submit, repository_root) # Go back to the original branch. git.run('checkout', refactor_branch) diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index a75e81496..9b3cdb068 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -839,16 +839,13 @@ class TestGitCl(unittest.TestCase): (('ask_for_data', 'Press Enter to upload, or Ctrl+C to abort'), ''), ] - calls += [ - ((['git', 'rev-parse', 'HEAD'],), '12345'), - ] - calls += [ ((['git', 'diff', '--no-ext-diff', '--stat', '-l100000', '-C50'] + ([custom_cl_base] if custom_cl_base else [ancestor_revision, 'HEAD']),), '+dat'), ] + return calls def _gerrit_upload_calls(self, description, reviewers, squash, @@ -1595,7 +1592,7 @@ class TestGitCl(unittest.TestCase): actual = [] for orig, reviewers, tbrs, _expected in data: obj = git_cl.ChangeDescription(orig) - obj.update_reviewers(reviewers, tbrs) + obj.update_reviewers(reviewers, tbrs, None, None, None) actual.append(obj.description) self.assertEqual(expected, actual)