Skip to content

gh-92550 - Fix regression in pathlib.Path.rglob() #92583

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
merged 1 commit into from
May 10, 2022

Conversation

barneygale
Copy link
Contributor

@barneygale barneygale commented May 9, 2022

We could try to remedy this by taking a slice, but we then run into an issue where the empty string will match altsep on POSIX. That rabbit hole could keep getting deeper.

A proper fix for the original issue involves making pathlib's path normalisation more configurable - in this case we want to retain trailing slashes, but in other we might want to preserve ./ prefixes, or elide ../ segments when we're sure we won't encounter symlinks.

This reverts commit ea2f5bc.

Automerge-Triggered-By: GH:brettcannon

We could try to remedy this by taking a slice, but we then run
into an issue where the empty string will match altsep on POSIX.
That rabbit hole could keep getting deeper.

A proper fix involves making pathlib's path normalisation more
configurable - in this case we want to retain trailing slashes,
but in other we might want to preserve `./` prefixes, or elide
`../` entries when we're sure we won't encounter symlinks.

This reverts commit ea2f5bc.
@barneygale barneygale requested a review from brettcannon as a code owner May 9, 2022 22:37
@AlexWaygood AlexWaygood added type-bug An unexpected behavior, bug, or error stdlib Python modules in the Lib dir needs backport to 3.11 only security fixes labels May 9, 2022
@miss-islington
Copy link
Contributor

Thanks @barneygale for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 10, 2022
…2583)

We could try to remedy this by taking a slice, but we then run into an issue where the empty string will match altsep on POSIX. That rabbit hole could keep getting deeper.

A proper fix for the original issue involves making pathlib's path normalisation more configurable - in this case we want to retain trailing slashes, but in other we might want to preserve `./` prefixes, or elide `../` segments when we're sure we won't encounter symlinks.

This reverts commit ea2f5bc.
(cherry picked from commit dcdf250)

Co-authored-by: Barney Gale <barney.gale@gmail.com>
@bedevere-bot
Copy link

GH-92589 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label May 10, 2022
miss-islington added a commit that referenced this pull request May 10, 2022
We could try to remedy this by taking a slice, but we then run into an issue where the empty string will match altsep on POSIX. That rabbit hole could keep getting deeper.

A proper fix for the original issue involves making pathlib's path normalisation more configurable - in this case we want to retain trailing slashes, but in other we might want to preserve `./` prefixes, or elide `../` segments when we're sure we won't encounter symlinks.

This reverts commit ea2f5bc.
(cherry picked from commit dcdf250)

Co-authored-by: Barney Gale <barney.gale@gmail.com>
@serhiy-storchaka
Copy link
Member

Wait, it is not a fix, it is a removing of the feature!

@barneygale
Copy link
Contributor Author

The rglob() bug is a brand new regression, whereas glob() ignoring trailing slashes is longstanding bug. We're not under any particular time pressure to fix the original issue, so I'd rather we spent more time getting it right.

@serhiy-storchaka
Copy link
Member

The fix is trivial. #92604

@barneygale
Copy link
Contributor Author

The original fix is a bit of a hack because parse_parts() doesn't do what we want, which makes it riskier. It sounds like you're reasonably confident no other regressions will come up? If so your latest fix is fine by me.

serhiy-storchaka added a commit that referenced this pull request May 11, 2022
serhiy-storchaka added a commit that referenced this pull request May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants