From dd81a7f0d7feee56f34ba4fa3c0a382fa9c23cab Mon Sep 17 00:00:00 2001 From: Walter Doekes Date: Sat, 27 Aug 2022 16:03:40 +0200 Subject: [PATCH] Do not move symlinks when saving a map, overwrite target instead If the user is editing a symlink to a target instead of a real file, chances are high they want the symlink to stay in place. Precondition: some.map -> /path/to/elsewhere.map After save (before): some.bak -> /path/to/elsewhere.map some.map (real file) After save (after): some.map -> /path/to/elsewhere.map Closes #107. --- libs/os/file.h | 22 +++++++++++++++++++--- radiant/preferences.cpp | 5 +---- radiant/referencecache.cpp | 20 ++++++++++++-------- 3 files changed, 32 insertions(+), 15 deletions(-) diff --git a/libs/os/file.h b/libs/os/file.h index 29af303f..984db856 100644 --- a/libs/os/file.h +++ b/libs/os/file.h @@ -45,12 +45,14 @@ #include // access() #endif -#include // rename(), remove() +#include // remove() #include // stat() #include // this is included by stat.h on win32 #include #include +#include // std::filesystem::rename() + #include "debugging/debugging.h" /// \brief Attempts to move the file identified by \p from to \p to and returns true if the operation was successful. @@ -62,7 +64,12 @@ /// - The directory component of \p to identifies an existing directory which is accessible for writing. inline bool file_move( const char* from, const char* to ){ ASSERT_MESSAGE( from != 0 && to != 0, "file_move: invalid path" ); - return rename( from, to ) == 0; + std::error_code err; + // Contrary to ::rename, std::filesystem::rename guarantees that + // existing files will be overwritten on both Windows and POSIX + // systems. + std::filesystem::rename( from, to, err ); + return !err; } /// \brief Attempts to remove the file identified by \p path and returns true if the operation was successful. @@ -98,7 +105,7 @@ inline bool file_readable( const char* path ){ } /// \brief Returns true if the file or directory identified by \p path exists and may be opened for writing. -inline bool file_writeable( const char* path ){ +inline bool file_writable( const char* path ){ return file_accessible( path, FileAccess::Write ); } @@ -117,6 +124,15 @@ inline bool file_is_directory( const char* path ){ return S_ISDIR( st.st_mode ) != 0; } +/// \brief Returns true if the target is a symlink and no error occurred. +inline bool file_is_symlink( const char* path ){ + ASSERT_MESSAGE( path != 0, "file_is_symlink: invalid path" ); + std::error_code err; + // Contrary to ::lstat, std::filesystem::is_symlink is portable. + bool ret = std::filesystem::is_symlink( path, err ); + return ret && !err; +} + typedef std::size_t FileSize; /// \brief Returns the size in bytes of the file identified by \p path, or 0 if the file was not found. diff --git a/radiant/preferences.cpp b/radiant/preferences.cpp index 38992bb6..577ca9d9 100644 --- a/radiant/preferences.cpp +++ b/radiant/preferences.cpp @@ -175,13 +175,10 @@ bool Preferences_Save( PreferenceDictionary& preferences, const char* filename ) bool Preferences_Save_Safe( PreferenceDictionary& preferences, const char* filename ){ const auto tmpName = StringOutputStream()( filename, "TMP" ); - return Preferences_Save( preferences, tmpName ) - && ( !file_exists( filename ) || file_remove( filename ) ) - && file_move( tmpName, filename ); + return Preferences_Save( preferences, tmpName ) && file_move( tmpName, filename ); } - void LogConsole_importString( const char* string ){ g_Console_enableLogging = string_equal( string, "true" ); Sys_LogFile( g_Console_enableLogging ); diff --git a/radiant/referencecache.cpp b/radiant/referencecache.cpp index 430370c6..34ded712 100644 --- a/radiant/referencecache.cpp +++ b/radiant/referencecache.cpp @@ -114,14 +114,15 @@ bool MapResource_saveFile( const MapFormat& format, scene::Node& root, GraphTrav } bool file_saveBackup( const char* path ){ - if ( file_writeable( path ) ) { - const auto backup = StringOutputStream( 256 )( PathExtensionless( path ), ".bak" ); + const auto backup = StringOutputStream( 256 )( PathExtensionless( path ), ".bak" ); - return ( !file_exists( backup ) || file_remove( backup ) ) // remove backup - && file_move( path, backup ); // rename current to backup + if ( file_move( path, backup.c_str() ) ) { + return true; } - - globalErrorStream() << "map path is not writeable: " << makeQuoted( path ) << "\n"; + if ( !file_exists( path ) ) { + return true; // nothing to move, no wonder it failed + } + globalErrorStream() << "map path (or backup path) is not writable: " << makeQuoted( path ) << "\n"; return false; } @@ -130,11 +131,14 @@ bool MapResource_save( const MapFormat& format, scene::Node& root, const char* p fullpath << path << name; if ( path_is_absolute( fullpath.c_str() ) ) { - if ( !file_exists( fullpath.c_str() ) || file_saveBackup( fullpath.c_str() ) ) { + // We don't want a backup + rename operation if the .map file is + // a symlink. Otherwise we'll break the user's careful symlink setup. + // Just overwrite the original file. Assume the user has versioning. + if ( file_is_symlink( fullpath.c_str() ) || file_saveBackup( fullpath.c_str() ) ) { return MapResource_saveFile( format, root, Map_Traverse, fullpath.c_str() ); } - globalErrorStream() << "failed to save a backup map file: " << makeQuoted( fullpath.c_str() ) << "\n"; + globalErrorStream() << "failed to save map file: " << makeQuoted( fullpath.c_str() ) << "\n"; return false; }