➜
This issue trackerhas been migrated toGitHub, and is currentlyread-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.
Created on2018-01-15 15:26 byeryksun, last changed2022-04-11 14:58 byadmin. This issue is nowclosed.
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 9372 | merged | python-dev,2018-09-17 21:32 | |
| Messages (11) | |||
|---|---|---|---|
| msg309992 -(view) | Author: Eryk Sun (eryksun)* | Date: 2018-01-15 15:26 | |
Issue 26330 was resolved by documenting that shutil.disk_usage requires a directory. However, the shutil module is in a position to harmonize cross-platform behavior in ways that aren't normally possible or recommended in the low-level os module. To that end, the Windows implementation could retry calling nt._getdiskusage on the resolved parent directory in case of NotADirectoryError. For example: def disk_usage(path): try: total, free = nt._getdiskusage(path) except NotADirectoryError: path = os.path.dirname(nt._getfinalpathname(path)) total, free = nt._getdiskusage(path) used = total - free return _ntuple_diskusage(total, used, free)Alternatively, this could be addressed in the implementation of nt._getdiskusage itself. | |||
| msg310286 -(view) | Author: Terry J. Reedy (terry.reedy)* | Date: 2018-01-19 20:04 | |
Seems sensible to me. | |||
| msg319371 -(view) | Author: Giampaolo Rodola' (giampaolo.rodola)* | Date: 2018-06-12 09:39 | |
+1 | |||
| msg325585 -(view) | Author: Joe Pamer (jopamer)* | Date: 2018-09-17 21:45 | |
Hi!I decided to try fixing this one as a way to get acquainted with the code base. I went ahead and updated the backing NT C function, but please let me know if you'd prefer I update disk_usage as proposed.Thanks! | |||
| msg325613 -(view) | Author: Steve Dower (steve.dower)* | Date: 2018-09-18 04:13 | |
I took a quick look at the patch and the main issue I see is the use of the MAX_PATH constant. We encourage people to disable this limit where possible, so using dynamic allocation where we need to reduce the path will be a better way to avoid this function breaking.There should be other examples of this pattern in the file, but I don't think it'll matter here if we always copy the string. Better to avoid it, of course, but that complicates things. | |||
| msg325655 -(view) | Author: Joe Pamer (jopamer)* | Date: 2018-09-18 14:48 | |
Got it - thanks! That sounds good to me, so I'll take a look at how other functions are working around MAX_PATH and update the diff to also avoid the copy when possible. | |||
| msg325787 -(view) | Author: Joe Pamer (jopamer)* | Date: 2018-09-19 19:40 | |
Just to loop back, I updated the PR to avoid MAX_PATH and only allocate in the "not a directory" case. Thanks for getting back to me so quickly!One question, though, is that it *does* seem like MAX_PATH is still referenced in several places in posixmodule.c. Is the ultimate goal to remove those references as well? | |||
| msg325818 -(view) | Author: Steve Dower (steve.dower)* | Date: 2018-09-19 23:12 | |
(Excuse the GitHub syntax - was about to post it there, but it got long enough to belong here)Regarding the `_dirnameW` discussion, fixing `_dirname` would be ideal, but that is bloating out your PR quite a bit :)The "right" function to use there is [PathCchRemoveFileSpec](https://docs.microsoft.com/en-us/windows/desktop/api/pathcch/nf-pathcch-pathcchremovefilespec), which unfortunately does not exist on Windows 7.The fallback is [PathRemoveFileSpec](https://docs.microsoft.com/en-us/windows/desktop/api/shlwapi/nf-shlwapi-pathremovefilespeca) which only supports up to `MAX_PATH` (but may do something relatively sensible for longer paths, such as trimming out the last directory separator before that limit - I haven't tested it).Supporting both is tricky - there's a similar example in `PC/getpathp.c` for `PathCchCombineEx` that could be replicated for dirname. But I'd be okay with taking this PR without that fix and filing a new bug for `_dirnameW`. | |||
| msg325906 -(view) | Author: Joe Pamer (jopamer)* | Date: 2018-09-20 16:58 | |
Awesome - thanks, Steve - this is all super helpful! If you're cool with it I'd like to stick to using _dirnameW for now, and then follow up with another set of PRs for the fixes you've recommended. | |||
| msg326359 -(view) | Author: miss-islington (miss-islington) | Date: 2018-09-25 14:57 | |
New changesetc8c0249c9e8f61ab7670119a5a5278354df27bbb by Miss Islington (bot) (Joe Pamer) in branch 'master':bpo-32557: allow shutil.disk_usage to take a file path on Windows also (GH-9372)https://github.com/python/cpython/commit/c8c0249c9e8f61ab7670119a5a5278354df27bbb | |||
| msg331603 -(view) | Author: STINNER Victor (vstinner)* | Date: 2018-12-11 10:48 | |
The new test is unstable: seebpo-35458. | |||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:58:56 | admin | set | github: 76738 |
| 2018-12-11 10:48:12 | vstinner | set | nosy: +vstinner messages: +msg331603 |
| 2018-09-25 15:25:35 | steve.dower | set | status: open -> closed resolution: fixed stage: patch review -> resolved |
| 2018-09-25 14:57:42 | miss-islington | set | nosy: +miss-islington messages: +msg326359 |
| 2018-09-20 16:58:28 | jopamer | set | messages: +msg325906 |
| 2018-09-19 23:12:36 | steve.dower | set | messages: +msg325818 |
| 2018-09-19 19:40:21 | jopamer | set | messages: +msg325787 |
| 2018-09-18 14:48:56 | jopamer | set | messages: +msg325655 |
| 2018-09-18 04:13:20 | steve.dower | set | messages: +msg325613 |
| 2018-09-17 21:45:24 | jopamer | set | nosy: +jopamer messages: +msg325585 |
| 2018-09-17 21:32:10 | python-dev | set | keywords: +patch stage: needs patch -> patch review pull_requests: +pull_request8795 |
| 2018-06-12 09:39:41 | giampaolo.rodola | set | nosy: +giampaolo.rodola messages: +msg319371 |
| 2018-01-19 20:04:41 | terry.reedy | set | nosy: +terry.reedy messages: +msg310286 title: allow shutil.disk_usage to take a file path -> allow shutil.disk_usage to take a file path on Windows also |
| 2018-01-15 15:26:52 | eryksun | create | |