Fix per-file owners check for deleted files.

Fix per-file owners check for deleted files.

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.

Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/944b60530e30ff97179b273360c8d3fb4f8cea7a

[dtu, 2016-07-14 00:59:44]

Description was changed from

==========
Fix per-file owners check for deleted files.

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 in CL.
==========

to

==========
Fix per-file owners check for deleted files.

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

[dtu, 2016-07-14 00:59:44]

dtu@chromium.org changed reviewers:
+ dpranke@chromium.org, phajdan.jr@chromium.org

[dtu, 2016-07-14 01:06:51]

https://codereview.chromium.org/2148153002/diff/1/owners_finder.py
File owners_finder.py (left):

https://codereview.chromium.org/2148153002/diff/1/owners_finder.py#oldcode47
owners_finder.py:47: if author:
This seemed like an abstraction layer violation. I've made `owned_by` and
`owners_for` protected.

https://codereview.chromium.org/2148153002/diff/1/tests/owners_unittest.py
File tests/owners_unittest.py (left):

https://codereview.chromium.org/2148153002/diff/1/tests/owners_unittest.py#ol...
tests/owners_unittest.py:153: # brett isn't allowed to approve ugly.cc
This comment didn't match the code. The OWNERS file does allow brett to approve
ugly.cc.

[Dirk Pranke, 2016-07-14 19:52:23]

lgtm.

There are some somewhat-subtle changes in the logic here, and it's been a long
time since I wrote this code, but I stared at it for quite some time and I think
it's probably right. And, the tests should be pretty good, so hopefully we're
okay.

Thanks for working on this!

https://codereview.chromium.org/2148153002/diff/1/tests/owners_unittest.py
File tests/owners_unittest.py (left):

https://codereview.chromium.org/2148153002/diff/1/tests/owners_unittest.py#ol...
tests/owners_unittest.py:153: # brett isn't allowed to approve ugly.cc
On 2016/07/14 01:06:51, dtu wrote:
> This comment didn't match the code. The OWNERS file does allow brett to
approve
> ugly.cc.

true, good catch.

[dtu, 2016-07-14 21:15:36]

On 2016/07/14 19:52:23, Dirk Pranke wrote:
> lgtm.
> 
> There are some somewhat-subtle changes in the logic here, and it's been a long
> time since I wrote this code, but I stared at it for quite some time and I
think
> it's probably right. And, the tests should be pretty good, so hopefully we're
> okay.
> 
> Thanks for working on this!
> 

Agreed. Thankfully, the code is fairly well-factored, and there weren't too many
sites that used owned_by, owners_for, and stop_looking. Just checking all those
sites was mostly sufficient, and the tests helpfully caught some edge cases.

> https://codereview.chromium.org/2148153002/diff/1/tests/owners_unittest.py
> File tests/owners_unittest.py (left):
> 
>
https://codereview.chromium.org/2148153002/diff/1/tests/owners_unittest.py#ol...
> tests/owners_unittest.py:153: # brett isn't allowed to approve ugly.cc
> On 2016/07/14 01:06:51, dtu wrote:
> > This comment didn't match the code. The OWNERS file does allow brett to
> approve
> > ugly.cc.
> 
> true, good catch.

[dtu, 2016-07-14 21:15:41]

The CQ bit was checked by dtu@chromium.org

[commit-bot: I haz the power, 2016-07-14 21:15:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 21:18:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-14 21:18:20]

Try jobs failed on following builders:
  Depot Tools Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Depot%20Tools%20Presubm...)

[dtu, 2016-07-14 21:40:58]

The CQ bit was checked by dtu@chromium.org

[dtu, 2016-07-14 21:40:58]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/2148153002/#ps20001
(title: "Rebase!")

[commit-bot: I haz the power, 2016-07-14 21:41:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 21:48:24]

Description was changed from

==========
Fix per-file owners check for deleted files.

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

to

==========
Fix per-file owners check for deleted files.

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.

Committed:
https://chromium.googlesource.com/chromium/tools/depot_tools/+/944b60530e30ff...
==========

[commit-bot: I haz the power, 2016-07-14 21:48:25]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/tools/depot_tools/+/944b60530e30ff...

[ncarter (slow), 2016-08-30 21:46:19]

nick@chromium.org changed reviewers:
+ nick@chromium.org

[ncarter (slow), 2016-08-30 21:46:19]

This change may have caused a serious performance regression in owners checking.

If you patch in https://codereview.chromium.org/2248873002/ to a chromium
checkout, and run 'git cl presubmit --upload', the checking owners step takes
71971 milliseconds. If I revert this change in my depot_tools, it only takes 144
milliseconds.

load_data_needed_for() alone takes over 50 seconds; almost all of this time is
spent in the _owners_for() function.

This seems unnecessary. Can we revert this change?

[Dirk Pranke, 2016-08-31 04:47:54]

On 2016/08/30 21:46:19, ncarter wrote:
> This change may have caused a serious performance regression in owners
checking.
> 
> If you patch in https://codereview.chromium.org/2248873002/ to a chromium
> checkout, and run 'git cl presubmit --upload', the checking owners step takes
> 71971 milliseconds. If I revert this change in my depot_tools, it only takes
144
> milliseconds.
> 
> load_data_needed_for() alone takes over 50 seconds; almost all of this time is
> spent in the _owners_for() function.
> 
> This seems unnecessary. Can we revert this change?

I don't think we should revert this change. It fixes real issues, and this is
the first
complaint I've heard about performance. 

I do take the complaint serious, though, of course, and we should fix it ASAP.

It looks like you posted a potential fix, so I'll take a look at that and we can
either
land that if it works, or I can figure out a different fix.