-
-
Notifications
You must be signed in to change notification settings - Fork 116
[PoC] copy on write #161
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
base: main
Are you sure you want to change the base?
[PoC] copy on write #161
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,11 @@ type Options struct { | |
| // e.g., You can use embed.FS to copy files from embedded filesystem. | ||
| FS fs.FS | ||
|
|
||
| // 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooh, that's a clever way of handling it. A few questions, though:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be better not to give the user full control over the function but instead let them pick between pre-made options. |
||
|
|
||
| // NumOfWorkers represents the number of workers used for | ||
| // concurrent copying contents of directories. | ||
| // If 0 or 1, it does not use goroutine for copying directories. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| # Support CopyOnWrite of macOS | ||
|
|
||
| // TODO: Write more description |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| //go:build darwin | ||
|
|
||
| package copyonwrite | ||
|
|
||
| import "golang.org/x/sys/unix" | ||
|
|
||
| func CopyOnWrite(src, dst string) error { | ||
| return unix.Clonefile(src, dst, 0) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will also fail if:
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 I think falling back to read and write copy on error is going to be the most compatible approach. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| //go:build !darwin | ||
|
|
||
| package copyonwrite | ||
|
|
||
| import "errors" | ||
|
|
||
| func CopyOnWrite(src, dst string) error { | ||
| return errors.New("copy-on-write is not supported on this platform") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| # CopyOnWrite plugin |
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.
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
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.
I doesn't matter