presubmit_diff: add support for -U

This is a reland of https://crrev.com/c/6168707 with fixes.
The CL adds a new param, unified, to git_diff() and create_diffs()
as a required param.

The functions themselves are tested in presubmit_diffs_test.py, but
their usages in presubmit_support.py were not, and the missing param
caused a failure in CiderG (b/389876151)

This CL basically carries the same patch, but makes the params
optional with a default value and adds unit tests for the usage
in presubmit_support.py

Tested with CiderG: https://screenshot.googleplex.com/PYEDZ3NGn5cYGtV


Change-Id: Ic747c6a133c016dbcd80034e521c76a3db3a3f60
Bug: 389876151, 379902295
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6182703
Commit-Queue: Scott Lee <ddoman@chromium.org>
Reviewed-by: Gary Tong <gatong@chromium.org>

presubmit_diff.py

@@ -45,7 +45,9 @@ def fetch_content(host: str, repo: str, ref: str, file: str) -> bytes:
     return base64.b64decode(response.read())
 
 
-def git_diff(src: str | None, dest: str | None) -> str:
+def git_diff(src: str | None,
+             dest: str | None,
+             unified: int | None = None) -> str:
     """Returns the result of `git diff --no-index` between two paths.
 
     If a path is not specified, the diff is against /dev/null. At least one of
@@ -54,13 +56,23 @@ def git_diff(src: str | None, dest: str | None) -> str:
     Args:
       src: Source path.
       dest: Destination path.
+      unified: Number of lines of context. If None, git diff uses 3 as
+        the default value.
 
     Returns:
         A string containing the git diff.
     """
-    return subprocess2.capture(
+    args = ["git", "diff", "--no-index"]
-        ["git", "diff", "--no-index", "--", src or DEV_NULL, dest
+    if unified is not None:
-         or DEV_NULL]).decode("utf-8")
+        # git diff doesn't error out even if it's given a negative <n> value.
+        # e.g., --unified=-3323, -U-3
+        #
+        # It just ignores the value and treats it as 0.
+        # hence, this script doesn't bother validating the <n> value.
+        args.append(f"-U{unified}")
+
+    args.extend(["--", src or DEV_NULL, dest or DEV_NULL])
+    return subprocess2.capture(args).decode("utf-8")
 
 
 def _process_diff(diff: str, src_root: str, dst_root: str) -> str:
@@ -99,7 +111,8 @@ def _process_diff(diff: str, src_root: str, dst_root: str) -> str:
     return header
 
 
-def _create_diff(host: str, repo: str, ref: str, root: str, file: str) -> str:
+def _create_diff(host: str, repo: str, ref: str, root: str, file: str,
+                 unified: int | None) -> str:
     new_file = os.path.join(root, file)
     if not os.path.exists(new_file):
         new_file = None
@@ -117,12 +130,16 @@ def _create_diff(host: str, repo: str, ref: str, root: str, file: str) -> str:
             raise RuntimeError(f"Could not access file {file} from {root} "
                                f"or from {host}/{repo}:{ref}.")
 
-        diff = git_diff(old_file, new_file)
+        diff = git_diff(old_file, new_file, unified)
         return _process_diff(diff, tmp_root, root)
 
 
-def create_diffs(host: str, repo: str, ref: str, root: str,
+def create_diffs(host: str,
-                 files: list[str]) -> dict[str, str]:
+                 repo: str,
+                 ref: str,
+                 root: str,
+                 files: list[str],
+                 unified: int | None = None) -> dict[str, str]:
     """Calculates diffs of files in a directory against a commit.
 
     Args:
@@ -131,6 +148,8 @@ def create_diffs(host: str, repo: str, ref: str, root: str,
       ref: Gerrit commit.
       root: Path of local directory containing modified files.
       files: List of file paths relative to root.
+      unified: Number of lines of context. If None, git diff uses 3 as
+        the default value.
 
     Returns:
         A dict mapping file paths to diffs.
@@ -142,7 +161,8 @@ def create_diffs(host: str, repo: str, ref: str, root: str,
     with concurrent.futures.ThreadPoolExecutor(
             max_workers=MAX_CONCURRENT_CONNECTION) as executor:
         futures_to_file = {
-            executor.submit(_create_diff, host, repo, ref, root, file): file
+            executor.submit(_create_diff, host, repo, ref, root, file, unified):
+            file
             for file in files
         }
         for future in concurrent.futures.as_completed(futures_to_file):
@@ -165,6 +185,12 @@ def main(argv):
     parser.add_argument("--root",
                         required=True,
                         help="Folder containing modified files.")
+    parser.add_argument("-U",
+                        "--unified",
+                        required=False,
+                        type=int,
+                        help="generate diffs with <n> lines context",
+                        metavar='<n>')
     parser.add_argument(
         "files",
         nargs="+",
@@ -173,7 +199,7 @@ def main(argv):
     options = parser.parse_args(argv)
 
     diffs = create_diffs(options.host, options.repo, options.ref, options.root,
-                         options.files)
+                         options.files, options.unified)
 
     unified_diff = "\n".join([d for d in diffs.values() if d])
     if options.output:

tests/presubmit_diff_test.py

@@ -89,6 +89,22 @@ class PresubmitDiffTest(unittest.TestCase):
             {"unchanged.txt": ""},
         )
 
+    @mock.patch('subprocess2.capture', return_value="".encode("utf-8"))
+    def test_create_diffs_executes_git_diff_with_unified(self, capture):
+        create_diffs = presubmit_diff.create_diffs
+        # None => no -U
+        create_diffs("host", "repo", "ref", self.root, ["unchanged.txt"], None)
+        capture.assert_called_with(
+            ["git", "diff", "--no-index", "--", mock.ANY, mock.ANY])
+        # 0 => -U0
+        create_diffs("host", "repo", "ref", self.root, ["unchanged.txt"], 0)
+        capture.assert_called_with(
+            ["git", "diff", "--no-index", "-U0", "--", mock.ANY, mock.ANY])
+        # 3 => -U3
+        create_diffs("host", "repo", "ref", self.root, ["unchanged.txt"], 3)
+        capture.assert_called_with(
+            ["git", "diff", "--no-index", "-U3", "--", mock.ANY, mock.ANY])
+
     def test_create_diffs_with_added_file(self):
         expected_diff = """diff --git a/added.txt b/added.txt
 new file mode 100644

tests/presubmit_support_test.py

@@ -98,19 +98,69 @@ class ProvidedDiffChangeTest(fake_repos.FakeReposTestBase):
         self._test('somewhere/else', ['not a top level file!'],
                    ['still not a top level file!'])
 
-    def test_old_contents_of_bad_diff_raises_runtimeerror(self):
+
-        diff = """
+class TestGenerateDiff(fake_repos.FakeReposTestBase):
-diff --git a/foo b/foo
+    """ Tests for --generate_diff.
-new file mode 100644
+
-index 0000000..9daeafb
+    The option is used to generate diffs of given files against the upstream
---- /dev/null
+    server as base.
-+++ b/foo
-@@ -0,0 +1 @@
-+add
     """
-        change = self._create_change(diff)
+    FAKE_REPOS_CLASS = ProvidedDiffChangeFakeRepo
-        with self.assertRaises(RuntimeError):
+
-            change._affected_files[0].OldContents()
+    def setUp(self):
+        super().setUp()
+        self.repo = os.path.join(self.FAKE_REPOS.git_base, 'repo_1')
+        self.parser = mock.Mock()
+        self.parser.error.side_effect = SystemExit
+
+    def test_with_diff_file(self):
+        """Tests that only either --generate_diff or --diff_file is allowed."""
+        options = mock.Mock(root=self.repo,
+                            all_files=False,
+                            generate_diff=True,
+                            description='description',
+                            files=None,
+                            diff_file="patch.diff")
+        with self.assertRaises(SystemExit):
+            presubmit_support._parse_change(self.parser, options)
+
+        self.parser.error.assert_called_once_with(
+            '<diff_file> cannot be specified when <generate_diff> is set.', )
+
+    @mock.patch('presubmit_diff.create_diffs')
+    def test_with_all_files(self, create_diffs):
+        """Ensures --generate_diff is noop if --all_files is specified."""
+        options = mock.Mock(root=self.repo,
+                            all_files=True,
+                            generate_diff=True,
+                            description='description',
+                            files=None,
+                            source_controlled_only=False,
+                            diff_file=None)
+        changes = presubmit_support._parse_change(self.parser, options)
+        self.assertEqual(changes.AllFiles(),
+                         ['added', 'somewhere/else', 'to_be_modified'])
+        create_diffs.assert_not_called()
+
+    @mock.patch('presubmit_diff.fetch_content')
+    def test_with_files(self, fetch_content):
+        """Tests --generate_diff with files, which should call create_diffs()."""
+        # fetch_content would return the old content of a given file.
+        # In this test case, the mocked file is a newly added file.
+        # hence, empty content.
+        fetch_content.side_effect = ['']
+        options = mock.Mock(root=self.repo,
+                            all_files=False,
+                            gerrit_url='https://chromium.googlesource.com',
+                            generate_diff=True,
+                            description='description',
+                            files=['added'],
+                            source_controlled_only=False,
+                            diff_file=None)
+        change = presubmit_support._parse_change(self.parser, options)
+        affected_files = change.AffectedFiles()
+        self.assertEqual(len(affected_files), 1)
+        self.assertEqual(affected_files[0].LocalPath(), 'added')
 
 
 class TestParseDiff(unittest.TestCase):