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

This reverts commit 9a9142793a.

Reason for revert: failed again

Original change's description:
> Reland "add support for -U in presubmit_diff.py"
>
> This reverts commit 4c54361841.
>
> 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 b576ab3b78.
> >
> > 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>
>
> Bug: 379902295
> Change-Id: Icbc4aa98bbfaa816143be064217fb2d992b48baf
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6173762
> Reviewed-by: Gavin Mak <gavinmak@google.com>
> Commit-Queue: Scott Lee <ddoman@chromium.org>

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 <ddoman@chromium.org>
Reviewed-by: Gary Tong <gatong@chromium.org>
changes/76/6177776/2
Scott Lee 3 months ago committed by LUCI CQ
parent 232ffd8df3
commit 7b9ced4220

@ -45,7 +45,7 @@ def fetch_content(host: str, repo: str, ref: str, file: str) -> bytes:
return base64.b64decode(response.read()) 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. """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 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: Args:
src: Source path. src: Source path.
dest: Destination path. dest: Destination path.
unified: Number of lines of context. If None, git diff uses 3 as
the default value.
Returns: Returns:
A string containing the git diff. A string containing the git diff.
""" """
args = ["git", "diff", "--no-index"] return subprocess2.capture(
if unified is not None: ["git", "diff", "--no-index", "--", src or DEV_NULL, dest
# git diff doesn't error out even if it's given a negative <n> value. or DEV_NULL]).decode("utf-8")
# 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: 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 return header
def _create_diff(host: str, def _create_diff(host: str, repo: str, ref: str, root: str, file: str) -> str:
repo: str,
ref: str,
root: str,
file: str,
unified: int | None = None) -> str:
new_file = os.path.join(root, file) new_file = os.path.join(root, file)
if not os.path.exists(new_file): if not os.path.exists(new_file):
new_file = None new_file = None
@ -132,12 +117,12 @@ def _create_diff(host: str,
raise RuntimeError(f"Could not access file {file} from {root} " raise RuntimeError(f"Could not access file {file} from {root} "
f"or from {host}/{repo}:{ref}.") 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) return _process_diff(diff, tmp_root, root)
def create_diffs(host: str, repo: str, ref: str, root: str, files: list[str], def create_diffs(host: str, repo: str, ref: str, root: str,
unified: int | None) -> dict[str, str]: files: list[str]) -> dict[str, str]:
"""Calculates diffs of files in a directory against a commit. """Calculates diffs of files in a directory against a commit.
Args: Args:
@ -146,8 +131,6 @@ def create_diffs(host: str, repo: str, ref: str, root: str, files: list[str],
ref: Gerrit commit. ref: Gerrit commit.
root: Path of local directory containing modified files. root: Path of local directory containing modified files.
files: List of file paths relative to root. 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: Returns:
A dict mapping file paths to diffs. 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( with concurrent.futures.ThreadPoolExecutor(
max_workers=MAX_CONCURRENT_CONNECTION) as executor: max_workers=MAX_CONCURRENT_CONNECTION) as executor:
futures_to_file = { futures_to_file = {
executor.submit(_create_diff, host, repo, ref, root, file, unified): executor.submit(_create_diff, host, repo, ref, root, file): file
file
for file in files for file in files
} }
for future in concurrent.futures.as_completed(futures_to_file): for future in concurrent.futures.as_completed(futures_to_file):
@ -183,20 +165,15 @@ def main(argv):
parser.add_argument("--root", parser.add_argument("--root",
required=True, required=True,
help="Folder containing modified files.") 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( parser.add_argument(
"files", "files",
nargs="+", nargs="+",
help="List of changed files. Paths are relative to the repo root.", help="List of changed files. Paths are relative to the repo root.",
) )
options = parser.parse_args(argv) options = parser.parse_args(argv)
diffs = create_diffs(options.host, options.repo, options.ref, options.root, 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]) unified_diff = "\n".join([d for d in diffs.values() if d])
if options.output: if options.output:

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

Loading…
Cancel
Save