feat: Add FileCopyMethod option / API #164
Conversation
eth-p
force-pushed
the
feat-copymethod-api
branch
4 times, most recently
from
98288f5
to
7bb84a4
Compare
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,
})
}There was a problem hiding this comment.
quick question: Do we use this error in this current version?
There was a problem hiding this comment.
Not yet. It's used in a later pull request I made
There was a problem hiding this comment.
OK, got it. In this case I'll merge it.
Very personally speaking, I would not like to mix what we don't use at that moment. Similar idea to YAGNI.
There was a problem hiding this comment.
I can remove it and add it back in the other PR, if that's preferred?
There was a problem hiding this comment.
No need. Thanks for your consideration. I understand why it's here and we don't have any doubt we will need it ;)
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
|
|
||
| // ErrUnsupportedCopyMethod is returned when the FileCopyMethod specified in | ||
| // Options is not supported. | ||
| var ErrUnsupportedCopyMethod = errors.New( |
There was a problem hiding this comment.
quick question: Do we use this error in this current version?
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.