Skip to content

bpo-39899: Make pathlib use os.path.expanduser() to expand home directories #18841

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

barneygale
Copy link
Contributor

@barneygale barneygale commented Mar 8, 2020

os.path.expanduser(): don't guess other Windows users' home directories if the basename of the current user's home directory doesn't match their username.

This makes ntpath.expanduser() match pathlib.Path.expanduser() in this regard, and is more in line with posixpath.expanduser()'s cautious approach.

Also remove the near-duplicate implementation of expanduser() in pathlib, and by doing so fix a bug where KeyError could be raised when expanding another user's home directory on Windows.

https://bugs.python.org/issue39899

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@barneygale

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@barneygale barneygale force-pushed the bpo-39899-pathlib-expanduser-reimpl branch 3 times, most recently from 0e82850 to b9b9801 Compare March 10, 2020 20:23
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no problem with this personally but I would like advice from our Windows experts, @zooba and @pfmoore, especially on the os.path.expanduser changes.

@barneygale barneygale force-pushed the bpo-39899-pathlib-expanduser-reimpl branch from b9b9801 to 2a49892 Compare January 21, 2021 23:52
@barneygale
Copy link
Contributor Author

Updated to use os.fsdecode() per review comments. Thanks for your help so far @pitrou.

@pfmoore @zooba flagging this with you again per pitrou's recommendation!

@zooba
Copy link
Member

zooba commented Apr 7, 2021

I think this one is okay, but I need to spend a bit more time reading through it (it's pretty late here now and I'm too tired).

@barneygale
Copy link
Contributor Author

Yep understood! If it helps, the rough process here is:

  1. Bring os.path.expanduser() in line with pathlib's home directory resolution. They were already almost identical, but pathlib was slightly stricter in the rare scenario:
  • We're on Windows, and
  • USERNAME is set, and
  • It doesn't match the last component of the current user's home directory, and
  • We're querying for another user's home directory
  1. Delete pathlib's implementation in the name of DRY.

@barneygale barneygale force-pushed the bpo-39899-pathlib-expanduser-reimpl branch from 2a49892 to d7074c2 Compare April 7, 2021 22:03
@barneygale
Copy link
Contributor Author

Rebased to fix conflicts

@zooba zooba merged commit 3f3d82b into python:master Apr 7, 2021
…e directories if the basename of the current user's home directory doesn't match their username.

This makes `ntpath.expanduser()` match `pathlib.Path.expanduser()` in this regard, and is more in line with `posixpath.expanduser()`'s cautious approach.

Also remove the near-duplicate implementation of `expanduser()` in pathlib, and by doing so fix a bug where KeyError could be raised when expanding another user's home directory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants