[3.12] gh-102511: Fix `os.path.normpath()` for UNC paths on Windows. by nineteendo · Pull Request #119394 · python/cpython

Fixes ntpath.normpath('//?/UNC/server/share/..') and makes it easier to back port an upcoming pull request.
If you don't want to use the C implementation of os.path.splitroot() when available, I can leave it out.

I want to hold this for a bit until we're confident in the change in later versions. Virtually every time we touch this kind of code, we introduce new failures or vulnerabilities, so let's keep it restricted to prerelease versions for now.

I'd say after 3.13.0b3 is released we'll reconsider, but that's because I'm hopeful we'll get a decent amount of people trying out those betas. If it seems like nobody is using them, we'll push this further.

@zooba, why is this being considered for 3.12?

@erlend-aasland For the reasons nineteendo mentioned above:

Fixes ntpath.normpath('//?/UNC/server/share/..') and makes it easier to back port an upcoming pull request.

Performance optimisations are usually okay in bugfix releases as well, provided they aren't changing public APIs, but generally aren't deemed worth the churn on their own. In this case, keeping the implementation consistent pushes it over the edge, IMHO.

Performance optimisations are usually okay in bugfix releases [...]

We normally do not backport performance optimisations unless we're talking about performance degradations that classify as bugs.

Quoting the devguide, for visibility:

The only changes allowed to occur in a maintenance branch without debate are bug fixes, test improvements, and edits to the documentation. Also, a general rule for maintenance branches is that compatibility must not be broken at any point between sibling micro releases (3.5.1, 3.5.2, etc.). For both rules, only rare exceptions are accepted and must be discussed first.

I don't see the problem here. We are fixing a bug, while at the same time improving the performance of os.path.splitroot(). I specifically asked whether it's OK to use the C implementation of os.path.splitroot():

If you don't want to use the C implementation of os.path.splitroot() when available, I can leave it out.

A performance-optimisation for performation-optimisation's sake, I agree. This is also ensuring that the code remains the same between supported versions, which makes backporting easier.

If the concern is the description of the fix, we can change it for 3.12. It won't match the description of the equivalent code that's already in 3.13, but that doesn't bother me.

This needs unittests ensuring that the C and Python versions of the functions have identical behavior (in main, not just in this backport).

If you want to propose this for backporting, it helps to make the change easy to understand what changed from a simple GH diff view. To do that, please don't indent the existing Python splitroot() implementations. Keep those at the top level, just renaming them to _py_splitroot, and using the conditional import logic only to assign/redefine the appropriate function to the splitroot name within the new code. This is also needed for testing purposes so that both versions exist. Work all that out in main first.

The Issue and the change in main/3.13 do not mention anything about a bug being fixed in the Issue, PR description, or commit message.

What was the bug?

Please make sure the Issue explains what the bug was and new desired behavior is, and that the NEWS entry in main/3.13 is fixed to describe the bugfix before considering a backport.

What was the bug?

>>> ntpath.normpath('//?/UNC/server/share/../..')
'\\\\?\\UNC\\' # instead of '\\\\?\\UNC\\server\\share\\'

All commit authors signed the Contributor License Agreement.

Steve, can we re-consider now? I've waited a bit longer until 3.13 was released.

The 3.12 bug-fix-window is considerably smaller now, implying that the code-remains-the-same argument of #119394 (comment) is considerably weakened.

The 3.12 bug-fix-window is considerably smaller now, implying that the code-remains-the-same argument...

I'd rate this code as security-sensitive, meaning it's more likely to get fixes that we'd have to backport throughout all security releases. But that also suggests we shouldn't be changing it for an issue that already existed in case we create a new issue. Plus it means a backport would likely need customisation for older versions anyway. The one year window where it's definitely worth it (after 3.11 is EOL) isn't very long, and the bug wasn't reported specifically, but was discovered.

I'm leaning towards status quo (don't merge), just because we apparently haven't upset anyone yet (except perhaps nineteendo, sorry) and not changing things rarely makes it worse.

I'm leaning towards status quo (don't merge), just because we apparently haven't upset anyone yet (except perhaps nineteendo, sorry) and not changing things rarely makes it worse.

It's fine, I'm just happy I can finally delete this branch and forget about it.