-
Notifications
You must be signed in to change notification settings - Fork 16
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
Recursively delete empty parent folders #94
Conversation
I should put in a check to not delete parents of the toplevel cloud store folder. |
localfs/store.go
Outdated
|
||
for dirName := path.Dir(filePath); len(dirName) > 0; dirName = path.Dir(dirName) { | ||
dir, err := os.Open(dirName) | ||
if os.IsNotExist(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block has race conditions between checking existence and emptiness, which is unnecessary as os.Remove()
errors on non-empty dirs.
Just try to os.Remove()
every parent dir, ignoring the dir already not existing or not being empty (ceasing looping if you hit a non-empty error)
localfs/emptydir_test.go
Outdated
testDir, err := os.MkdirTemp("/tmp", "dirtest") | ||
require.NoError(t, err) | ||
defer os.RemoveAll(testDir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[NIT] t.TempDir()
does all this for us IIRC:
testDir, err := os.MkdirTemp("/tmp", "dirtest") | |
require.NoError(t, err) | |
defer os.RemoveAll(testDir) | |
testDir := t.TempDir() |
err := os.MkdirAll(dir, 0755) | ||
require.NoError(t, err) | ||
err = os.WriteFile(fullPath, []byte("don't delete this folder"), 0755) | ||
require.NoError(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're using subtests below I believe you need to accept t testing.TB
as an argument, otherwise you'll reference the wrong *testing.T
.
localfs/store.go
Outdated
if pathErr, ok := err.(*fs.PathError); ok { | ||
if sysErr, ok := pathErr.Err.(syscall.Errno); ok && sysErr == syscall.ENOTEMPTY { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[NIT] errors.As()
is preferred here AFAIK: https://pkg.go.dev/errors
No description provided.