Show a list of directories missing OWNER reviewers on upload, and show directories rather than file…

Show a list of directories missing OWNER reviewers on upload, and show directories rather than files missing OWNER approvals on commit.

BUG=117166
TEST=covered by presubmit unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125588

[Pam (message me for reviews), 2012-03-07 19:48:03]

Here's sample output:

-----

> gcl presubmit test
*** Presubmit checks for COMMIT would report: ***
...

** Presubmit ERRORS **
Missing LGTM from an OWNER for these directories:
    chrome\browser\profiles

-----
> gcl presubmit test --upload
*** Presubmit checks for UPLOAD would report: ***
...

Done processing D:\src\chrome\browser\profiles\profile.h
Done processing D:\src\chrome\browser\profiles\profile.cc

** Presubmit Messages **
Missing OWNER reviewers for these directories:
    chrome\browser\profiles

Presubmit checks passed.
-----

- Pam

[Dirk Pranke, 2012-03-07 21:51:47]

Hi Pam! Thanks for working on this. The idea looks basically good, once I
figured out what you were trying to do :). I think if you make the following
tweaks it'll be more-or-less fine.

http://codereview.chromium.org/9621012/diff/2001/owners.py
File owners.py (right):

http://codereview.chromium.org/9621012/diff/2001/owners.py#newcode89
owners.py:89: def files_not_covered_by(self, files, reviewers,
directories=False):
With your change in presubmit_canned_checks, I think we don't actually need
files_not_covered_by, do we? I also don't think files_are_covered_by() is called
by anyone at this point, so you can probably get rid of these two routines, and
just rename files_not_covered_by to directories_not_covered_by and only have the
directories=True path).

http://codereview.chromium.org/9621012/diff/2001/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

http://codereview.chromium.org/9621012/diff/2001/presubmit_canned_checks.py#n...
presubmit_canned_checks.py:766: return [output('Missing %s for these
directories:\n    %s' %
The wording in your messages confused me, because I wasn't sure why you were
telling me about directories at all. I think if you change the message to "for
files in these directories" instead of "for these directories" it will be less
confusing.

[Pam (message me for reviews), 2012-03-07 22:21:11]

Thanks! Please take another look.

- Pam

http://codereview.chromium.org/9621012/diff/2001/owners.py
File owners.py (right):

http://codereview.chromium.org/9621012/diff/2001/owners.py#newcode89
owners.py:89: def files_not_covered_by(self, files, reviewers,
directories=False):
Done. It slightly changes the special-cased behavior when there are no
reviewers, but the only place that could have been used was in
files_are_covered_by(), so I moved the no-reviewers check there.

[Dirk Pranke, 2012-03-08 07:19:48]

lgtm.

http://codereview.chromium.org/9621012/diff/5002/owners.py
File owners.py (right):

http://codereview.chromium.org/9621012/diff/5002/owners.py#newcode78
owners.py:78: def files_are_covered_by(self, files, reviewers):
I meant that I don't think anyone calls this routine at all, so you can just
delete it.

http://codereview.chromium.org/9621012/diff/5002/owners.py#newcode102
owners.py:102: files_by_dir = self._files_by_dir(files)
You no longer need the files, just the dirs, so you can do:

dirs = self._files_by_dir(files).keys()
covered_dirs = self._dirs_covered_by(reviewers)
uncovered_dirs = filter(lambda d: not self._is_dir_covered_by(d, covered_dirs,
dirs)
return set(self._enclosing_dir_with_owners(d) for d in uncovered_dirs)

(or something like it).

[Pam (message me for reviews), 2012-03-08 09:11:54]

http://codereview.chromium.org/9621012/diff/5002/owners.py
File owners.py (right):

http://codereview.chromium.org/9621012/diff/5002/owners.py#newcode102
owners.py:102: files_by_dir = self._files_by_dir(files)
Thanks, I should have seen that. No more coding at midnight for me.

I took out _files_by_dir(), too, since it's also no longer needed.

[commit-bot: I haz the power, 2012-03-08 09:12:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/9621012/9001

[commit-bot: I haz the power, 2012-03-08 09:18:19]

Change committed as 125588