[PoC] copy on write #161
[PoC] copy on write #161
Conversation
| import "errors" | ||
|
|
||
| func CopyOnWrite(src, dst string) error { | ||
| return errors.New("copy-on-write is not supported on this platform") |
There was a problem hiding this comment.
I think it would be better to fall back to read and write copy here. Having it fail with an error is going to cause problems if people write code on one platform but never test it on any others.
In this proposal, there also isn't any way to check if the platform does support copy-on-write.
| import "golang.org/x/sys/unix" | ||
|
|
||
| func CopyOnWrite(src, dst string) error { | ||
| return unix.Clonefile(src, dst, 0) |
There was a problem hiding this comment.
This needs to be able to gracefully handle EEXIST in order to be compatible with the read and write copy.
Read and write copy will just overwrite the file, but clonefile will fail if the destination already exists.
| // FileCopyFunc is a function to copy an actual file. | ||
| // If nil, it uses the default file copying function with `io.Copy`. | ||
| // e.g., You can use `CopyOnWrite` to copy files with copy-on-write. | ||
| FileCopyFunc func(src, dest string) error |
There was a problem hiding this comment.
Ooh, that's a clever way of handling it.
A few questions, though:
-
How does this work when the source is a
fs.FS. If it's not supposed to, how can that be communicated to the user? Documentation? Error? -
Similarly, what about also using the
Syncoption orWrapReaderoption?
There was a problem hiding this comment.
It might be better not to give the user full control over the function but instead let them pick between pre-made options.
| } | ||
| } | ||
|
|
||
| chmodfunc, err := opt.PermissionControl(info, dest) |
There was a problem hiding this comment.
Just a question for my own reference: the point where the permissions are changed doesn't matter, as long as it happens?
Example: after file creation, or after the file is finished writing
| import "golang.org/x/sys/unix" | ||
|
|
||
| func CopyOnWrite(src, dst string) error { | ||
| return unix.Clonefile(src, dst, 0) |
There was a problem hiding this comment.
This will also fail if:
- The filesystem doesn't support copy-on-write.
- The source file is on a different filesystem than the destination.
You can check for both of these ahead of time for every file, but it's more platform-specific logic and overhead. It wouldn't be possible to only do a single check on the original source and destination given to Copy either, since a directory can contain mount points for other filesystems.
I think falling back to read and write copy on error is going to be the most compatible approach.
Interface design PoC of #160
If any modification is needed, please send pull-request to this branch first.
This pull request will close #160
Huge thanks to @eth-p