Browse Source

Revert "add support for -U in presubmit_diff.py"

This reverts commit b576ab3b78a9d19c33060c821d4f11643397fa30.

Reason for revert: http://b/389876151

Original change's description:
> add support for -U in presubmit_diff.py
>
> presubmit_diff.py is going to be used to compute the changes to be
> formatted, and -U helps minimize the number of irrelevant lines
> from formatting.
>
> Bug: 379902295
> Change-Id: I9c0a2ee6b5ffa6b9fe4427362556020d525f1105
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6168707
> Reviewed-by: Gavin Mak <gavinmak@google.com>
> Commit-Queue: Scott Lee <ddoman@chromium.org>

Bug: 379902295
Change-Id: I82dd707e5ae3d4b1760e632506ee0e1bc1d76e09
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6173760
Reviewed-by: Scott Lee <ddoman@chromium.org>
Commit-Queue: Gavin Mak <gavinmak@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Gavin Mak 7 months ago
parent
commit
4c54361841
2 changed files with 12 additions and 48 deletions
  1. 11 30
      presubmit_diff.py
  2. 1 18
      tests/presubmit_diff_test.py

+ 11 - 30
presubmit_diff.py

@@ -45,7 +45,7 @@ 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, unified: int | None) -> str:
+def git_diff(src: str | None, dest: str | 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,23 +54,13 @@ def git_diff(src: str | None, dest: str | None, unified: int | 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.
     """
-    args = ["git", "diff", "--no-index"]
-    if unified is not None:
-        # 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")
+    return subprocess2.capture(
+        ["git", "diff", "--no-index", "--", src or DEV_NULL, dest
+         or DEV_NULL]).decode("utf-8")
 
 
 def _process_diff(diff: str, src_root: str, dst_root: str) -> str:
@@ -109,8 +99,7 @@ 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,
-                 unified: int | None) -> str:
+def _create_diff(host: str, repo: str, ref: str, root: str, file: str) -> str:
     new_file = os.path.join(root, file)
     if not os.path.exists(new_file):
         new_file = None
@@ -128,12 +117,12 @@ def _create_diff(host: str, repo: str, ref: str, root: str, file: str,
             raise RuntimeError(f"Could not access file {file} from {root} "
                                f"or from {host}/{repo}:{ref}.")
 
-        diff = git_diff(old_file, new_file, unified)
+        diff = git_diff(old_file, new_file)
         return _process_diff(diff, tmp_root, root)
 
 
-def create_diffs(host: str, repo: str, ref: str, root: str, files: list[str],
-                 unified: int | None) -> dict[str, str]:
+def create_diffs(host: str, repo: str, ref: str, root: str,
+                 files: list[str]) -> dict[str, str]:
     """Calculates diffs of files in a directory against a commit.
 
     Args:
@@ -142,8 +131,6 @@ def create_diffs(host: str, repo: str, ref: str, root: str, files: list[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.
@@ -155,8 +142,7 @@ def create_diffs(host: str, repo: str, ref: str, root: str, files: list[str],
     with concurrent.futures.ThreadPoolExecutor(
             max_workers=MAX_CONCURRENT_CONNECTION) as executor:
         futures_to_file = {
-            executor.submit(_create_diff, host, repo, ref, root, file, unified):
-            file
+            executor.submit(_create_diff, host, repo, ref, root, file): file
             for file in files
         }
         for future in concurrent.futures.as_completed(futures_to_file):
@@ -179,20 +165,15 @@ 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="+",
         help="List of changed files. Paths are relative to the repo root.",
     )
     options = parser.parse_args(argv)
+
     diffs = create_diffs(options.host, options.repo, options.ref, options.root,
-                         options.files, options.unified)
+                         options.files)
 
     unified_diff = "\n".join([d for d in diffs.values() if d])
     if options.output:

+ 1 - 18
tests/presubmit_diff_test.py

@@ -60,7 +60,7 @@ class PresubmitDiffTest(unittest.TestCase):
 
     def _test_create_diffs(self, files: List[str], expected: Dict[str, str]):
         actual = presubmit_diff.create_diffs("host", "repo", "ref", self.root,
-                                             files, None)
+                                             files)
         self.assertEqual(actual.keys(), expected.keys())
 
         # Manually check each line in the diffs except the "index" line because
@@ -81,7 +81,6 @@ class PresubmitDiffTest(unittest.TestCase):
             "ref",
             self.root,
             ["doesnotexist.txt"],
-            None,
         )
 
     def test_create_diffs_with_unchanged_file(self):
@@ -90,22 +89,6 @@ 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