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 <ehmaldonado@chromium.org>
Reviewed-by: Anthony Polito <apolito@google.com>

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()
 
 

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)

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)