On Windows, the command line has a character limit of 8191. When checking for commit objects in submodules, the `git ls-tree` command can become very long if many files are affected, potentially exceeding this limit.
This change modifies `CheckForCommitObjects` to check the estimated length of the `git ls-tree` command when running on Windows. If the command line with all affected files would exceed the limit, it falls back to using a recursive `ls-tree` (`-r`) instead of listing each file individually. This prevents command line overflow errors on Windows.
Change-Id: I6a340baefee57f5933473add0601a42ff1e61bb6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7254474
Auto-Submit: Josiah Kiehl <kiehl@google.com>
Commit-Queue: Josiah Kiehl <kiehl@google.com>
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
Significantly reduces the execution time of presubmit checks by
optimizing CheckForCommitObjects.
For CLs with fewer than 1000 affected files, run `git ls-tree` only on
specific files instead of scanning the full tree. This yields a ~70x
speedup (~0.97s -> ~0.01s) for typical CLs.
Change-Id: Ia8b89dbb14a5129ba79944282deba52a3558bdf2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7234371
Commit-Queue: Josiah Kiehl <kiehl@google.com>
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
This CL optimizes CheckForCommitObjects in two ways:
- Fast Path: If the raw `git ls-tree` output does not contain `160000` (gitlink mode), return early. This avoids parsing entirely for repositories without submodules at the cost of about 3ms.
- Iterative Parsing: For repositories with submodules, use an iterative find loop to locate gitlink entries instead of splitting the entire tree content.
On the Chromium repo:
- Speed: The parsing step is ~7.7x faster (0.04s vs 0.30s).
- Memory: Saves ~78 MB of memory per run by avoiding the creation of ~300,000 string objects during the split.
I also added tests now that it's more than a simple loop.
Change-Id: I903effc50aaedb9130772491ad38385b22477d58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7231149
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
Commit-Queue: Josiah Kiehl <kiehl@google.com>
Makes the following changes related to the exception made for fake
Chromium checkouts when checking git submodules:
1. Adds an instance of this being used in Dawn to the comment
2. Adds third_party/placeholder_chromium as an allowed dependency name
since the "dummy" from third_party/dummy_chromium runs into the
inclusive language checks.
Bug: 452840620
Change-Id: I65099d150cf9c4971e3183f1cb208df668582e4c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7170740
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Commit-Queue: Scott Lee <ddoman@chromium.org>
Auto-Submit: Brian Sheedy <bsheedy@chromium.org>
Reviewed-by: Scott Lee <ddoman@chromium.org>
This updates the CheckInfraFreeze/CheckChromiumInfraFreeze functions to
operate on the _CHROMIUM_FREEZES list of tuples with start, end and
details that define freeze periods rather than a single freeze period
defined by the _CHROMIUM_FREEZE_START, _CHROMIUM_FREEZE_END and
_CHROMIUM_FREEZE_DETAILS values. This will enable defining the freeze
periods for end-of-year all at once rather than requiring an update
between the November and December/January periods.
The periods have been set to match the announced end-of-year freeze
dates.
Change-Id: I7ea9d19fad71d086cef3de0e02ea40fda35460ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7090808
Commit-Queue: Garrett Beaty <gbeaty@google.com>
Reviewed-by: Scott Lee <ddoman@chromium.org>
CheckPatchFormatted passes the directory path of a given input file
to `git cl format` command if the directory doesn't directly contain
PRESUBMIT.py.
Now the presubmit check uses a diff file which includes a full list
of files to check the format of, and passing the directory is
no longer necessary.
Bug: 449775904
Change-Id: I50cdd58f46d058bbf5274f926578572573d814cc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7018893
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Scott Lee <ddoman@chromium.org>
This check is making any script which runs `git cl presubmit` locally fail.
In particular, `jj upload` runs `git cl presubmit`, and then uploads
only if it succeeds (we cannot run `git cl upload`, because that can
specifically only upload the currently checked out commit).
According to the bug I linked, "I think that historically it was possible
for developers to try to commit directly from their Chromium repo,
without uploading, and this aims to stop that. It also warns users that
their change has not been uploaded. Whether it still needs to be an
error is not clear, but I'm not sure it's causing any problems so I'm
inclined to leave it."
To sum things up:
* Since presubmits can be bypassed, this is not a security measure
* These days I don't believe you can commit directly from your chromium
repo
* It's now starting to cause problems.
Bug: 40253731
Change-Id: I0d83fa68d0e65dcdd8932c7d3053750dcc16d3fb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6875404
Reviewed-by: Scott Lee <ddoman@chromium.org>
Commit-Queue: Matt Stark <msta@google.com>
In https://crrev.com/c/6697693, presubmit's canned formatting checks
started looking at *.star files. This included enforcing a max line
length of 80 chars in such files.
Most (all?) *.star files found in gclient-managed repos are lucicfg
client repos, which generally have their own formatter and use 4-space
indents. This can lead to some friction with presubmit's 80 char limit.
So this uncaps that limit for all *.star files.
Bug: None
Change-Id: Ice68676c5d097d7951e83cf81ca9ac8adc8151ed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6862147
Reviewed-by: Scott Lee <ddoman@chromium.org>
Commit-Queue: Ben Pastene <bpastene@chromium.org>
This reverts commit eb60ab38de.
Reason for revert: re-landing with an additional patch
-------------
* Problem
Browser infra runs ci.*-presubmit builders, such as linux-presubmit,
with --all to ensure that the entire chromium/src passes
presubmit checks.
crrev.com/c/6842238 changed the finding type for License Check
from warning to error, but the CI presubmit builders failed because
there are many files without valid CopyRight.
Not only the existing files, all the new files that are added with
`Bypass-Check-License: <reason> footer could also cause the presubmit
builder to fail.
* This CL
In addition to the original patch from crrev.com/c/6842238,
this CL makes additional patches to turn the CopyRight errors into
warnings, if --file or --all is given.
The same approach is used in
https://source.chromium.org/chromium/chromium/tools/depot_tools/+/main:presubmit_canned_checks.py;l=982-987;drc=eb60ab38deeda6975c9b0fef883978f2a9f69120
Bug: 435696543,40237859
Original change's description:
> Revert "presubmit: emit errors instead of warnings for bad copyright headers"
>
> This reverts commit fa62515ecb.
>
> Reason for revert: it seems that the existing files without valid copyright headers are causing linux-presubmit builds to fail. b/438791294
>
> Bug: 435696543,40237859
> Original change's description:
> > presubmit: emit errors instead of warnings for bad copyright headers
> >
> > This is an effective revert of https://crrev.com/c/4895337 with
> > additional patches to support a footer.
> >
> > https://crrev.com/c/3887721 updated CheckLicense() to emit errors
> > for bad copyright headers. However, https://crrev.com/c/4895337
> > was changed the finding type from error to warning, claiming that
> > the check is N/A for moved third files, but it's not so easy
> > to programatiically distinguish moved third-party files.
> >
> > After discussions, it was decided to change the finding type back
> > to error to prevent accidental submissions for new files without
> > correct CopyRight headers.
> >
> > To mitigate moved, third-party files, this CL adds support for
> > "Bypass-Check-License: <reason>" footer.
> >
> > If the check should be ignored in a given CL, CL authors should
> > use the footer instead.
> >
> > Bug: 435696543,40237859
> > Change-Id: I177915c65932a3d76ea60ee6a0e396f726bc400d
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6842238
> > Reviewed-by: Gavin Mak <gavinmak@google.com>
> > Commit-Queue: Scott Lee <ddoman@chromium.org>
>
> Bug: 435696543,40237859
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Change-Id: Ibedf8d13e3742249947e29e625a14cceaf89879c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6852377
> Commit-Queue: Scott Lee <ddoman@chromium.org>
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Reviewed-by: Gavin Mak <gavinmak@google.com>
Bug: 435696543,40237859
Change-Id: Iafdb29b928c016eb3949e29fd43a2ba5f53e0ba0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6852108
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Scott Lee <ddoman@chromium.org>
This reverts commit fa62515ecb.
Reason for revert: it seems that the existing files without valid copyright headers are causing linux-presubmit builds to fail. b/438791294
Bug: 435696543,40237859
Original change's description:
> presubmit: emit errors instead of warnings for bad copyright headers
>
> This is an effective revert of https://crrev.com/c/4895337 with
> additional patches to support a footer.
>
> https://crrev.com/c/3887721 updated CheckLicense() to emit errors
> for bad copyright headers. However, https://crrev.com/c/4895337
> was changed the finding type from error to warning, claiming that
> the check is N/A for moved third files, but it's not so easy
> to programatiically distinguish moved third-party files.
>
> After discussions, it was decided to change the finding type back
> to error to prevent accidental submissions for new files without
> correct CopyRight headers.
>
> To mitigate moved, third-party files, this CL adds support for
> "Bypass-Check-License: <reason>" footer.
>
> If the check should be ignored in a given CL, CL authors should
> use the footer instead.
>
> Bug: 435696543,40237859
> Change-Id: I177915c65932a3d76ea60ee6a0e396f726bc400d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6842238
> Reviewed-by: Gavin Mak <gavinmak@google.com>
> Commit-Queue: Scott Lee <ddoman@chromium.org>
Bug: 435696543,40237859
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Change-Id: Ibedf8d13e3742249947e29e625a14cceaf89879c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6852377
Commit-Queue: Scott Lee <ddoman@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Gavin Mak <gavinmak@google.com>
This is an effective revert of https://crrev.com/c/4895337 with
additional patches to support a footer.
https://crrev.com/c/3887721 updated CheckLicense() to emit errors
for bad copyright headers. However, https://crrev.com/c/4895337
was changed the finding type from error to warning, claiming that
the check is N/A for moved third files, but it's not so easy
to programatiically distinguish moved third-party files.
After discussions, it was decided to change the finding type back
to error to prevent accidental submissions for new files without
correct CopyRight headers.
To mitigate moved, third-party files, this CL adds support for
"Bypass-Check-License: <reason>" footer.
If the check should be ignored in a given CL, CL authors should
use the footer instead.
Bug: 435696543,40237859
Change-Id: I177915c65932a3d76ea60ee6a0e396f726bc400d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6842238
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Scott Lee <ddoman@chromium.org>
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>
Reciprocal licenses can only be used in open source projects.
This change updates the presubmit validation checks to accept an
optional flag `allow_reciprocal_licenses`. When True, the allowlist is
extended to include reciprocal licenses.
Bug: 385020146
Change-Id: I0374658207bc87ffd74e033762ee4973c6e83b3b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6107863
Reviewed-by: Jordan Brown <rop@google.com>
Auto-Submit: Jordan Brown <rop@google.com>
Reviewed-by: Rachael Newitt <renewitt@google.com>
Commit-Queue: Rachael Newitt <renewitt@google.com>
