Implement support for file: includes in OWNERS files.

Implement support for file: includes in OWNERS files.

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
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=294854

[Peter Beverloo, 2015-04-15 16:35:09]

peter@chromium.org changed reviewers:
+ brettw@chromium.org, dpranke@chromium.org

[Peter Beverloo, 2015-04-15 16:35:09]

+brettw and dpranke

WDYT? I'm sure there are rough edges, but this should be a fair start.

[Dirk Pranke, 2015-04-15 19:42:37]

Thanks for working on this! I think it'll make a number of people quite happy.

https://codereview.chromium.org/1085993004/diff/20001/owners.py
File owners.py (right):

https://codereview.chromium.org/1085993004/diff/20001/owners.py#newcode51
owners.py:51: "//" it is considered to be an absolute path, otherwise a relative
path.
"absolute path" might be confusing. I might call this "source-absolute", or
"relative to the top of the repo, otherwise it is relative to the current file".

https://codereview.chromium.org/1085993004/diff/20001/owners.py#newcode214
owners.py:214: return
Since read_files is a set, calling add() multiple times is harmless and
presumably cheap, so having this check is unnecessary and potentially confusing.

https://codereview.chromium.org/1085993004/diff/20001/owners.py#newcode303
owners.py:303: return dirpath, owners_path
see comment above; it's not clear to me that returning both dirpath and
owners_path is a great idea, and it's not clear what the relation between the
two values is.

[Peter Beverloo, 2015-04-15 20:04:31]

Thanks for the review!

I haven't yet applied the comment change since I left the office a while ago,
but will do so first thing tomorrow.

https://codereview.chromium.org/1085993004/diff/20001/owners.py
File owners.py (right):

https://codereview.chromium.org/1085993004/diff/20001/owners.py#newcode51
owners.py:51: "//" it is considered to be an absolute path, otherwise a relative
path.
On 2015/04/15 19:42:37, Dirk Pranke wrote:
> "absolute path" might be confusing. I might call this "source-absolute", or
> "relative to the top of the repo, otherwise it is relative to the current
file".

Ah yes, thanks. I'll adopt your comment with s/top/root/ to match terminology in
this class. (Database.root)

https://codereview.chromium.org/1085993004/diff/20001/owners.py#newcode214
owners.py:214: return
On 2015/04/15 19:42:37, Dirk Pranke wrote:
> Since read_files is a set, calling add() multiple times is harmless and
> presumably cheap, so having this check is unnecessary and potentially
confusing.

I may be misunderstanding you, but both the set and the check exist to avoid
infinite recursion. See the test_file_include_recursive_loop() test.

We can't safely move the check to _resolve_include() since two paths could need
to include the same file, for example (in a CL touching both directories):

//content/child/my_feature/OWNERS
  "file://content/browser/my_feature/OWNERS"
//content/renderer/my_feature/OWNERS
  "file://content/browser/my_feature/OWNERS"

https://codereview.chromium.org/1085993004/diff/20001/owners.py#newcode303
owners.py:303: return dirpath, owners_path
On 2015/04/15 19:42:37, Dirk Pranke wrote:
> see comment above; it's not clear to me that returning both dirpath and
> owners_path is a great idea, and it's not clear what the relation between the
> two values is.

I named them to be consistent with the arguments of _read_owners_in_dir(), which
is how we use them in _add_entry().

The reason for splitting this up is to be consistent with google3 syntax: when
using includes, the name of an OWNERS file doesn't have to be "OWNERS" anymore.
The following is perfectly valid:

  "file://content/owners/my_feature"

This is covered by the test_file_include_different_filename() test.

_read_owners_in_dir() has a number of different uses of |dirpath|, which is
scoped to the root of the repository. I don't know enough about depot
tools/OWNERS files to judge whether it'd be appropriate to update all these
uses.

(Whether this is a pattern to adopt by a project is a higher level question, I
guess.)

[Dirk Pranke, 2015-04-15 20:52:03]

this is what I get for not really staring at the patch, given that it's been 2+
years since I (or anyone) did real work on this file :).

https://codereview.chromium.org/1085993004/diff/20001/owners.py
File owners.py (right):

https://codereview.chromium.org/1085993004/diff/20001/owners.py#newcode214
owners.py:214: return
On 2015/04/15 20:04:31, Peter Beverloo wrote:
> On 2015/04/15 19:42:37, Dirk Pranke wrote:
> > Since read_files is a set, calling add() multiple times is harmless and
> > presumably cheap, so having this check is unnecessary and potentially
> confusing.
> 
> I may be misunderstanding you, but both the set and the check exist to avoid
> infinite recursion. See the test_file_include_recursive_loop() test.

Oh, I was misreading the code, and thought the function ended after line 216.
Never mind :).

https://codereview.chromium.org/1085993004/diff/20001/owners.py#newcode303
owners.py:303: return dirpath, owners_path
I'm not fully sure I understood your comments, but let me try two different
responses.

1) I agree that we will probably want to support owners files that are not named
'OWNERS',
so I don't think we're disagreeing or confused about this point.

2) I suggest that we rename _read_owners_in_dir(dirpath) to just
_read_owners(path), and 
stop passing in a dirpath (rather, join dirpath and OWNERS in
load_data_needed_for() and
other places as needed), and change change _resolve_include() to use 'path' and
'start' as
parameters and just return owners_path rather than both owners_path and dirpath.

Using the new parameter names would be consistent with os.path.relpath(), which
is
almost what you're emulating, and I think returning both dirpath and owners_file
is somewhat confusing, because it's not clear what the relationship between them
is.

I'm assuming the returned dirpath is equivalent to dirname(owners_path), but I'm
not
sure that that's true, because the way you're manipulating paths makes it a bit
hard
to follow. If that's not true, then we need comments or something to make it
clearer
how the two values are related.

Does that make more sense?

[Peter Beverloo, 2015-04-15 20:57:09]

https://codereview.chromium.org/1085993004/diff/20001/owners.py
File owners.py (right):

https://codereview.chromium.org/1085993004/diff/20001/owners.py#newcode303
owners.py:303: return dirpath, owners_path
On 2015/04/15 20:52:03, Dirk Pranke wrote:
> I'm not fully sure I understood your comments, but let me try two different
> responses.
> 
> 1) I agree that we will probably want to support owners files that are not
named
> 'OWNERS',
> so I don't think we're disagreeing or confused about this point.
> 
> 2) I suggest that we rename _read_owners_in_dir(dirpath) to just
> _read_owners(path), and 
> stop passing in a dirpath (rather, join dirpath and OWNERS in
> load_data_needed_for() and
> other places as needed), and change change _resolve_include() to use 'path'
and
> 'start' as
> parameters and just return owners_path rather than both owners_path and
dirpath.
> 
> Using the new parameter names would be consistent with os.path.relpath(),
which
> is
> almost what you're emulating, and I think returning both dirpath and
owners_file
> is somewhat confusing, because it's not clear what the relationship between
them
> is.
> 
> I'm assuming the returned dirpath is equivalent to dirname(owners_path), but
I'm
> not
> sure that that's true, because the way you're manipulating paths makes it a
bit
> hard
> to follow. If that's not true, then we need comments or something to make it
> clearer
> how the two values are related.
> 
> Does that make more sense?

Yes, it does. Thanks! I'll apply the changes tomorrow.

[Peter Beverloo, 2015-04-16 09:54:06]

https://codereview.chromium.org/1085993004/diff/20001/owners.py
File owners.py (right):

https://codereview.chromium.org/1085993004/diff/20001/owners.py#newcode51
owners.py:51: "//" it is considered to be an absolute path, otherwise a relative
path.
On 2015/04/15 20:04:31, Peter Beverloo wrote:
> On 2015/04/15 19:42:37, Dirk Pranke wrote:
> > "absolute path" might be confusing. I might call this "source-absolute", or
> > "relative to the top of the repo, otherwise it is relative to the current
> file".
> 
> Ah yes, thanks. I'll adopt your comment with s/top/root/ to match terminology
in
> this class. (Database.root)

Done.

https://codereview.chromium.org/1085993004/diff/20001/owners.py#newcode303
owners.py:303: return dirpath, owners_path
On 2015/04/15 20:57:09, Peter Beverloo wrote:
> On 2015/04/15 20:52:03, Dirk Pranke wrote:
> > I'm not fully sure I understood your comments, but let me try two different
> > responses.
> > 
> > 1) I agree that we will probably want to support owners files that are not
> named
> > 'OWNERS',
> > so I don't think we're disagreeing or confused about this point.
> > 
> > 2) I suggest that we rename _read_owners_in_dir(dirpath) to just
> > _read_owners(path), and 
> > stop passing in a dirpath (rather, join dirpath and OWNERS in
> > load_data_needed_for() and
> > other places as needed), and change change _resolve_include() to use 'path'
> and
> > 'start' as
> > parameters and just return owners_path rather than both owners_path and
> dirpath.
> > 
> > Using the new parameter names would be consistent with os.path.relpath(),
> which
> > is
> > almost what you're emulating, and I think returning both dirpath and
> owners_file
> > is somewhat confusing, because it's not clear what the relationship between
> them
> > is.
> > 
> > I'm assuming the returned dirpath is equivalent to dirname(owners_path), but
> I'm
> > not
> > sure that that's true, because the way you're manipulating paths makes it a
> bit
> > hard
> > to follow. If that's not true, then we need comments or something to make it
> > clearer
> > how the two values are related.
> > 
> > Does that make more sense?
> 
> Yes, it does. Thanks! I'll apply the changes tomorrow.

Done.

[Dirk Pranke, 2015-04-16 16:27:33]

lgtm, thanks!

[Peter Beverloo, 2015-04-16 16:35:42]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2015-04-16 16:38:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1085993004/60001

[commit-bot: I haz the power, 2015-04-16 16:42:10]

Committed patchset #4 (id:60001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=294854