feat: Add FileCopyMethod option / API #164
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eth-p
force-pushed
the
feat-copymethod-api
branch
4 times, most recently
from
98288f5
to
7bb84a4
Compare
otiai10
reviewed
Sep 5, 2024
options.go
Outdated
| @@ -27,6 +27,10 @@ type Options struct { | |||
| // RenameDestination can specify the destination file or dir name if needed to rename. | |||
| RenameDestination func(src, dest string) (string, error) | |||
|
|
|||
| // FileCopyFunc specifies the method by which a regular file is copied. | |||
| // The default is CopyBytes. | |||
| FileCopyFunc FileCopyMethod | |||
There was a problem hiding this comment.
What's the purpose of using different names here?
Both the name of prop and the type can be the same. Let me know your thoughts
There was a problem hiding this comment.
Good point. I'll make them both FileCopyMethod
|
|
||
| // ErrUnsupportedCopyMethod is returned when the FileCopyMethod specified in | ||
| // Options is not supported. | ||
| var ErrUnsupportedCopyMethod = errors.New( |
There was a problem hiding this comment.
So the user can tell if the copy failed because of no support, instead of another error like permission:
err := copy.Copy("/from", "/to", copy.Options {
FileCopyMethod: copy.ReflinkCopy,
})
if errors.Is(err, copy.ErrUnsupportedCopyMethod) {
// Retry with other method
err = copy.Copy("/from", "/to", copy.Options {
FileCopyMethod: copy.CopyBytes,
})
}
copy.go
Outdated
| return | ||
| // Use FileCopyMethod to do copy. | ||
| if err = opt.FileCopyFunc.fcopy(src, dest, info, opt); err != nil { | ||
| if pathError, ok := err.(*os.PathError); ok && errors.Is(err, os.ErrNotExist) && pathError.Path == src { |
There was a problem hiding this comment.
Just at a glance, it looks like increasing complexity of source code.
Do we really have a specific issue we need to solve in this PR?
or do you think we can separate PR for different point of interest?
There was a problem hiding this comment.
If I don't do this here, it breaks the file is deleted while copying test. Let me see if I can do something else though
There was a problem hiding this comment.
I changed it to have a skipFile return instead
This will be used so we can have multiple implementations for copying a regular file. Currently, there is just CopyBytes, which is the old fcopy code. The FileCopyMethod struct is a struct with an un-exported member. This is done to make the type opaque so the user can't make their own copy function. There are still design questions left about passing around the `Options` struct and `os.FileInfo`, so I opted to do this so that can be figured out later. The type can be made into `type FileCopyMethod func(...)` later, and it won't be a breaking change. Or, instead, a `UserCopy(func (src, dest string) error) FileCopyMethod` function can be added.
Permission changes, ownership preservation, and access/modify/creation time presevation is something all implementations need. I brought it out of the CopyBytes implementation so it can be shared when more implementations are added later. A small note: The "ignore if source file deleted" check had to be brought out of `CopyBytes`. The way it worked before is it returned no error, which meant `fcopy` would try and modify the destination file (which was never created). Instead, I check for the error explicitly in `fcopy`.
External contributors don't do development on the main or develop branch, but may still need to see the CI results. This lets them do `gh workflow run Go --repo=<user>/copy` on their own branches.
eth-p
force-pushed
the
feat-copymethod-api
branch
from
35d00a9
to
f530620
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implemented as discussed in #163 in this comment.
A change was needed:
I have to check for the source file being deleted after the
FileCopyMethodfunction is run.nilif the source was deleted.fcopy, it would think there was no error.fcopywould then try to apply changes to the destination file that was never made, then error.I also had to change the CI so I could run workflows on my own repo. You can drop that commit before merging it.