@barneygale or@encukou maybe on of you could have a quick look at this fix? Since you merged/reviewed a PR in this code area very recently.

Thank you!

According to thedevguide, I think this is not a trivial change like a typo fix, so we need to create an issue first if not exist and addgh-{issue number}: at the beginning of the PR title. The news entry file is also required.

According to thedevguide, I think this is not a trivial change like a typo fix, so we need to create an issue first if not exist and addgh-{issue number}: at the beginning of the PR title. The news entry file is also required.

Thanks! Created an issue and added entries toMisc/NEWS.d/ andMisc/ACKS

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

For future reference, please don't force-push to CPython PR branches -- it makes the changes a little harder to follow for reviewers, and every PR gets squashed anyway.

Looking good! Please could you add a test thatValueError is still raised whenstrict=True? Also mention that your change affects non-strict mode in the NEWS please.

Thank you, for your review!
I have made the requested changes; please review again

Thanks for making the requested changes!

@barneygale: please review the changes made to this pull request.

Looking good! Please could you add a test thatValueError is still raised whenstrict=True? Also mention that your change affects non-strict mode in the NEWS please.

In 3.11+ on Windows,realpath() raisesOSError for an embedded null in strict mode. This was changed by a PR for issue#106242. I was against changingnormpath() to raiseValueError for an embedded null, but therealpath() behavior was a separate case. I summarized therealpath() case inthis comment. On Windows, it had been mistakenly raisingValueError for an embedded null since 3.8. When thestrict parameter was added in 3.10, it should have been fixed to ignore theValueError in the default non-strict mode. Steve fixed it in 3.11, but for some reason he also decided to convert theValueError toOSError in strict mode.

ntpath source:

exceptValueErrorasex:# gh-106242: Raised for embedded null characters# In strict mode, we convert into an OSError.# Non-strict mode returns the path as-is, since we've already# made it absolute.ifstrict:raiseOSError(str(ex))fromNone

example:

>>> os.path.realpath('\0')'C:\\Temp\\\x00'>>> os.path.realpath('\0',strict=True)Traceback (most recent call last):  File "<stdin>", line 1, in <module>  File "<frozen ntpath>", line 704, in realpathOSError: _getfinalpathname: embedded null character in path

Steve fixed it in 3.11, but for some reason he also decided to convert theValueError toOSError in strict mode.

Good to know, I am not able to test with Windows.
Should I convert theValueError toOSError in strict mode to be consistent with the Windows implementation?

The missing tests you mentioned inyour comment are also added with this PR.

Should I convert theValueError toOSError in strict mode to be consistent with the Windows implementation?

That's up to@barneygale and@zooba.

IIRC, we changed toOSError because someone pointed out that we didn't specify thatValueError might be raised (and apparently I either didn't win the argument that "ValueError is always implied when you pass a bad value", or I couldn't be bothered arguing that day).

Strictly speaking, since it's never going to work without the caller changing the value (as opposed to the OS changing),ValueError is correct.

Steve, in#106242 I argued against changingnormpath() to raiseValueError. You opted to modify the C implementation of_path_normpath() instead, so the discussion aboutValueError became irrelevant.

The behavior ofrealpath() also came up. On Windows, it started raisingValueError for embedded null characters in 3.8, and similarly on POSIX in 3.10. Personally, I just wantedrealpath() to ignoreValueError in the default permissive mode.

Strictly speaking, since it's never going to work without the caller changing the value (as opposed to the OS changing),ValueError is correct.

Is that a vote for changing it back to raisingValueError in strict mode on Windows? Or has that ship sailed?

Ah, I was going for consistency with the earlier behaviour, which was the wrong behaviour as well by my definition above. (I do remember someone arguing that it wasn't documented though, so perhaps that was another issue? I don't think it was Eryk Sun.)

I guess that's a vote for changing the Windows implementation to not replace that particular error, but there's no particular urgency (i.e. not in this PR). And I think non-strict mode should be as garbage-in-garbage-out as we can make it, so suppressing the error in that case is fine as long as the final string includes the text after the null.

The following commit authors need to sign the Contributor License Agreement:

• 1478183+stefanhoelzl@users.noreply.github.com