implement per-file OWNERS support

implement per-file OWNERS support

R=maruel@chromium.org
BUG=119394
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162529

[M-A Ruel, 2012-10-12 15:19:04]

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 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|.

https://codereview.chromium.org/11114005/diff/4001/owners.py#newcode39
owners.py:39: class Database(object):
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.

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)
80 cols

https://codereview.chromium.org/11114005/diff/4001/owners.py#newcode109
owners.py:109: objects_not_covered_by = directories_not_covered_by
Why not rename right away? I don't see any advantage in doing it in two steps
(?)

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)
You should assert not self.os_path.isabs(glob_string)

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]:
for baseline in (self.os_path.relpath(self.root, b) for b in baselines):

https://codereview.chromium.org/11114005/diff/4001/owners.py#newcode202
owners.py:202: ('%s is not a "set" directive, '
Technically, you'd also want to say it's not an EVERYONE(*) directive either.

[Dirk Pranke, 2012-10-16 22:42:41]

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, 2012-10-17 20:07:07]

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")>
s/>/./ ?

http://codereview.chromium.org/11114005/diff/6001/testing_support/filesystem_...
File testing_support/filesystem_mock.py (right):

http://codereview.chromium.org/11114005/diff/6001/testing_support/filesystem_...
testing_support/filesystem_mock.py:84: return path.replace(base, '', 1)
Why not path[len(base):] ?

[Dirk Pranke, 2012-10-17 21:08:36]

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 2012/10/17 20:07:07, Marc-Antoine Ruel wrote:
> s/>/./ ?

Good catch. Done.

https://codereview.chromium.org/11114005/diff/6001/testing_support/filesystem...
File testing_support/filesystem_mock.py (right):

https://codereview.chromium.org/11114005/diff/6001/testing_support/filesystem...
testing_support/filesystem_mock.py:84: return path.replace(base, '', 1)
On 2012/10/17 20:07:07, Marc-Antoine Ruel wrote:
> Why not path[len(base):] ?

That is better. Done.

[commit-bot: I haz the power, 2012-10-17 21:09:08]

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

[commit-bot: I haz the power, 2012-10-17 21:12:13]

Change committed as 162529