Issue 11114005: implement per-file OWNERS support

Issue 11114005: implement per-file OWNERS support (Closed)

Created:
8 years, 2 months ago by Dirk Pranke

Modified:
8 years, 2 months ago

Reviewers:
M-A Ruel

CC:
chromium-reviews, Dirk Pranke, cmp+cc_chromium.org, M-A Ruel, jschuh, Tom Sepez

Base URL:
https://git.chromium.org/chromium/tools/depot_tools.git@master

Visibility:
Public.

Description

implement per-file OWNERS support R=maruel@chromium.org BUG=119394 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162529

Patch Set 2 : fix lint warning by making relpath a static method #

Patch Set 3 : change "owner" to "directive" in add_entry #

Total comments: 14

Patch Set 4 : add comments about how OWNERS files work, update w/ review feedback #

Total comments: 4

Patch Set 5 : update w/ more review feedback #

Created: 8 years, 2 months ago

	Unified diffs	Side-by-side diffs	Stats (+157 lines, -19 lines)
M	owners.py	View	7 chunks	+97 lines, -13 lines	0 comments
M	presubmit_support.py	View	2 chunks	+2 lines, -1 line	0 comments
M	testing_support/filesystem_mock.py	View	3 chunks	+10 lines, -0 lines	0 comments
M	tests/owners_unittest.py	View	5 chunks	+45 lines, -2 lines	0 comments
M	tests/presubmit_unittest.py	View	2 chunks	+3 lines, -3 lines	0 comments

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

M-A Ruel

https://codereview.chromium.org/11114005/diff/4001/owners.py File owners.py (right): https://codereview.chromium.org/11114005/diff/4001/owners.py#newcode5 owners.py:5: """A database of OWNERS files.""" One thing I'd really ...

8 years, 2 months ago (2012-10-12 15:19:04 UTC) #2

Dirk Pranke

Please take another look? Let me know if you want me to rename the functions ...

8 years, 2 months ago (2012-10-16 22:42:41 UTC) #3

Please take another look? Let me know if you want me to rename the functions now
(I would slightly prefer to do it separately but don't care that much).

https://codereview.chromium.org/11114005/diff/4001/owners.py
File owners.py (right):

https://codereview.chromium.org/11114005/diff/4001/owners.py#newcode5
owners.py:5: """A database of OWNERS files."""
On 2012/10/12 15:19:04, Marc-Antoine Ruel wrote:
> One thing I'd really like is a formal description of the DSL 
> (domain-specific-language) here.
> 
> Explain the various statements allowed in the file format and their exact
> effects.
> 
> Side-effect by the placement of directories as they are found in the checkout
up
> to |root|.

Okay, I'll write up something and see what you think.

https://codereview.chromium.org/11114005/diff/4001/owners.py#newcode39
owners.py:39: class Database(object):
On 2012/10/12 15:19:04, Marc-Antoine Ruel wrote:
> I think the code would be easier to read if it was split functionally into:
> - One Owners class presenting a parsed set of OWNERS files in a tree.
> - One class deciding based on Owners() instance if the list of files along the
> list of reviewers, should be accepted or not.
> 
> Technically, the second is a function*, here it's a member function, but I
think
> it'd be a bit simpler to read. Overall, it's a large refactor so I'm not
asking
> you to do it, just sayin'.
> 
> * But the "function" becomes more complex and stateful now that a glob is
> required.

I'll think about this, but definitely not do it in this patch.

https://codereview.chromium.org/11114005/diff/4001/owners.py#newcode52
owners.py:52: glob: function callback to list entries in a directory match a
glob (i.e., glob.glob)
On 2012/10/12 15:19:04, Marc-Antoine Ruel wrote:
> 80 cols

Done.

https://codereview.chromium.org/11114005/diff/4001/owners.py#newcode109
owners.py:109: objects_not_covered_by = directories_not_covered_by
On 2012/10/12 15:19:04, Marc-Antoine Ruel wrote:
> Why not rename right away? I don't see any advantage in doing it in two steps
> (?)

I thought it would make the patch easier to review.

https://codereview.chromium.org/11114005/diff/4001/owners.py#newcode181
owners.py:181: full_glob_string = self.os_path.join(self.root, dirpath,
glob_string)
On 2012/10/12 15:19:04, Marc-Antoine Ruel wrote:
> You should assert not self.os_path.isabs(glob_string)

Good point. Actually it's probably better to ensure that we don't have os.sep in
the string at all, to eliminate possible relpaths, too.

https://codereview.chromium.org/11114005/diff/4001/owners.py#newcode183
owners.py:183: for baseline in [self.os_path.relpath(self.root, b) for b in
baselines]:
On 2012/10/12 15:19:04, Marc-Antoine Ruel wrote:
> for baseline in (self.os_path.relpath(self.root, b) for b in baselines):

I tend to prefer the comprehension notation to the generator notation except
when inside function calls, but ok.

https://codereview.chromium.org/11114005/diff/4001/owners.py#newcode202
owners.py:202: ('%s is not a "set" directive, '
On 2012/10/12 15:19:04, Marc-Antoine Ruel wrote:
> Technically, you'd also want to say it's not an EVERYONE(*) directive either.

Done.

M-A Ruel

lgtm http://codereview.chromium.org/11114005/diff/6001/owners.py File owners.py (right): http://codereview.chromium.org/11114005/diff/6001/owners.py#newcode42 owners.py:42: apply (up until a "set noparent is encountered")> ...

8 years, 2 months ago (2012-10-17 20:07:07 UTC) #4

Dirk Pranke

https://codereview.chromium.org/11114005/diff/6001/owners.py File owners.py (right): https://codereview.chromium.org/11114005/diff/6001/owners.py#newcode42 owners.py:42: apply (up until a "set noparent is encountered")> On ...

8 years, 2 months ago (2012-10-17 21:08:36 UTC) #5

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dpranke@chromium.org/11114005/12001

8 years, 2 months ago (2012-10-17 21:09:08 UTC) #6

Change committed as 162529

Expand Messages | Collapse Messages