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

implement remove and rename feature for fat32 #233

Merged
merged 8 commits into from
Nov 17, 2024

Conversation

ludelion5
Copy link
Contributor

Sorry for the late PR. Here is my proposal to rename and remove files in fat32.

closes #191

@ludelion5 ludelion5 changed the title feat: implement remove and rename feature for fat32, add new unit-tests implement remove and rename feature for fat32 Jul 3, 2024
Copy link
Collaborator

@deitch deitch left a comment

Choose a reason for hiding this comment

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

Overall, this looks great, and thank you.

In terms of naming, are these meant to be restricted just to files, or any structure (files, dirs, symlinks where supported, etc.)? I would think the latter, since CLI-level commands for the above in most OSes don't care. Same for linux mv / Windows ren.

So should these be RemoveFile() and RenameFile(), or just Remove() and Rename()?

On the other hand, in keeping with golang style, maybe these should parallel and be Remove() and RemoveAll() and [Rename()](https://pkg.go.dev/os#Rename]?

@ludelion5
Copy link
Contributor Author

Thanks for the feedback. I will look into your request and test, if the implementation will also work for symlinks and directories. If so, its a good idea to stick to your suggested naming-scheme.

Additionally I can check if the RemoveAll is easy to implement, I have to see if I can spare enough time for that feature.

@deitch
Copy link
Collaborator

deitch commented Nov 15, 2024

@ludelion5 sorry to send you on a bit of a U turn. #264 was merged a few days ago, which extends the general interface with support for remove and rename, specifically as fs.Remove() and fs.Rename(). To keep fat32 compliant, it just made this not supported, same for ext4 and squashfs. Can you simplify this PR to just change the implementation of Remove() and Rename() in fat32? The others are not needed.

@ludelion5
Copy link
Contributor Author

Sorry for the late update on this PR.

We adapted our Rename and Remove methods to the new interface.

Additionally we have implemented the ability to remove empty directories and the renaming to already existing files (overwriting).
Currently, the renaming (move) works only in the same directory, this would require a deeper understanding of your fs.allocateSpace, I guess this method has to be called multiple times, depending on how many directory levels the file moves. We check if the base-dir is different and return an error.

func (fs *FileSystem) Rename(oldpath, newpath string) error {
return filesystem.ErrNotImplemented
}

// removes the named file or (empty) directory.
//
//nolint:revive // parameters will be used eventually
Copy link
Contributor

Choose a reason for hiding this comment

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

As the method got implemented, lint exception is not needed anymore here. Please remove it.

@deitch
Copy link
Collaborator

deitch commented Nov 17, 2024

Currently, the renaming (move) works only in the same directory, this would require a deeper understanding of your fs.allocateSpace, I guess this method has to be called multiple times, depending on how many directory levels the file moves. We check if the base-dir is different and return an error

That certainly is enough for now. We go from "this is not implemented" to "this is partially implemented", which his an improvement. So thank you.

Copy link
Collaborator

@deitch deitch left a comment

Choose a reason for hiding this comment

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

Just the lint comment.

@ludelion5
Copy link
Contributor Author

done

deitch
deitch previously approved these changes Nov 17, 2024
@deitch
Copy link
Collaborator

deitch commented Nov 17, 2024

Just some lint errors that need to be fixed.

@ludelion5
Copy link
Contributor Author

make lint should now be satisfied

@deitch deitch merged commit 0aefa87 into diskfs:master Nov 17, 2024
20 checks passed
@deitch
Copy link
Collaborator

deitch commented Nov 17, 2024

Looks great, thanks!

@ludelion5
Copy link
Contributor Author

are you planning to create a new tag as well?

@deitch
Copy link
Collaborator

deitch commented Nov 17, 2024

Yeah but let's wait for this one.

@ludelion5
Copy link
Contributor Author

Do you have an estimation when a new tag is created?

@deitch
Copy link
Collaborator

deitch commented Dec 17, 2024

We can cut one now. We have the extended filesystem interface, this, go 1.22 restored support, some fixed, etc.

@deitch
Copy link
Collaborator

deitch commented Dec 17, 2024

https://github.com/diskfs/go-diskfs/releases/tag/v1.5.0

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.

Support to remove files
3 participants