From 7b9ced4220177d94e448e0bb13b6fe807dcaa736 Mon Sep 17 00:00:00 2001 From: Scott Lee Date: Wed, 15 Jan 2025 10:38:03 -0800 Subject: [PATCH] Revert "Reland "add support for -U in presubmit_diff.py"" This reverts commit 9a9142793a90d65cd1424f6826be89ec15bfa233. Reason for revert: failed again Original change's description: > Reland "add support for -U in presubmit_diff.py" > > This reverts commit 4c54361841933e29ec19a3104e4f11f0e898674a. > > Reason for revert: Reland with a fix. > > Find https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6173762/1..2 > Tested by running presubmit_support.py --generate_diff > > > Original change's description: > > 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 > > > Commit-Queue: Scott Lee > > > > Bug: 379902295 > > Change-Id: I82dd707e5ae3d4b1760e632506ee0e1bc1d76e09 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6173760 > > Reviewed-by: Scott Lee > > Commit-Queue: Gavin Mak > > Bot-Commit: Rubber Stamper > > Bug: 379902295 > Change-Id: Icbc4aa98bbfaa816143be064217fb2d992b48baf > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6173762 > Reviewed-by: Gavin Mak > Commit-Queue: Scott Lee Bug: 379902295 Change-Id: I84875f6667689e1a9085876555bc6aef4ea2d7b4 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6177776 Commit-Queue: Scott Lee Reviewed-by: Gary Tong --- presubmit_diff.py | 45 +++++++++--------------------------- tests/presubmit_diff_test.py | 19 +-------------- 2 files changed, 12 insertions(+), 52 deletions(-) diff --git a/presubmit_diff.py b/presubmit_diff.py index 384b90c9d..f60cac072 100755 --- a/presubmit_diff.py +++ b/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 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 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,12 +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 = 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 @@ -132,12 +117,12 @@ def _create_diff(host: 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: @@ -146,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. @@ -159,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): @@ -183,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 lines context", - metavar='') 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: diff --git a/tests/presubmit_diff_test.py b/tests/presubmit_diff_test.py index b78811d12..c6f5f2bfc 100755 --- a/tests/presubmit_diff_test.py +++ b/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