Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2471)

#25596: regular files handled as directories in the glob module

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 11 months ago by xdegaye
Modified:
1 year, 8 months ago
Reviewers:
shadowranger+python, storchaka
CC:
gvanrossum, haypo, benhoyt, xdegaye, devnull_psf.upfronthosting.co.za, dimaqq, storchaka
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Patch Set 5 #

Patch Set 6 #

Patch Set 7 #

Patch Set 8 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/glob.rst View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
Doc/whatsnew/3.6.rst View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
Lib/glob.py View 1 2 3 4 5 6 7 3 chunks +41 lines, -29 lines 4 comments Download

Messages

Total messages: 2
josh.rosenberg
A couple comments on listifying more aggressively than needed. https://bugs.python.org/review/25596/diff/16562/Lib/glob.py File Lib/glob.py (right): https://bugs.python.org/review/25596/diff/16562/Lib/glob.py#newcode80 Lib/glob.py:80: ...
1 year, 8 months ago #1
storchaka_gmail.com
1 year, 8 months ago #2
https://bugs.python.org/review/25596/diff/16562/Lib/glob.py
File Lib/glob.py (right):

https://bugs.python.org/review/25596/diff/16562/Lib/glob.py#newcode80
Lib/glob.py:80: names = list(_iterdir(dirname, dironly))
On 2016/02/11 19:22:52, josh.rosenberg wrote:
> Definitely don't wrap in list here. fnmatch will create a list of all passing
> results before we return anyway, so we're not risking too many open directory
> handles thanks to recursive traversal or anything by list-ifying, we're just
> making unfiltered lists for no reason when we could wait and filter just
before
> returning.

Agree. It would be better to make fnmatch.filter() return an iterator in 3.0,
but this ship has sailed.

https://bugs.python.org/review/25596/diff/16562/Lib/glob.py#newcode133
Lib/glob.py:133: names = list(_iterdir(dirname, dironly))
On 2016/02/11 19:22:52, josh.rosenberg wrote:
> Why wrap in list? This means storing complete listings of (possibly) huge
> directories when we're going to process them iteratively and could pull them
> lazily. Is using tree-depth directories handles at once more of a problem than
> delaying processing of any outputs until we've read the whole contents of
every
> parent directory and stored them all in memory?

This is no worse than in listdir-based implementation. But using many FDs would
make it worse in certain respects and could break the code that worked with old
implementation.

We can achieve even more gain if use dir_fd parameter in stat(). But this needs
to use multiple FDs and can't be default implementation.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7