Merge pull request #108 from wdoekes/do-not-clobber-symlink-when-saving-map

Do not move symlinks when saving a map, overwrite target instead

libs/os/file.h

@@ -45,12 +45,14 @@
 #include <unistd.h> // access()
 #endif
 
-#include <cstdio> // rename(), remove()
+#include <cstdio> // remove()
 #include <sys/stat.h> // stat()
 #include <sys/types.h> // this is included by stat.h on win32
 #include <cstddef>
 #include <ctime>
 
+#include <filesystem> // 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.

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 );

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;
 	}