Skip to content

Revert "Fix #9584 - Using dirEntries and chdir() can have unwanted results (#10666)" #10718

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
Mar 27, 2025

Conversation

0xEAB
Copy link
Member

@0xEAB 0xEAB commented Mar 27, 2025

This essentially reverts commit 274109b.
It doesn’t remove the added unittest yet because it wonderfully underlines the point I’m going to make:

It adds no value.
The patch was a nerfed version of #6125. Well, it was nerfed to provide backwards compatibility. But this came at a cost: It no longer helped with the footgun imposed by relative paths that the original version did actually help with.

Let’s verify that Phobos still passes its testsuite and remove it for better (or worse).

Further thoughts

In addition it still failed to address the underlying problem that DirEntry-s that hold relative paths aren’t really aware of which directory they are relative to. That’s what needs to be fixed to resolve the UX issue at hand. I’ve spent quite a while hacking on the module and debugging my and existing code with my attempts at #10669 and #10706. The insight I’ve gained from this made it clear that any improvement implemented in this regard needs to be a thoroughly engineered one that doesn’t try to cut corners. I’ve also come to the conclusion that we still lack a proper unittest that doesn’t make premature assumptions; in particular one thing that prominent examples in the original issue report (and my later patches) got wrong is the expectation that we could just request the absolute path of a DirEntry and get a accurate result independent of the current working directory. (I’m actually a bit surprised now that I could get ae555fb to pass CI tests.) The way std.file currently works, doesn’t provide any guarantees that could come close to fulfilling such a requirement yet. If we want to make DirEntry work with no dependency on the current working directory, we’ll need to overhaul the whole module accordingly. Otherwise users are still in for surprises like the one in #9584. Fixing dirEntries alone won’t provide a satisfactory result.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @0xEAB!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#10718"

@0xEAB
Copy link
Member Author

0xEAB commented Mar 27, 2025

Technically, the removal should also fix a potential regression in a certain edge case as outlined by CyberShadow in #9584 (comment).
If for nothing else, let’s revert it for my peace of mind on this matter :)

@thewilsonator
Copy link
Contributor

Should #9584 be reopened or remain closed?

@thewilsonator thewilsonator merged commit 89f5914 into dlang:master Mar 27, 2025
10 checks passed
@0xEAB
Copy link
Member Author

0xEAB commented Mar 27, 2025

Should #9584 be reopened or remain closed?

I see no reason to close it in the context of this PR.
(Obviously GitHub does so because the commit message mentions “Fix <issue no.>”. I’ll reopen it.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants