Do not assume that all presubmit directories still exist.

Do not assume that all presubmit directories still exist.

In the case where a CL deletes the last file in the directory,
it is listed in the presubmit files list, but the directory to
which it refers will not exist, leading to os.listdir failing.

BUG=638343
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/ff061c0d1d2d30909e201487cc72c106335ac3c6

[Tobias Sargeant, 2016-08-17 09:35:38]

tobiasjs@chromium.org changed reviewers:
+ tandrii@chromium.org

[tandrii(chromium), 2016-08-17 09:39:13]

LGTM % suggestion

https://codereview.chromium.org/2250353003/diff/20001/presubmit_support.py
File presubmit_support.py (right):

https://codereview.chromium.org/2250353003/diff/20001/presubmit_support.py#ne...
presubmit_support.py:1099: directory_files = []
continue
it's shorter and faster.

[Tobias Sargeant, 2016-08-17 09:57:45]

https://codereview.chromium.org/2250353003/diff/20001/presubmit_support.py
File presubmit_support.py (right):

https://codereview.chromium.org/2250353003/diff/20001/presubmit_support.py#ne...
presubmit_support.py:1099: directory_files = []
On 2016/08/17 09:39:13, tandrii(chromium) wrote:
> continue
> it's shorter and faster.

Thanks. It seems like it's best to just wrap the whole loop, as in PS3. WDYT?

[tandrii(chromium), 2016-08-17 10:01:56]

https://codereview.chromium.org/2250353003/diff/40001/presubmit_support.py
File presubmit_support.py (right):

https://codereview.chromium.org/2250353003/diff/40001/presubmit_support.py#ne...
presubmit_support.py:1097: for f in os.listdir(directory):
the only thing that can raise here is this line, so I don't see a reason to wrap
everything in try-catch. In fact, you could do which is even better:

if not os.path.isdir(directory):
  continue
for f in os.listdir......
  ...

[Tobias Sargeant, 2016-08-17 10:15:35]

On 2016/08/17 10:01:56, tandrii(chromium) wrote:
> https://codereview.chromium.org/2250353003/diff/40001/presubmit_support.py
> File presubmit_support.py (right):
> 
>
https://codereview.chromium.org/2250353003/diff/40001/presubmit_support.py#ne...
> presubmit_support.py:1097: for f in os.listdir(directory):
> the only thing that can raise here is this line, so I don't see a reason to
wrap
> everything in try-catch. In fact, you could do which is even better:
> 
> if not os.path.isdir(directory):
>   continue
> for f in os.listdir......
>   ...

That latter would be ok, except I suspect that it'd mean changing the unittests
too, to add mock calls for isdir.

I realise that isfile doesn't throw, but wrapping the whole block avoids needing
to create another (named) local.

[tandrii(chromium), 2016-08-17 10:18:03]

LGTM you already have. this CL isn't worth the time we burn on discussions.

[tandrii(chromium), 2016-08-17 10:18:40]

On 2016/08/17 10:18:03, tandrii(chromium) wrote:
> LGTM you already have. this CL isn't worth the time we burn on discussions.
but your previous expansion to PRESUBMIT* is quite nice and I'm glad i have
learned about it.

[Tobias Sargeant, 2016-08-17 10:20:34]

The CQ bit was checked by tobiasjs@chromium.org

[commit-bot: I haz the power, 2016-08-17 10:20:43]

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-08-17 10:24:02]

Description was changed from

==========
Do not assume that all presubmit directories still exist.

In the case where a CL deletes the last file in the directory,
it is listed in the presubmit files list, but the directory to
which it refers will not exist, leading to os.listdir failing.

BUG=638343
==========

to

==========
Do not assume that all presubmit directories still exist.

In the case where a CL deletes the last file in the directory,
it is listed in the presubmit files list, but the directory to
which it refers will not exist, leading to os.listdir failing.

BUG=638343

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

[commit-bot: I haz the power, 2016-08-17 10:24:03]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/tools/depot_tools/+/ff061c0d1d2d30...