From b576ab3b78a9d19c33060c821d4f11643397fa30 Mon Sep 17 00:00:00 2001 From: Scott Lee Date: Mon, 13 Jan 2025 09:28:54 -0800 Subject: [PATCH] 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 --- presubmit_diff.py | 41 ++++++++++++++++++++++++++---------- tests/presubmit_diff_test.py | 19 ++++++++++++++++- 2 files changed, 48 insertions(+), 12 deletions(-) diff --git a/presubmit_diff.py b/presubmit_diff.py index f60cac072..67d722bd7 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) -> str: +def git_diff(src: str | None, dest: str | None, unified: int | 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 +54,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( - ["git", "diff", "--no-index", "--", src or DEV_NULL, dest - or DEV_NULL]).decode("utf-8") + 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") def _process_diff(diff: str, src_root: str, dst_root: str) -> str: @@ -99,7 +109,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 +128,12 @@ 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, - files: list[str]) -> dict[str, str]: +def create_diffs(host: str, repo: str, ref: str, root: str, files: list[str], + unified: int | None) -> dict[str, str]: """Calculates diffs of files in a directory against a commit. Args: @@ -131,6 +142,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 +155,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,15 +179,20 @@ 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.files, options.unified) 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 c6f5f2bfc..b78811d12 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) + files, None) self.assertEqual(actual.keys(), expected.keys()) # Manually check each line in the diffs except the "index" line because @@ -81,6 +81,7 @@ class PresubmitDiffTest(unittest.TestCase): "ref", self.root, ["doesnotexist.txt"], + None, ) def test_create_diffs_with_unchanged_file(self): @@ -89,6 +90,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