Look for PRESUBMIT.py beyond root if inherit-review-settings-ok is present.

Look for PRESUBMIT.py beyond root if inherit-review-settings-ok is present.

Patch contributed by Darin Petkov

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

[petkov, 2010-06-15 16:35:41]

Please take a look at this, and see if it's OK overall.

Two issues that I need to look into:

1. Unit-tests fail with unexpected calls for the inherit-review-settings-ok file
-- I'd need to look into fixing them.
2. I haven't really tested on Windows -- just Linux.

[petkov, 2010-06-15 16:40:45]

> 2. I haven't really tested on Windows -- just Linux.

Ok, just tried on Windows:

>>> print os.path.dirname(os.getcwd())
C:\
>>> print os.path.dirname(os.path.dirname(os.getcwd()))
C:\

So it should be OK too (not that we really care).

[petkov, 2010-06-15 16:56:12]

> 1. Unit-tests fail with unexpected calls for the inherit-review-settings-ok
file
> -- I'd need to look into fixing them.

I've updated/fixed one unit test. Please comment before I update the rest.
Thanks.

[M-A Ruel, 2010-06-15 16:58:04]

a few notes then I'll be fine with the patch.

http://codereview.chromium.org/2845004/diff/3001/4001
File presubmit_support.py (right):

http://codereview.chromium.org/2845004/diff/3001/4001#newcode753
presubmit_support.py:753: top_root = root
nit: While at it, do you mind rewriting the function a bit? The current way it's
not nice. Here's a better way IMHO. It think this would make more sense. As you
prefer. 

files = [os.path.normpath(os.path.join(root, f)- for f in files]
# List all the individual directories containing files.
directories = sorted(list(set([os.path.dirname(f) for f in files])))
if os.path.isfile(os.path.join(root, 'inherit-review-settings-ok')):
  root = None
results = []
for directory in directories:
  while directory and directory != root:
    p = os.path.join(directory, 'PRESUBMIT.py')
    if not p in results and os.path.isfile(p):
      results.append(p)
    parent_dir = os.path.dirname(directory)
    if parent_dir == directory:
      # We hit the root directory.
      break
    directory = parent_dir
return sorted(results)

http://codereview.chromium.org/2845004/diff/3001/4001#newcode1034
presubmit_support.py:1034: parser.add_option("--root", default='')
parser.add_option("--root", default=os.getcwd(), help='If
inherit-review-settings-ok is present in this directory, parent directories up
to the root file system directories will be searched for PRESUBMIT.py.')

Then you can remove lines 1038-1039. Otherwise, this new behavior is not
documented at all.

[Daniel Erat, 2010-06-15 17:10:33]

LGTM with Marc-Antoine's suggested changes

[petkov, 2010-06-15 17:58:23]

Please take a look (along with the unit test change from the previous patch). If
all looks OK, I'll update the rest of the unit tests and upload for final
review.

[M-A Ruel, 2010-06-15 18:01:29]

lgtm with unit test passing, hopefully I haven't borked them all lately.

http://codereview.chromium.org/2845004/diff/12001/13001
File presubmit_support.py (right):

http://codereview.chromium.org/2845004/diff/12001/13001#newcode766
presubmit_support.py:766: if directory == root:
you're right, the check has to be after adding the result.

[petkov, 2010-06-15 21:21:37]

PTAL

I've rewritten the function again to scan each candidate
directory only once, updated the unit tests (all pass),
and added a new test.

[Daniel Erat, 2010-06-15 21:57:31]

LGTM

http://codereview.chromium.org/2845004/diff/4002/2003
File tests/presubmit_unittest.py (right):

http://codereview.chromium.org/2845004/diff/4002/2003#newcode125
tests/presubmit_unittest.py:125: 'inherit-review-settings-ok')
make 'inherit-review-settings-ok' a global var, at least within this module, so
you don't need to repeat it over and over?

[petkov, 2010-06-15 22:23:16]

http://codereview.chromium.org/2845004/diff/4002/2003
File tests/presubmit_unittest.py (right):

http://codereview.chromium.org/2845004/diff/4002/2003#newcode125
tests/presubmit_unittest.py:125: 'inherit-review-settings-ok')
On 2010/06/15 21:57:31, Daniel Erat wrote:
> make 'inherit-review-settings-ok' a global var, at least within this module,
so
> you don't need to repeat it over and over?

Done, although, that's inconsistent with the 'PRESUBMIT.py' use throughout file
:-)

[petkov, 2010-06-15 22:34:23]

Marc-Antoine,

Could you please review when you get a chance and, if it all LGTY, land the CL
-- I don't think I have svn commit access.

Thanks.

[petkov, 2010-06-15 22:43:24]

http://codereview.chromium.org/2845004/diff/24001/25001
File presubmit_support.py (right):

http://codereview.chromium.org/2845004/diff/24001/25001#newcode9
presubmit_support.py:9: __version__ = '1.3.4'
Do we need to bump this?

[M-A Ruel, 2010-06-15 22:49:15]

lgtm

you have a read-only checkout right now so I can commit it for you.

http://codereview.chromium.org/2845004/diff/24001/25001
File presubmit_support.py (right):

http://codereview.chromium.org/2845004/diff/24001/25001#newcode9
presubmit_support.py:9: __version__ = '1.3.4'
On 2010/06/15 22:43:24, petkov wrote:
> Do we need to bump this?
> 

yes that would make sense, 1.3.5.

[petkov, 2010-06-15 22:51:31]

Please submit the CL for me (and let me and/or the team know when it's in).
Thanks!

http://codereview.chromium.org/2845004/diff/24001/25001
File presubmit_support.py (right):

http://codereview.chromium.org/2845004/diff/24001/25001#newcode9
presubmit_support.py:9: __version__ = '1.3.4'
On 2010/06/15 22:49:15, Marc-Antoine Ruel wrote:
> On 2010/06/15 22:43:24, petkov wrote:
> > Do we need to bump this?
> > 
> 
> yes that would make sense, 1.3.5.

Done.