bpo-38671: Make sure to return an absolute path in pathlib._WindowsFlavour.resolve() by uranusjr · Pull Request #17716

bpo-38671: Make sure to return an absolute path in pathlib._WindowsFlavour.resolve() by uranusjr · Pull Request #17716 · python/cpython

I’ll rebase and change the call to os.path.abspath().

Edit: Done. I ended up adding the os.path.abspath() call later in the function, since the non-absolute s is needed in early parts of the function. I also took the liberty to refactor the function slightly to avoid very deep indentations in the function.

OK, I’ve

Move the unit test under _BasePathTest so it is run for all platforms.
Remove the _getfinalpathname check since it should now be available everywhere. This also allows some additional cleanups that aren’t exactly related to the .resolve() fix.
Use _getfullpathname instead of abspath as suggested.

I split each of these changes into individual commits so they can be easily reverted if needed.

Is this just blocked on reviews?

Choose a reason for hiding this comment

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

Lines 189-191 make a bad assumption that the current working directory (CWD) is a final path. The CWD is an absolute path but not a final path. The assignment should be something like s = str(path) or '.'. In practice it may never be an issue since PurePath.__str__ assigns and returns self._str = self._format_parsed_parts(self._drv, self._root, self._parts) or '.'.

Choose a reason for hiding this comment

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

The CWD is an absolute path but not a final path.

TIL.

I like the or '.' approach since it avoids special-casing things, but want @pitrou to take a look since this is inherited from the initial pathlib implementation.

Is this a case of letting the best be the enemy of the good? This bug is a real time-waster and can easily slip into cross-platform code. If the issues @eryksun highlights are pre-existing then perhaps you should merge it and then fix those separately?

@prescod, I just created bpo-43455 for the os.getcwd() issue. It should not hold this issue up.

Is it too late for this to be in 3.10?

@ofek, this is a bug fix, not a new feature. Even if it were, new features can be added to 3.10 until the first beta release in May. Hopefully it gets merged into the 3.8 branch in time for the 3.8.9 release in May, since it's the last bug-fix release for 3.8.

This guarentees we are returning an absolute path when the input `path`
is relative. Nothing would change if `path` is already absolute.

Strong nudge.

Hopefully it gets merged into the 3.8 branch in time for the 3.8.9 release in May

This is our last chance.

I’ve rebased this multiple times and this time the new incoming changes is too much for me. I give up. Someone else can take over.

uranusjr deleted the path-resolve-nonstrict-win branch

March 3, 2022 10:04