Fix temporary file renaming #2242
Conversation
|
|
||
| namespace mamba_fs | ||
| { | ||
| // Like std::rename, but works across file systems |
There was a problem hiding this comment.
What does "across file systems" means here?
There was a problem hiding this comment.
Or did you mean that this is more like a move?
There was a problem hiding this comment.
I think the issue is that sometimes tmp is on a different filesystem partition and then the rename doesn't work as one needs to copy the file. Maybe there is something builtin to do rename_or_move?
There was a problem hiding this comment.
I'm aware of two ways to "move" files from path A to path B:
- Within the same file system, changing some inodes. Posix guarantees that this is atomic and that's what
std::renameuses IIUC. - Creating a copy and removing the old file, this is what's implemented here as I didn't find a std implementation.
Are there other ways to move files?
There was a problem hiding this comment.
Other ways would be to get into platform-specific apis, so I think it's fine here. Although indeed the name is not clear to me and some clarification on when/why it should be used in the comments would help.
There was a problem hiding this comment.
Agreed, will add!
| @@ -826,7 +826,16 @@ namespace mamba | |||
| { | |||
| fs::u8path state_file = json_file; | |||
| state_file.replace_extension(".state.json"); | |||
| fs::rename(m_temp_file->path(), json_file); | |||
There was a problem hiding this comment.
hmm, now we don't have rename at all? I think we should first try to do the rename, then the move. I'll update your PR.
Fixes #2239