Fix checkdeps.py to check all the source directories in git checkouts.

Fix checkdeps.py to check all the source directories in git checkouts.

BUG=33165
TEST=Manual: add an invalid #include in ppapi/examples/ime/ime.cc and
verify it to be correctly warned.


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

[kinaba, 2011-10-27 21:49:42]

Hi, Satoru-san, can you review the change? Thanks.

[satorux1, 2011-10-27 22:03:48]

http://codereview.chromium.org/8416016/diff/1/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/8416016/diff/1/tools/checkdeps/checkdeps.py#ne...
tools/checkdeps/checkdeps.py:420: while dir_name not in git_source_directory:
if dir_name is initially "foo/bar/baz", we'll add "foo/bar" and "foo" if needed,
right?

Would it be possible that "foo/bar" is already in git_source_directory, but
"foo" is not? If it's possible, this logic won't be able to add "foo". I guess
it'd be safer to always add all parent directories like

  while not dir_name:
    git_source_directory.add(dir_name)
    dir_name = os.path.dirname(dir_name)

assuming dir_name will eventually be empty.

[kinaba, 2011-10-27 22:30:14]

Updated.

http://codereview.chromium.org/8416016/diff/1/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/8416016/diff/1/tools/checkdeps/checkdeps.py#ne...
tools/checkdeps/checkdeps.py:420: while dir_name not in git_source_directory:
On 2011/10/27 22:03:48, satorux1 wrote:
> if dir_name is initially "foo/bar/baz", we'll add "foo/bar" and "foo" if
needed,
> right?

Yes.

> Would it be possible that "foo/bar" is already in git_source_directory, but
> "foo" is not?

It is impossible, because all additions are done through this logic; if it has
added "foo/bar", then surely it should have tried "foo" next.

> it'd be safer to always add all parent directories like
> 
>   while not dir_name:
>     git_source_directory.add(dir_name)
>     dir_name = os.path.dirname(dir_name)
> 
> assuming dir_name will eventually be empty.

But, yes, I agree this is more safer and easier to understand. It is a bit
slower (around 300ms on my machine in addition to the 100msec slowdown of the
first patch compared to the ToT version), but I guess it is tolerable in the
total running time of the script (6-7 sec).

[satorux1, 2011-10-27 23:04:16]

LGTM

http://codereview.chromium.org/8416016/diff/3/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/8416016/diff/3/tools/checkdeps/checkdeps.py#ne...
tools/checkdeps/checkdeps.py:419: while dir_name != root:
Would be nice to add some comment as the logic is not so obvious, like

# Add the directory as well as all the parent directories.

[kinaba, 2011-10-27 23:07:20]

http://codereview.chromium.org/8416016/diff/3/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/8416016/diff/3/tools/checkdeps/checkdeps.py#ne...
tools/checkdeps/checkdeps.py:419: while dir_name != root:
On 2011/10/27 23:04:16, satorux1 wrote:
> Would be nice to add some comment as the logic is not so obvious, like
> 
> # Add the directory as well as all the parent directories.

Done.

[commit-bot: I haz the power, 2011-10-28 00:02:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinaba@chromium.org/8416016/5003

[commit-bot: I haz the power, 2011-10-28 01:05:07]

Change committed as 107668