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

Move final file if in same file system, not only in same directory #211

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aydreas
Copy link

@aydreas aydreas commented Apr 21, 2021

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.

@aydreas aydreas force-pushed the main branch 2 times, most recently from 189ba6c to 3c574f1 Compare April 21, 2021 17:49
@aqk
Copy link
Contributor

aqk commented Apr 24, 2021

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?

@aqk aqk requested a review from wjblanke April 24, 2021 06:10
@@ -25,3 +25,4 @@ build
.mypy_cache
*.whl
venv
/.vs
Copy link
Contributor

Choose a reason for hiding this comment

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

was this deliberate?

Copy link
Author

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

@@ -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);
Copy link
Contributor

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.

Copy link
Author

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.

@wjblanke
Copy link
Contributor

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?

I think the question is whether Gulrak supports it. If it is compiling on MacOS should be OK

@hoffmang9
Copy link
Member

Also note there is an update to Gulrak - #259

@github-actions
Copy link

'This PR has been flagged as stale due to no activity for over 60
days. It will not be automatically closed, but it has been given
a stale-pr label and should be manually reviewed.'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants