Change behaviour of Storage.Move to overwrite existing by default
#5984
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Coming from https://github.com/ppy/osu/actions/runs/5948760882/job/16133189924, the only way I can see this happening is if the underlying `File.Move` logic is less atomic than we expect it to be. In the process of looking into this, I noticed there's an `overwrite` flag available for `File.Move` which tends to prefer a `rename` operation internally on unix, rather than `link`. On windows (the failcase in question) it likely won't help at the end of the day. Anyway, personally I think having it overwrite by default is what we prefer from our API. If that isn't agreed on then closing this without further consideration is fine.
|
I weakly agree with this. The other option is to do as .NET does and add an overload. |
bdach
approved these changes
Aug 24, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Coming from https://github.com/ppy/osu/actions/runs/5948760882/job/16133189924, the only way I can see this happening is if the underlying
File.Movelogic is less atomic than we expect it to be.In the process of looking into this, I noticed there's an
overwriteflag available forFile.Movewhich tends to prefer arenameoperation internally on unix, rather thanlink.On windows (the failcase in question) it likely won't help at the end of the day.
Anyway, personally I think having it overwrite by default is what we prefer from our API. If that isn't agreed on then closing this without further consideration is fine.