classification
Title: allow shutil.disk_usage to take a file path on Windows also
Type: enhancement Stage: resolved
Components: Library (Lib), Windows Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eryksun, giampaolo.rodola, jopamer, miss-islington, paul.moore, steve.dower, terry.reedy, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2018-01-15 15:26 by eryksun, last changed 2018-09-25 15:25 by steve.dower. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 9372 merged python-dev, 2018-09-17 21:32
Messages (10)
msg309992 - (view) Author: Eryk Sun (eryksun) * (Python triager) 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) * (Python committer) Date: 2018-01-19 20:04
Seems sensible to me.
msg319371 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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 changeset c8c0249c9e8f61ab7670119a5a5278354df27bbb 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
History
Date User Action Args
2018-09-25 15:25:35steve.dowersetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-09-25 14:57:42miss-islingtonsetnosy: + miss-islington
messages: + msg326359
2018-09-20 16:58:28jopamersetmessages: + msg325906
2018-09-19 23:12:36steve.dowersetmessages: + msg325818
2018-09-19 19:40:21jopamersetmessages: + msg325787
2018-09-18 14:48:56jopamersetmessages: + msg325655
2018-09-18 04:13:20steve.dowersetmessages: + msg325613
2018-09-17 21:45:24jopamersetnosy: + jopamer
messages: + msg325585
2018-09-17 21:32:10python-devsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request8795
2018-06-12 09:39:41giampaolo.rodolasetnosy: + giampaolo.rodola
messages: + msg319371
2018-01-19 20:04:41terry.reedysetnosy: + 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:52eryksuncreate