config: do not remove/overwrite other files during config file save - fixes #3759

docs/content/docs.md

@@ -952,15 +952,9 @@ To reduce risk of corrupting an existing configuration file, rclone
 will not write directly to it when saving changes. Instead it will
 first write to a new, temporary, file. If a configuration file already
 existed, it will (on Unix systems) try to mirror its permissions to
-the new file. Then it will rename the existing file by adding suffix
+the new file. Then it will rename the existing file to a temporary
-".old" to its name (e.g. `rclone.conf.old`), if a file with the same
+name as backup. Next, rclone will rename the new file to the correct name,
-name already exists it will simply be replaced. Next, rclone will rename
+before finally cleaning up by deleting the backup file.
-the new file to the correct name, before finally cleaning up by deleting
-the original file now which has now ".old" suffix to its name. Note that
-one side-effect of this is that if you happen to have a file in the same
-directory and with the same name as your configuration file, but with
-suffix ".old", then rclone will end up deleting this file next time it
-updates its configuration file!
 
 ### --contimeout=TIME ###
 

fs/config/configfile/configfile.go

@@ -106,7 +106,6 @@ func (s *Storage) Save() error {
 	if configPath == "" {
 		return fmt.Errorf("failed to save config file, path is empty")
 	}
-
 	dir, name := filepath.Split(configPath)
 	err := file.MkdirAll(dir, os.ModePerm)
 	if err != nil {
@@ -119,7 +118,7 @@ func (s *Storage) Save() error {
 	defer func() {
 		_ = f.Close()
 		if err := os.Remove(f.Name()); err != nil && !os.IsNotExist(err) {
-			fs.Errorf(nil, "failed to remove temp config file: %v", err)
+			fs.Errorf(nil, "Failed to remove temp file for new config: %v", err)
 		}
 	}()
 
@@ -154,15 +153,33 @@ func (s *Storage) Save() error {
 		fs.Errorf(nil, "Failed to set permissions on config file: %v", err)
 	}
 
-	if err = os.Rename(configPath, configPath+".old"); err != nil && !os.IsNotExist(err) {
+	fbackup, err := os.CreateTemp(dir, name+".old")
-		return fmt.Errorf("failed to move previous config to backup location: %w", err)
+	if err != nil {
+		return fmt.Errorf("failed to create temp file for old config backup: %w", err)
+	}
+	err = fbackup.Close()
+	if err != nil {
+		return fmt.Errorf("failed to close temp file for old config backup: %w", err)
+	}
+	keepBackup := true
+	defer func() {
+		if !keepBackup {
+			if err := os.Remove(fbackup.Name()); err != nil && !os.IsNotExist(err) {
+				fs.Errorf(nil, "Failed to remove temp file for old config backup: %v", err)
+			}
+		}
+	}()
+
+	if err = os.Rename(configPath, fbackup.Name()); err != nil {
+		if !os.IsNotExist(err) {
+			return fmt.Errorf("failed to move previous config to backup location: %w", err)
+		}
+		keepBackup = false // no existing file, no need to keep backup even if writing of new file fails
 	}
 	if err = os.Rename(f.Name(), configPath); err != nil {
 		return fmt.Errorf("failed to move newly written config from %s to final location: %v", f.Name(), err)
 	}
-	if err := os.Remove(configPath + ".old"); err != nil && !os.IsNotExist(err) {
+	keepBackup = false // new file was written, no need to keep backup
-		fs.Errorf(nil, "Failed to remove backup config file: %v", err)
-	}
 
 	// Update s.fi with the newly written file
 	s.fi, _ = os.Stat(configPath)