Skip to content
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 to replace _rsync with _fileCopy #254

Merged

Conversation

abetomo
Copy link
Contributor

@abetomo abetomo commented May 2, 2017

In preparation for a problem, processing that can be switched by environment variable is put in place.
If the next version has been released and there are no major problems, Replace with _fileCopy.

If there is a problem with `_ fileCopy`, it is possible to switch to
`_rsync` in the environment variable
Copy link
Collaborator

@DeviaVir DeviaVir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, would it make sense to only use _fileCopy in the case of Windows, and rsync with other platforms? Is there an optimization to be had there?

@abetomo
Copy link
Contributor Author

abetomo commented May 2, 2017

@DeviaVir
According to the result of travis-ci, _ fileCopy seems to be a little bit faster.
(It does not measure strictly)
(It will be displayed as XX ms to the right of the test results when it took a long time to test.)

As the difference in speed is really small, should we use _rsync as usual except in Windows?
(There is also an advantage that the influence range is reduced, so will I fix it?)

@DeviaVir
Copy link
Collaborator

DeviaVir commented May 2, 2017

@abetomo long as fileCopy does not make it smaller, and indeed makes it faster, I'm fine with switching to that. Will merge this PR.

@DeviaVir DeviaVir merged commit 63d9020 into motdotla:master May 2, 2017
@abetomo
Copy link
Contributor Author

abetomo commented May 2, 2017

@DeviaVir Thank you!

@abetomo abetomo deleted the fix_to_replace_rsync_with_filecopy branch May 2, 2017 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants