git cl owners orders owners by score with alphabetization being the tie
breaker. This leads to some owners being suggested far more often than
others.
Adding a tiny amount of randomization to the scoring leads to an even
distribution of equally qualified reviewers. Less qualified reviewers
will still be sorted into distinct buckets - the randomness is too small
to do anything except break ties.
The tests were updated so that they can tolerate the randomness, but
only for breaking ties.
Bug: 1024083
Change-Id: If7d39d1b3bbd980b80b46ab3f62c65215309bdc8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1913642
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Anthony Polito <apolito@google.com>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
This ensures relative file: directives in OWNERS files also work if
the directory of the owners file gets deleted.
Bug: 1015444
Change-Id: I9471a28a7246513120dd3ebb924f6d64eb50c2df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1870249
Reviewed-by: Tamer Tas <tmrts@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@google.com>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Ran:
vi $(git grep --name-only iteritems | grep -v third_party)
vi $(git grep --name-only itervalues | grep -v third_party)
vi $(git grep --name-only 'print ' | grep -v third_party)
and edited the files quickly with adhoc macros. Then ran in recipes/:
./recipes.py test train
There was only a small subset of files that had been updated to use
six.iteritems() and six.itervalues(). Since the dataset size that is
being used in gclient is small (pretty much always below 200 items),
it's better to just switch to .items() right away and take the temporary
performance hit, so that we don't need to come back to rewrite the code.
Recipe-Nontrivial-Roll: build
Bug: 984182
Change-Id: I5faf11486b66b0d73c9098ab0f2ce1b15a45c53e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1854900
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Auto-Submit: Marc-Antoine Ruel <maruel@chromium.org>
This method allows retrieving the owners listed in a file, as well as those
transitively listed in files included by it. This change also adds a new test to
cover this method, which is just a wrapper for the existing
_read_just_the_owners() internal method.
Bug: None
Change-Id: Iee956f115d3846acf0ee0806451807b0aa96d2f8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1773904
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
The parsing logic for OWNERS files included via "file:" lines currently
rejects inline comments (e.g. "foo@example.com # for foo.cc"), while
the normal OWNERS parsing logic correctly ignores such inline comments.
This CL makes the inline case ignore inline comments too.
Bug: 995474
Change-Id: I6f30554daf0a5f63b81719dced44f59187707eaa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1769603
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Commit-Queue: John Budorick <jbudorick@chromium.org>
The final improvement of the patch, better comments, was never
included in the CL that got checked in. Here they are.
Bug: 920591
Change-Id: I2a5f7f67d4bbc5b40fdb0e7021795cd204d6f0c8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1583772
Auto-Submit: Daniel Bratell <bratell@opera.com>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Python (prior to 3.8) treats meaningless string escape sequences as if
they were a slash followed by the character. That is, '\w' == '\\w'.
Python 3.8 rejects this, and it's confusing. This change fixes seven of these
regex strings found in depot_tools (through a regex search, natch). Most of
the fixes don't actually change the value of the strings, and this was
manually verified:
>>> '(/c(/.*/\+)?)?/(\d+)(/(\d+)?/?)?$' == r'(/c(/.*/\+)?)?/(\d+)(/(\d+)?/?)?$'
True
>>> '#\s*OWNERS_STATUS\s+=\s+(.+)$' == r'#\s*OWNERS_STATUS\s+=\s+(.+)$'
True
>>> 'COM\d' == r'COM\d'
True
>>> '^\s+Change-Id:\s*(\S+)$' == r'^\s+Change-Id:\s*(\S+)$'
True
>>> 'ETag:\s+([a-z0-9]{32})' == r'ETag:\s+([a-z0-9]{32})'
True
Two exceptions were the regex expressions in filter_demo_output.py and scm.py.
These were turned into raw strings despite this changing the value of the
string passed to re. This works because re supports the \x, \d, \w, \t, and
other escape sequences needed to make this work.
TL;DR - use raw strings for regex to avoid melting your brain. If bulk changing
regex strings to raw watch out for double-slashes.
Bug: 958138
Change-Id: Ic45264cfc63e8bae9cfcffe2cd88a57c2d3dcdae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1590534
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
The current owners systems scales badly on the number of files in a
patch, on the number of OWNERS files and on the number of "set noparent"
rules. This patch addresses all three of those.
If you have a patch with many (thousands) of files, you run into a lot
of repeated work because the parent directories are visited for every
file. By using a cache or remembering what directories have been
visited, a lot of work can be avoided.
If you have many OWNERS files or "set noparent" rules in scope of a
patch, owners and stop_looking lookups became slow. By splitting the
rules by longest glob-free directory path, you get much smaller lists
to process, which makes the code much faster.
(Test case with "all the files" went from >15 minutes to 30 seconds).
This saves about 2.5 second for git cl upload with a patch of
350 files spread over the tree.
The addition of many noparent rules in the Chromium tree might have
been the major reason this is needed, but there are also more files to
change, allowing larger patches.
Bug: 920591
Change-Id: If21178746ca9b88c2c07d265bd583b556d1734b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1404172
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Auto-Submit: Daniel Bratell <bratell@opera.com>
Commit-Queue: Daniel Bratell <bratell@opera.com>
The script gets confused on Windows, because the path delimieter is \,
not /.
Bug: 899724
Change-Id: Id907aceb4f7bb6fcc0460245890b9e4716054ff9
Reviewed-on: https://chromium-review.googlesource.com/c/1308897
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Files that control ownership have special ownership rules, to disallow
someone from self-approving a CL that adds themselves to a file that
controls ownership. owners.py now requires that they be named OWNERS or
end with a suffix of _OWNERS to ensure the special ownership rules are
correctly applied.
Bug: 801315
Change-Id: I083a2d15bdc9c749e3838fb1c983286d5a7c4af9
Reviewed-on: https://chromium-review.googlesource.com/1204917
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
A CL might delete an OWNERS file which we still need to read for the OWNERS
check.
BUG=778870
R=agable@chromium.org
Change-Id: I25636ff36228a1afb3c10edf5c2419773a4d057e
Reviewed-on: https://chromium-review.googlesource.com/754623
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Jochen Eisinger <jochen@chromium.org>
I've hit this assert a few times, seeing the output of the set usually
helps highlight quickly how my command-line arguments are broken.
Bug:
Change-Id: I8b80b635df91f2707a599a33ab9b4959b3c76614
Reviewed-on: https://chromium-review.googlesource.com/535834
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
If every file in a change has OWNERS, then the code would find
them fine. Similarly, if no files has OWNERS, then this code would
return an empty set just fine.
But if some files had OWNERS while others didn't, it would crash
when it tried to find an OWNER for file 'foo' while all the possible
OWNERS only provided coverage for file 'bar'.
This code purges the list of possible OWNERS as they become useless
for providing additional coverage, and returns whatever set we have
accumulated so far when the set of possible OWNERS becomes empty.
R=iannucci@chromium.org
Bug: 715062
Change-Id: I408601bd89379381db1cc7df56beed97ab3c27e6
Reviewed-on: https://chromium-review.googlesource.com/506239
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
A comment that is preceded with an empty line (or starts at the
beginning of the file) will be attributed to owners listed directly
below the comment. Otherwise, if the comment is in the middle of
a list of owners, it will only be attributed to the next owner.
BUG=712589
R=dpranke@chromium.org
Change-Id: I19bd7809836b6ee65ef56e2ec399e5cd09eaa132
Reviewed-on: https://chromium-review.googlesource.com/481303
Commit-Queue: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
This allows for having some global comments such as timezones or
long-term unavailability.
The comments go into build/OWNERS.status (that way, they should be
available in all repos that map in build/ for the gn config files).
The local can be overwritten in codereview.settings.
The format is
email: status
Comments (starting with #) are allowed in that file, but they're ignored.
BUG=694222
R=dpranke@chromium.org
Change-Id: I49f58be87497d1ccaaa74f0a2f3d373403be44e7
Reviewed-on: https://chromium-review.googlesource.com/459542
Commit-Queue: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Previously, if an OWNERS file included //foo/API_OWNERS, then the
code would get confused and think that it had already read and processed
//foo/OWNERS, transferring the contents of the former to the latter.
This was wrong. This change fixes the attribution and adds tests to make
sure we catch this in the future.
R=thakis@chromium.org
BUG=697156
Change-Id: I1f1b846cafac2ad6d792d2dccfce94911e9d15c3
Reviewed-on: https://chromium-review.googlesource.com/447962
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Dirk Pranke <dpranke@chromium.org>
This affects a bunch of files, but only changes comments,
and shouldn't make any difference to behavior.
The purpose is to slightly improve readability of pylint
disable comments.
Change-Id: Ic6cd0f8de792b31d91c6125f6da2616450b30f11
Reviewed-on: https://chromium-review.googlesource.com/420412
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
fnmatch.fnmatch seems to fall off a performance cliff once you start cycling through more patterns than can fit in its internal cache.
BUG=642793
Review-Url: https://codereview.chromium.org/2293233002
Previously if you deleted a file that you had per-file owners on, it would fail
the owners check. This fixes that.
Originally, owners.Database used glob to enumerate the directory and added all
the matching files in the directory to some dicts holding the owners
information. If a CL deleted a file, it'd no longer be on the filesystem, so it
wouldn't be in these dicts. There'd be no per-file owners information for it.
With this patch, the Database no longer enumerates individual files. It instead
keeps track of the glob patterns and checks the CL's files against the patterns
at lookup time.
BUG=622381
TEST=tests/owners_unittest.py && tests/owners_finder_test.py # Unit test included.
Review-Url: https://codereview.chromium.org/2148153002
If an OWNERS file used the file: directive with a relative file
path, but was using a root other than '/' (e.g.
'/path/to/my/real/root'), then the include resolver would incorrectly
leave a leading '/' on the include path. When os_path.join was then
called, the leading '/' meant the path was treated as an absolute path
and the join did not behave as expected.
Review-Url: https://codereview.chromium.org/2148683003
This CL implements support for file: include lines in OWNERS files,
both as top-level directives and as per-file directives. The
paths can be either relative or absolute.
Examples of lines in OWNERS files:
file:test/OWNERS (relative, top-level)
file://content/OWNERS (absolute, top-level)
per-file mock_impl.h=file:test/OWNERS (relative, per-file)
per-file mock_impl.h=file://content/OWNERS (absolute, per-file)
A whole series of tests to cover this feature have been added
to owners_unittest.py as well.
BUG=119396, 147633
Review URL: https://codereview.chromium.org/1085993004
git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@294854 0039d316-1c4b-4281-b951-d872f2087c98
Right now we require approval from someone, and we require an owner
approval, but we don't require an approval from an owner *other than
the patch other*. It's conceivable that we might want this, so
I am making this a configurable argument to the presubmit check.
This will also be needed to ensure that we don't suggest you as an
owner for your own patches, when we actually know who you are.
R=maruel@chromium.org
BUG=
Review URL: https://chromiumcodereview.appspot.com/12326151
git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@185294 0039d316-1c4b-4281-b951-d872f2087c98
Currently, when we run the OWNERS check, we print the list of directories
that contain the relevant OWNERS files for any modified files in a change
still needing approval.
This has two problems:
1) if we bubble all the way up to the top level OWNERS, we print "" instead of
"src/" or something more useful (bug 157191)
2) for OWNERS files that contain per-file set-noparent entries (like changes to IPC messages), this can be really confusing because an owner of other stuff in the directory might've approved things already.
This change will now print the list of files in the CL that are still unapproved.
This might be a lot more verbose (since you get N lines rather than 1 for N files in a given directory), but hopefully it'll be clearer in the two cases above.
Also, this change takes care of some lingering clean-up in the code to rename some methods to be clearer.
R=maruel@chromium.org
BUG=157191
Review URL: https://chromiumcodereview.appspot.com/12314044
git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@184219 0039d316-1c4b-4281-b951-d872f2087c98
previously we would return "*" as one of the suggested owners when
a CL included a file that anyone could approve. If the change had
other owners, the "*" was unnecessary, and if the change only included
wildcard-owned files, "*" isn't very helpful, so I've changed the text slightly.
R=maruel@chromium.org
BUG=169168
Review URL: https://chromiumcodereview.appspot.com/11867016
git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@177575 0039d316-1c4b-4281-b951-d872f2087c98
This version handles the case where we need to suggest two reviewers
who have overlapping sets of directories they can review.
R=maruel@chromium.org
BUG=76727
Review URL: https://chromiumcodereview.appspot.com/11639028
git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@174216 0039d316-1c4b-4281-b951-d872f2087c98
It turns out that we were weighting all possible owners equally,
and picking the last one out of the list. Given the way we traversed
owners files, and given that we got rid of the "set noparent"s, this
meant that we were always suggesting Ben for just about everything.
This change implements a much smarter algorithm that attempts to balance
number of reviewers and closeness to the files under review. The unit
tests added show specific examples and explanations for why things are
chosen the way they are.
R=maruel@chromium.org
BUG=76727
Review URL: https://chromiumcodereview.appspot.com/11567052
git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@173784 0039d316-1c4b-4281-b951-d872f2087c98
If you were creating a new OWNERS file that only had per-file owners
in it (and no catch-all owners for the whole directory), then we
would not look for suggested owners in parent directories, and end up
suggesting nothing. See https://chromiumcodereview.appspot.com/11555036/
for the CL that revealed this.
Also, the unit tests were incorrectly using absolute paths in some cases,
making the code less predictable; I've fixed the unit tests and added
a check for this into owners.py (real changes never used absolute paths,
just paths relative to the checkout root).
R=maruel@chromium.org
Review URL: https://chromiumcodereview.appspot.com/11569018
git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@173000 0039d316-1c4b-4281-b951-d872f2087c98
BUG=None
TEST=Change files that need OWNERS review. Upload the patch. Check that the warning suggests a minimum set of reviewers.
Review URL: https://chromiumcodereview.appspot.com/10222020
git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@135619 0039d316-1c4b-4281-b951-d872f2087c98
This change requires us to change the previous signature for the CheckOwners() hook to provide the server address and email regexp to use for parsing approvals from Rietveld.
Review URL: http://codereview.chromium.org/6657028
git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@77891 0039d316-1c4b-4281-b951-d872f2087c98
Bruce Dawson
Edward Lemur
Michael Achenbach
Marc-Antoine Ruel
Elly Fong-Jones
John Budorick
Daniel Bratell
Daniel Cheng
Jochen Eisinger
Gabriel Charette
Francois Doray
Aaron Gable
Dirk Pranke
Quinten Yearsley
nick
dtu
mbjorge
peter@chromium.org
ikarienator@chromium.org
dpranke@chromium.org
zork@chromium.org
bauerb@chromium.org
pam@chromium.org
maruel@chromium.org