pre-upload: use RawDiff from chromite

BUG=chromium:397714
TEST=upload checks still work

Change-Id: I7695adb51fbb4a7da72c5f7cba1646950c36753b
Reviewed-on: https://chromium-review.googlesource.com/231613
Reviewed-by: Doug Anderson <dianders@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
diff --git a/pre-upload.py b/pre-upload.py
index a4f1414..a619cac 100755
--- a/pre-upload.py
+++ b/pre-upload.py
@@ -17,8 +17,8 @@
 import os
 import re
 import sys
-import subprocess
 import stat
+import subprocess
 
 from errors import (VerifyException, HookFailure, PrintErrorForProject,
                     PrintErrorsForCommit)
@@ -29,6 +29,7 @@
 if __name__ in ('__builtin__', '__main__'):
   sys.path.insert(0, os.path.join(os.path.dirname(sys.argv[0]), '..', '..'))
 
+from chromite.lib import git
 from chromite.lib import osutils
 from chromite.lib import patch
 from chromite.licensing import licenses_lib
@@ -239,45 +240,6 @@
   return new_lines
 
 
-def _parse_affected_files(output, include_deletes=False, relative=False):
-  """Parses git diff's 'raw' format, returning a list of modified file paths.
-
-  This excludes directories and symlinks, and optionally includes files that
-  were deleted.
-
-  Args:
-    output: The result of the 'git diff --raw' command
-    include_deletes: If true, we'll include deleted files in the result
-    relative: Whether to return relative or full paths to files
-
-  Returns:
-    A list of modified/added (and perhaps deleted) files
-  """
-  files = []
-  # See the documentation for 'git diff --raw' for the relevant format.
-  for statusline in output.splitlines():
-    attributes, paths = statusline.split('\t', 1)
-    _, mode, _, _, status = attributes.split(' ')
-
-    # Ignore symlinks and directories.
-    imode = int(mode, 8)
-    if stat.S_ISDIR(imode) or stat.S_ISLNK(imode):
-      continue
-
-    # Ignore deleted files, and optionally return absolute paths of files.
-    if include_deletes or status != 'D':
-      # If a file was merely modified, we will have a single file path.
-      # If it was moved, we will have two paths (source and destination).
-      # In either case, we want the last path.
-      f = paths.split('\t')[-1]
-      if not relative:
-        pwd = os.getcwd()
-        f = os.path.join(pwd, f)
-      files.append(f)
-
-  return files
-
-
 def _get_affected_files(commit, include_deletes=False, relative=False):
   """Returns list of file paths that were modified/added, excluding symlinks.
 
@@ -292,8 +254,22 @@
   if commit == PRE_SUBMIT:
     return _run_command(['git', 'diff-index', '--cached',
                          '--name-only', 'HEAD']).split()
-  output = _run_command(['git', 'diff', '--raw', commit + '^!'])
-  return _parse_affected_files(output, include_deletes, relative)
+
+  path = os.getcwd()
+  files = git.RawDiff(path, '%s^!' % commit)
+
+  # Filter out symlinks.
+  files = [x for x in files if not stat.S_ISLNK(int(x.dst_mode, 8))]
+
+  if not include_deletes:
+    files = [x for x in files if x.status != 'D']
+
+  # Caller only cares about filenames.
+  files = [x.dst_file if x.dst_file else x.src_file for x in files]
+  if relative:
+    return files
+  else:
+    return [os.path.join(path, x) for x in files]
 
 
 def _get_commits():
diff --git a/pre-upload_unittest.py b/pre-upload_unittest.py
index e2d14bf..b256895 100755
--- a/pre-upload_unittest.py
+++ b/pre-upload_unittest.py
@@ -24,6 +24,7 @@
   sys.path.insert(0, os.path.join(os.path.dirname(sys.argv[0]), '..', '..'))
 
 from chromite.lib import cros_test_lib
+from chromite.lib import git
 from chromite.lib import osutils
 
 
@@ -403,31 +404,6 @@
     self.assertTrue(isinstance(ret, errors.HookFailure))
 
 
-class CheckGitOutputParsing(cros_test_lib.MockTestCase):
-  """Tests for git output parsing."""
-
-  def testParseAffectedFiles(self):
-    """Test parsing git diff --raw output."""
-    # Sample from git diff --raw.
-    sample_git_output = '\n'.join([
-        ":100644 100644 ff03961... a198e8b... M\tMakefile",
-        ":100644 000000 e69de29... 0000000... D\tP1/P2",
-        ":100755 100644 454d5ef... 0000000... C86\tP3\tP4",
-        ":100755 100644 454d5ef... 0000000... R86\tP5\tP6/P7",
-        ":100755 120644 454d5ef... 0000000... M\tIsASymlink",
-    ])
-    expected_modified_files_no_deletes = ['Makefile', 'P4', 'P6/P7']
-    expected_modified_files_with_deletes = ['Makefile', 'P1/P2', 'P4', 'P6/P7']
-    result = pre_upload._parse_affected_files(sample_git_output,
-                                              include_deletes=True,
-                                              relative=True)
-    self.assertEqual(result, expected_modified_files_with_deletes)
-    result = pre_upload._parse_affected_files(sample_git_output,
-                                              include_deletes=False,
-                                              relative=True)
-    self.assertEqual(result, expected_modified_files_no_deletes)
-
-
 class CheckLicenseCopyrightHeader(cros_test_lib.MockTestCase):
   """Tests for _check_license."""
 
@@ -672,5 +648,61 @@
     self.assertMessageRejected('o' * 200)
 
 
+class HelpersTest(cros_test_lib.MockTestCase):
+  """Various tests for utility functions."""
+
+  def _SetupGetAffectedFiles(self):
+    self.PatchObject(git, 'RawDiff', return_value=[
+        # A modified normal file.
+        git.RawDiffEntry(src_mode='100644', dst_mode='100644', src_sha='abc',
+                         dst_sha='abc', status='M', score=None,
+                         src_file='buildbot/constants.py', dst_file=None),
+        # A new symlink file.
+        git.RawDiffEntry(src_mode='000000', dst_mode='120000', src_sha='abc',
+                         dst_sha='abc', status='A', score=None,
+                         src_file='scripts/cros_env_whitelist', dst_file=None),
+        # A deleted file.
+        git.RawDiffEntry(src_mode='100644', dst_mode='000000', src_sha='abc',
+                         dst_sha='000000', status='D', score=None,
+                         src_file='scripts/sync_sonic.py', dst_file=None),
+    ])
+
+  def testGetAffectedFilesNoDeletesNoRelative(self):
+    """Verify _get_affected_files() works w/no delete & not relative."""
+    self._SetupGetAffectedFiles()
+    path = os.getcwd()
+    files = pre_upload._get_affected_files('HEAD', include_deletes=False,
+                                           relative=False)
+    exp_files = [os.path.join(path, 'buildbot/constants.py')]
+    self.assertEquals(files, exp_files)
+
+  def testGetAffectedFilesDeletesNoRelative(self):
+    """Verify _get_affected_files() works w/delete & not relative."""
+    self._SetupGetAffectedFiles()
+    path = os.getcwd()
+    files = pre_upload._get_affected_files('HEAD', include_deletes=True,
+                                           relative=False)
+    exp_files = [os.path.join(path, 'buildbot/constants.py'),
+                 os.path.join(path, 'scripts/sync_sonic.py')]
+    self.assertEquals(files, exp_files)
+
+  def testGetAffectedFilesNoDeletesRelative(self):
+    """Verify _get_affected_files() works w/no delete & relative."""
+    self._SetupGetAffectedFiles()
+    files = pre_upload._get_affected_files('HEAD', include_deletes=False,
+                                           relative=True)
+    exp_files = ['buildbot/constants.py']
+    self.assertEquals(files, exp_files)
+
+  def testGetAffectedFilesDeletesRelative(self):
+    """Verify _get_affected_files() works w/delete & relative."""
+    self._SetupGetAffectedFiles()
+    path = os.getcwd()
+    files = pre_upload._get_affected_files('HEAD', include_deletes=True,
+                                           relative=True)
+    exp_files = ['buildbot/constants.py', 'scripts/sync_sonic.py']
+    self.assertEquals(files, exp_files)
+
+
 if __name__ == '__main__':
   cros_test_lib.main()