The presubmit format validation hook and git cl format is given or
generate a patch such that they check/format the modified lines only.
The context lines matter because the diff parsers extract the line
numbers and length from the diffs, which include the context lines,
if included. For instance, if a patch is generated with -U3,
the presubmit hook actually verifies the format of not only the changed
lines but also the context lines. Therefore, it's important to make sure
that all the diff(s) commands use the same -U to ensure that they check
and format the same lines.
crrev.com/c/6952890 was submitted to match the context line numbers.
However, it was missing one case. The Chromium Cider extension also
generates the diff but with -U3. Therefore, cl/834917755 was made
to have it generating patches with -U0.
Another issue is that "git apply" fails by default if a given patch
was generated with -U0. It expects context lines in case a user tries
to apply a patch into a different commit so that it could adjust
the patches and applies even if the location of the corresponding,
original block has changed in the file to apply the patch.
However, the scenario of smartly modifying the patch to make it
applicable to as many commits as possible is not applicable to
depot_tools' case. The patch is not made to be shared with others.
It's generated instantly for a specific, base commit and applies it
to the current commit.
"--unidiff-zero" is the option to allow patches even if it was
generated with -U0. It's fine to use the option because the smart
patch adjustment is not applicable to our use cases.
This CL adds the option to _ProvidedDiffCache() so that it is
happy with the patch generated with -U0. The reason why the other
functionalities have been working, even though crrev.com/c/6952890
changed the context option to -U0 is that other logic doesn't use
`git apply`. They just parse the hunks and use the numbers.
Bug: 458442277
Change-Id: I8af98e9a8bcbd8d3175a162fced127ff89ce526c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7211480
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
Commit-Queue: Scott Lee <ddoman@chromium.org>
The diff hunk can omit the number of lines after the line position.
For example, the following hunks are all valid
- @@ -1,2 +1,3 @@
- @@ -1 +1,3 @@
- @@ -1,2 +1 @@
- @@ -1 +1 @@
However, the existing regex assumes that the number of lines is always
given. It wasn't a problem before crrev.com/c/6952890, because -U3 was
applied always. However, since it changed the context lines to -U0,
git diff started generating hunks without the number of lines, if
a given changed section contains only a single line.
This CL fixes the regex.
Bug: 453641840
Change-Id: I67f680bc1d8fa046e373b065e8ccbb6ce652eb3f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7158599
Auto-Submit: Scott Lee <ddoman@chromium.org>
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
Commit-Queue: Scott Lee <ddoman@chromium.org>
".gn" was not a recognized extension for the `mimetypes.guess_type` function. As a result, files like "BUILD.gn" weren't added to affected files, and so some presubmits fail. (Specifically, the presubmit which requires new header files to be added to a BUILD.gn file.)
Bug: b:456124666
Change-Id: I38c4d8007e242acd218cf130d3b81d71b0c189ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7102959
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Terrence Reilly <treilly@google.com>
git cl format uses `git diff -U0` when it has to find the files
and ranges to format. Similarly, CiderG generates diffs with
-U0 and run presubmits for the checks.
However, the scm.py:GenerateDiff(), which CheckPatchFormatted() uses,
doesn't specify -U and it defaults to -U3.
This CL updates CheckPatchFormatted() to pass -U0 to
scm.py:GenerateDiff() so that Presubmit and git cl format are
always given diffs generated with -U0.
Bug: 386840832
Change-Id: I19479ab139b7dba8ba4e5e5fed89eca4d2e66412
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6952890
Auto-Submit: Scott Lee <ddoman@chromium.org>
Commit-Queue: Brian Ryner <bryner@google.com>
Commit-Queue: Scott Lee <ddoman@chromium.org>
Reviewed-by: Brian Ryner <bryner@google.com>
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 <ddoman@chromium.org>
Reviewed-by: Gavin Mak <gavinmak@google.com>
The existing licence regex assumed each line would start with an unknown
and optional comment delimiter, followed by a required space. However
the standard way to write the licence in XML files uses a multi-line
comment, each individual line simply starts with the licence text, no
delimiter or space. The license check thus needed to be updated to make
the space optional as well.
Bug: 407060754
Change-Id: I4a83dd9408de890593952e441ba6b650f726907c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6409392
Commit-Queue: Sky Malice <skym@chromium.org>
Reviewed-by: Andy Perelson <ajp@google.com>
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
As part of new findings cider extension work, we need to show
presubmit result as findings in the new panel. The extension only
has access to stdout/stderr of the process executing presubmit
checks. Therefore, we need a way to write structured data
(i.e. json) to the stdout.
Bug: 397700948
Change-Id: Ie9e51d39389613c5f93ca054102b169a99e356b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6330090
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
Commit-Queue: Yiwei Zhang <yiwzhang@google.com>
This adds AffectedFile.UnixLocalPath(), Change.UnixLocalPaths(), and
input_api.UnixLocalPaths() to the presubmit support APIs, so that we can
check paths in a platform-independent manner (i.e., not have to worry
about whether we're running on Windows and might need to handle
backslashes).
Change-Id: I1a424825384cb950ef2d4d3008c9c6e60028cf07
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6236404
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Dirk Pranke <dpranke@google.com>
When a presubmit warning is triggered this message is printed:
There were Python 3 presubmit warnings.
The Python version information was useful when we were transitioning
our presubmits from Python 2 to Python 3, but that transition is long
finished, and now that information is just noise.
This change deletes the excess information.
Change-Id: I4e43e1c9daf36daaaa9430d76ad557c800cf962b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5907924
Commit-Queue: Gavin Mak <gavinmak@google.com>
Reviewed-by: Gavin Mak <gavinmak@google.com>
Auto-Submit: Bruce Dawson <brucedawson@chromium.org>
NewContents always reads from cache, but it is sometimes useful flush
cache and use NewContents as a way to read the updatedcontents of a
file.
For example, some presubmit checks run formatters and check if any files
are modified. Many of these use `git diff` but that isn't available in
a non-git workspace, so checks will have to rely on reading contents
directly.
Bug: b/333744051
Change-Id: I3b3f4c88cc130607020ad599bbc15616d559725c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5740609
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
Commit-Queue: Gavin Mak <gavinmak@google.com>
For a ProvidedDiffChange, the AllFiles method naively returns all files
with rglob("*"). The returned list includes files in nested submodules.
This does not match the behavior of GitChange's AllFiles which uses
git ls-files to find all files.
Implement a new SCM that stops iterating recursively when it
sees a submodule from .gitmodules in the repo root.
Bug: b/323243527
Change-Id: I170d0f1bc4a838acea04779dee3df7fca0bce359
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5648616
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
Commit-Queue: Gavin Mak <gavinmak@google.com>
GitChange uses 'scm.GIT.ListSubmodules' to filter out files under repo
root that belong to a submodule. ProvidedDiffChange does not use this
so doesn't filter out submoduled files.
'scm.GIT.ListSubmodules' just reads '.gitmodules' at repo root and
can be run outside a git workspace. Use this to provide useful
submodule info for Change/ProvidedDiffChange.
Bug: b/333744051
Change-Id: Idaead11c69681e86276a29b0dc58090e7c4ec2c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5466527
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
Commit-Queue: Gavin Mak <gavinmak@google.com>
+ Change scm GetAllFiles back to returning all files
+ should be safe to do since it's only used by presubmit code
+ add ignore_submodules option to CaptureStatus
+ It's used by other parts of depot_tools code
+ Add AffectedSubmodules to return submodules only
+ AffectedFiles still only returns non-submodule files.
Bug: 1475770
Change-Id: I457ad96099fb20eff4cc1a4fc84f59e7ae707abe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5216005
Auto-Submit: Joanna Wang <jojwang@chromium.org>
Commit-Queue: Joanna Wang <jojwang@chromium.org>
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
