Skip to content
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

Fix broken file and dir helpers on Windows #163

Merged
merged 4 commits into from
Aug 19, 2024

Conversation

brandondyck
Copy link
Contributor

File and directory helpers don't work properly on Windows, and a couple of them even panic. This PR fixes a few underlying problems:

  1. os.DirFS does not handle Windows paths properly, and brokenfs isn't doing the trick to fix it. When those functions validate names, they choke on the colon after the drive letter and return fs.ErrInvalid. Perhaps this can be avoided by not providing a drive letter (that seems to be a solution given in os: hard to use DirFS for cross-platform root filesystem golang/go#44279) but that's inconvenient and would probably only work if the file/directory is on the same drive as the working directory. I fixed this by adding having the non-FS helpers use new assertions that call os functions instead of fs ones.

  2. Some file and directory assertions only check for ErrNotExist and then assume that Stat succeeded. In the best case, this misses an uncaught ErrInvalid and gives a misleading result; in the worst case, as in FileExistsFS and DirExistsFS, it leads to a panic from a nil dereference. I prevented the panics by checking for other errors as well before dereferencing the Stat result.

  3. There are no tests in the test suite that make sure an assertion wasn't triggered, and some of the tests that do exist get skipped on Windows. I updated the tests to check both major paths, and I made most of the tests Windows-compatible by using t.TempDir() instead of standard Unix paths. I didn't do this to the FileMode and DirMode tests because I don't know how Unix permissions work on Windows.

@shoenig
Copy link
Owner

shoenig commented Aug 19, 2024

Wow @brandondyck, this is fantastic work! I'll be the first to admit I don't use Windows much, so any attention it can get is greatly appreciated. This LGTM, just as soon as we get the linter passing (just an extra newline).

Copy link
Owner

@shoenig shoenig left a comment

Choose a reason for hiding this comment

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

LGTM; thanks @brandondyck!

@shoenig shoenig merged commit 5062c5a into shoenig:main Aug 19, 2024
6 checks passed
@brandondyck brandondyck deleted the fix-direxists branch August 19, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants