-
-
Notifications
You must be signed in to change notification settings - Fork 115
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
[Fix] Code Refactor & Cleanup #154
base: main
Are you sure you want to change the base?
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 |
---|---|---|
|
@@ -86,18 +86,12 @@ func copyNextOrSkip(src, dest string, info os.FileInfo, opt Options) error { | |
// with considering existence of parent directory | ||
// and file permission. | ||
func fcopy(src, dest string, info os.FileInfo, opt Options) (err error) { | ||
|
||
var readcloser io.ReadCloser | ||
if opt.FS != nil { | ||
readcloser, err = opt.FS.Open(src) | ||
} else { | ||
readcloser, err = os.Open(src) | ||
} | ||
readcloser, err := fopen(src, opt) | ||
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. In the following function |
||
if err != nil { | ||
if os.IsNotExist(err) { | ||
return nil | ||
if !os.IsNotExist(err) { | ||
return | ||
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. [Just IMO for readability] 🤔 In addition, |
||
} | ||
return | ||
return nil | ||
} | ||
defer fclose(readcloser, &err) | ||
|
||
|
@@ -155,14 +149,23 @@ func fcopy(src, dest string, info os.FileInfo, opt Options) (err error) { | |
return | ||
} | ||
|
||
// fopen opens the named file, | ||
// and returns it regardless of its type | ||
// fs.File or *os.File | ||
func fopen(src string, opt Options) (io.ReadCloser, error) { | ||
if opt.FS != nil { | ||
return opt.FS.Open(src) | ||
} | ||
return os.Open(src) | ||
} | ||
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 love this. |
||
|
||
// dcopy is for a directory, | ||
// with scanning contents inside the directory | ||
// and pass everything to "copy" recursively. | ||
func dcopy(srcdir, destdir string, info os.FileInfo, opt Options) (err error) { | ||
if skip, err := onDirExists(opt, srcdir, destdir); err != nil { | ||
return err | ||
} else if skip { | ||
return nil | ||
skip, err := onDirExists(opt, srcdir, destdir) | ||
if err != nil || skip { | ||
return | ||
} | ||
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. [Just IMO for readability] 🤔 |
||
|
||
// Make dest dir with 0755 so that everything writable. | ||
|
@@ -172,20 +175,9 @@ func dcopy(srcdir, destdir string, info os.FileInfo, opt Options) (err error) { | |
} | ||
defer chmodfunc(&err) | ||
|
||
var entries []fs.DirEntry | ||
if opt.FS != nil { | ||
entries, err = fs.ReadDir(opt.FS, srcdir) | ||
if err != nil { | ||
return err | ||
} | ||
} else { | ||
entries, err = os.ReadDir(srcdir) | ||
if err != nil { | ||
if os.IsNotExist(err) { | ||
return nil | ||
} | ||
return err | ||
} | ||
entries, err := dread(srcdir, opt.FS) | ||
if err != nil || entries == nil { | ||
return | ||
} | ||
|
||
contents := make([]fs.FileInfo, 0, len(entries)) | ||
|
@@ -197,16 +189,12 @@ func dcopy(srcdir, destdir string, info os.FileInfo, opt Options) (err error) { | |
contents = append(contents, info) | ||
} | ||
|
||
if yes, err := shouldCopyDirectoryConcurrent(opt, srcdir, destdir); err != nil { | ||
return err | ||
} else if yes { | ||
if err := dcopyConcurrent(srcdir, destdir, contents, opt); err != nil { | ||
return err | ||
} | ||
} else { | ||
if err := dcopySequential(srcdir, destdir, contents, opt); err != nil { | ||
return err | ||
} | ||
shouldCopyConcurrent, err := shouldCopyDirectoryConcurrent(opt, srcdir, destdir) | ||
if err != nil { | ||
return | ||
} | ||
if err = chooseAndPerformCopyMethod(shouldCopyConcurrent, srcdir, destdir, contents, opt); err != nil { | ||
return | ||
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 don't see any benefit of separating this block to a function. |
||
} | ||
|
||
if opt.PreserveTimes { | ||
|
@@ -224,6 +212,30 @@ func dcopy(srcdir, destdir string, info os.FileInfo, opt Options) (err error) { | |
return | ||
} | ||
|
||
// dread reads the named directory, | ||
// it regardless if it's a filesystem | ||
// and returns list of directory entries. | ||
func dread(srcdir string, fsys fs.FS) ([]fs.DirEntry, error) { | ||
if fsys != nil { | ||
return fs.ReadDir(fsys, srcdir) | ||
} | ||
entries, err := os.ReadDir(srcdir) | ||
if err != nil { | ||
if !os.IsNotExist(err) { | ||
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. Even in |
||
return nil, err | ||
} | ||
return nil, nil | ||
} | ||
return entries, nil | ||
} | ||
|
||
func chooseAndPerformCopyMethod(shouldCopyConcurrent bool, srcdir, destdir string, contents []fs.FileInfo, opt Options) error { | ||
if shouldCopyConcurrent { | ||
return dcopyConcurrent(srcdir, destdir, contents, opt) | ||
} | ||
return dcopySequential(srcdir, destdir, contents, opt) | ||
} | ||
|
||
func dcopySequential(srcdir, destdir string, contents []os.FileInfo, opt Options) error { | ||
for _, content := range contents { | ||
cs, cd := filepath.Join(srcdir, content.Name()), filepath.Join(destdir, content.Name()) | ||
|
@@ -262,7 +274,10 @@ func dcopyConcurrent(srcdir, destdir string, contents []os.FileInfo, opt Options | |
|
||
func onDirExists(opt Options, srcdir, destdir string) (bool, error) { | ||
_, err := os.Stat(destdir) | ||
if err == nil && opt.OnDirExists != nil && destdir != opt.intent.dest { | ||
if err != nil && !os.IsNotExist(err) { | ||
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 love this condition separation and moving exception case first. |
||
return true, err // Unwelcome error type...! | ||
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 love this comment. |
||
} | ||
if opt.OnDirExists != nil && destdir != opt.intent.dest { | ||
switch opt.OnDirExists(srcdir, destdir) { | ||
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. IMO, we can separate another exception case |
||
case Replace: | ||
if err := os.RemoveAll(destdir); err != nil { | ||
|
@@ -271,8 +286,6 @@ func onDirExists(opt Options, srcdir, destdir string) (bool, error) { | |
case Untouchable: | ||
return true, nil | ||
} // case "Merge" is default behaviour. Go through. | ||
} else if err != nil && !os.IsNotExist(err) { | ||
return true, err // Unwelcome error type...! | ||
} | ||
return false, nil | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,7 +130,7 @@ func getDefaultOptions(src, dest string) Options { | |
OnError: nil, // Default is "accept error" | ||
Skip: nil, // Do not skip anything | ||
AddPermission: 0, // Add nothing | ||
PermissionControl: PerservePermission, // Just preserve permission | ||
PermissionControl: PreservePermission, // Just preserve permission | ||
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. In order to keep backward compatibility, we can still keep 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. Accordingly, let's painfully respect |
||
Sync: false, // Do not sync | ||
Specials: false, // Do not copy special files | ||
PreserveTimes: false, // Do not preserve the modification time | ||
|
@@ -155,8 +155,9 @@ func assureOptions(src, dest string, opts ...Options) Options { | |
} | ||
if opts[0].AddPermission > 0 { | ||
opts[0].PermissionControl = AddPermission(opts[0].AddPermission) | ||
} else if opts[0].PermissionControl == nil { | ||
opts[0].PermissionControl = PerservePermission | ||
} | ||
if opts[0].PermissionControl == nil { | ||
opts[0].PermissionControl = PreservePermission | ||
} | ||
opts[0].intent.src = defopt.intent.src | ||
opts[0].intent.dest = defopt.intent.dest | ||
|
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 love this.