-
Notifications
You must be signed in to change notification settings - Fork 282
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
Move final file if in same file system, not only in same directory #211
base: main
Are you sure you want to change the base?
Conversation
189ba6c
to
3c574f1
Compare
Hello - thanks for your fix. We do want to move to C++ stdlib calls, but have had to move slowly because of the number of platforms we support. @wjblanke will this work on the oldest macos we currently support? |
@@ -25,3 +25,4 @@ build | |||
.mypy_cache | |||
*.whl | |||
venv | |||
/.vs |
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.
was this deliberate?
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.
Visual Studio added it, and I thought since .vscode is already in there I can include it
src/plotter_disk.hpp
Outdated
@@ -369,7 +369,9 @@ class DiskPlotter { | |||
Timer copy; | |||
do { | |||
std::error_code ec; | |||
if (tmp_2_filename.parent_path() == final_filename.parent_path()) { | |||
bool same_filesystem = | |||
fs::equivalent(tmp_2_filename.parent_path(), final_filename.parent_path(), ec); |
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.
the name same_filesystem
make this sound like a broader check than it is. The parent paths may refer to different directories, but still live within the same filesystem.
In order to prefer a rename, when that's possible, I think the most reliable way is to try renaming, and if it fails, fall back to copying.
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 am sorry, my bad. I misread the documentation of fs::equivalent
as that it checks whether the two paths resign in the same filesystem.
I updated my pull request to try a rename before falling back to a copy.
I think the question is whether Gulrak supports it. If it is compiling on MacOS should be OK |
Also note there is an update to Gulrak - #259 |
'This PR has been flagged as stale due to no activity for over 60 |
Use
std::filesystem::equivalent
instead of just comparing the parent directories to determine if tmp_2_file can be moved.Prevents useless IO activity and speeds up plotting if temp_2_file and final_file are in the same file system.