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

[copy_file] allow FILE_SHARE_* #2718

Merged
merged 3 commits into from
May 17, 2022
Merged

Conversation

strega-nil-ms
Copy link
Contributor

The existing code does not allow other programs to hold read permissions
on _Source or _Target when
copy_options = overwrite_existing or skip_existing.
We could very easily allow reading, and could also allow writing for
skip_existing, except for a clever implementation of equivalent,
so just switch to a dumb implementation of equivalent here.

Fixes #2185

The existing code does not allow other programs to hold read permissions
on _Source or _Target when
copy_options = overwrite_existing or skip_existing.
We could very easily allow reading, and could also allow writing for
skip_existing, except for a clever implementation of equivalent,
so just switch to a dumb implementation of equivalent here.
@strega-nil-ms strega-nil-ms requested a review from a team as a code owner May 13, 2022 02:27
@strega-nil-ms strega-nil-ms changed the title [copy_file] allow FILE_SHARE_* [copy_file] allow FILE_SHARE_* May 13, 2022
@StephanTLavavej StephanTLavavej added bug Something isn't working filesystem C++17 filesystem labels May 13, 2022
stl/src/filesystem.cpp Outdated Show resolved Hide resolved
stl/src/filesystem.cpp Outdated Show resolved Hide resolved
stl/src/filesystem.cpp Outdated Show resolved Hide resolved
return _Last_error; // real error, bail to the caller
}

#ifndef _CRT_APP
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we simplify the else case to simply be return __std_win_error{...} and move the switch into here

Copy link
Member

Choose a reason for hiding this comment

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

This is pre-existing code that's being extracted from __std_fs_get_file_id(). I see what you're suggesting, and the control flow transformation wouldn't be risky, but my general preference is to avoid significantly altering code when moving it (unless it's necessary). We can do this as a followup cleanup, since this doesn't affect correctness or performance.

@StephanTLavavej StephanTLavavej self-assigned this May 16, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request May 16, 2022
@StephanTLavavej StephanTLavavej merged commit 1a20fe1 into microsoft:main May 17, 2022
@StephanTLavavej
Copy link
Member

Thanks for teaching the STL how to play nice and share!

😹 📂 🎉

@strega-nil strega-nil deleted the fix-2185 branch August 10, 2022 20:35
fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working filesystem C++17 filesystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<filesystem>: copy_file(a, b, update_existing) fails when b is locked even if no copy is needed
5 participants