From 077e02a7696bccafcdd169992524830fa65b738e Mon Sep 17 00:00:00 2001 From: Scott Lee Date: Fri, 12 Sep 2025 10:34:21 -0700 Subject: [PATCH] presubmit: generate a diff with prefix a/ and b/ Historically, git_cl.py and presubmit_support.py generated diffs inconsistently. ==== git_cl.py ==== 1) git_cl.py:_RunClangFormatDiff() assumed the input diff was generated with --no-prefix. Therefore, it hard-coded -p0 as a param for clang_format_diff.py. If the diff was generated without --no-prefix, it would have to pass -p1 instead. 2) git_cl.py _RunYapf() and _RunGoogleJavaFormat assumed that the input diff was generated with prefixes. Therefore, it parsed the diffs, assuming that the diff was generated with a/ and b/ prefixes. This discrepancy wasn't an issue because each of the Run functions generated and consumed the diffs within themselves. It became an issue when https://crrev.com/c/6931775 was developed. The CL consolidated the diff generation into a common function, so that all _Run functions need to agree with the presence of the prefixes. The CL changed _RunClangFormatDiff() to use -p1 instead of -p0, mainly because - it's the p level for the default git-diff option - it's the p level for the unix diff util - CiderG Chromium also generates the diff with the prefixes to emulate the default diff output. - I highly believe that --no-prefix was chosen for no reasons. It seems to be a random choice to work with -p0. - Either Java/Python or Clang wrapper should be changed. ==== presubmit_support.py ==== presubmit_support.py can be given a diff_file via --diff_file. Otherwise, it generates the diff based on --upstream_commit. a) it doesn't enforce the presence of the prefixes in the input diff_file. As stated above, the Chromium extension generates a diff with prefixes. b) in contrast, when it is not given --diff_file, it generates the diff with --no-prefix, and I don't find any reasons for it. I believe that it's from a copy-pasted random choice. ==== the problem ==== This discarepancy became a problem, when crrev.com/c/6937365 was landed. It enforces git_cl.py and presubmit_support.py to agree for the format of the input diff. That is, the presence of the prefixes must be agreed by all the following. - presubmit_support.py with --diff_file /tmp/abc - presubmit_suppory.py without --diff_file /tmp/abc - git cl format - git cl format --input_diff_file /tmp/abc ==== possible solutions ==== Obviously, there are 3 choices. 1) update the regex and parsers of git_cl.py to auto detect the best -p{num}. This was the least preferred option, as it can be fragile. 2) update Chromium extension and git_cl.py to use --no-prefix. i.e., - update the Chromium CiderG to generate diffs without the prefix. - update the regex used in _ComputeFormatDiffLineRanges() to assume that the input is generated with --no-prefix. - change _RunClangFormatDiff() to pass -p0 instead of -p1. 3) update presubmit_support.py to generate diff without --no-prefix. ==== What this CL does ==== It implements (3). - That is the default option for git-diff. - I don't find obvious reasons to use --no-prefix. - CiderG has been generating diffs with prefixes for weeks. - It implies that presubmit_support.py works fine with a prefixed diff. Bug: 386840832 Change-Id: Iac8a4fc30f101e70e3ccc56f9f8ee48198dfa833 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6939737 Commit-Queue: Scott Lee Reviewed-by: Gavin Mak --- presubmit_support.py | 3 ++- scm.py | 15 ++++++++++++--- tests/presubmit_unittest.py | 3 ++- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/presubmit_support.py b/presubmit_support.py index c6242495fc..3e549a281a 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -1059,7 +1059,8 @@ class _GitDiffCache(_DiffCache): files=[], full_move=True, branch=self._upstream, - branch_head=self._end_commit) + branch_head=self._end_commit, + allow_prefix=True) # Compute a single diff for all files and parse the output; with git # this is much faster than computing one diff for each file. self._diffs_by_file = _parse_unified_diff(unified_diff) diff --git a/scm.py b/scm.py index 1793b400f3..1df082af97 100644 --- a/scm.py +++ b/scm.py @@ -1020,7 +1020,8 @@ class GIT(object): branch: Optional[str] = None, branch_head: str = 'HEAD', full_move: bool = False, - files: Optional[Iterable[str]] = None) -> str: + files: Optional[Iterable[str]] = None, + allow_prefix: bool = False) -> str: """Diffs against the upstream branch or optionally another branch. full_move means that move or copy operations should completely recreate the @@ -1034,10 +1035,18 @@ class GIT(object): 'diff', '-p', '--no-color', - '--no-prefix', '--no-ext-diff', - branch + "..." + branch_head, ] + + if allow_prefix: + # explicitly setting --src-prefix and --dst-prefix is necessary in the + # case that diff.noprefix is set in the user's git config. + command.extend(['--src-prefix=a/', '--dst-prefix=b/']) + else: + command.extend(['--no-prefix']) + + command.append(branch + "..." + branch_head) + if full_move: command.append('--no-renames') else: diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 3c6584f658..c863b9edd4 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -419,7 +419,8 @@ class PresubmitUnittest(PresubmitTestsBase): files=[], full_move=True, branch='upstream', - branch_head='end_commit') + branch_head='end_commit', + allow_prefix=True) f_blat = os.path.normpath('boo/blat.cc')