From 61a182077f20b01de6eb60e035fdef7c457da500 Mon Sep 17 00:00:00 2001
From: Jakob Borg <jakob@nym.se>
Date: Sat, 8 Aug 2015 12:44:17 +0200
Subject: [PATCH] Clarify and correct handling of existing files/directories
 when pulling

This fixes a corner case I discovered in the symlink branch, where we
unexpectedly succeed in "replacing" an entire non-empty directory tree
with a file or symlink. This happens when archiving is in use, as we
then just move the entire tree away into the archive. This is wrong as
we should just archive files and fail on non-empty dirs in all cases.

New handling first checks what the (old) thing is, and if it's a
directory or symlink just does the delete, otherwise does conflict
handling or archiving as appropriate.
---
 lib/model/rwfolder.go | 64 ++++++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 25 deletions(-)

diff --git a/lib/model/rwfolder.go b/lib/model/rwfolder.go
index 629ba7270..cb74e6fed 100644
--- a/lib/model/rwfolder.go
+++ b/lib/model/rwfolder.go
@@ -1260,34 +1260,48 @@ func (p *rwFolder) performFinish(state *sharedPullerState) error {
 		p.virtualMtimeRepo.UpdateMtime(state.file.Name, info.ModTime(), t)
 	}
 
-	var err error
-	if p.inConflict(state.version, state.file.Version) {
-		// The new file has been changed in conflict with the existing one. We
-		// should file it away as a conflict instead of just removing or
-		// archiving. Also merge with the version vector we had, to indicate
-		// we have resolved the conflict.
-		state.file.Version = state.file.Version.Merge(state.version)
-		err = osutil.InWritableDir(moveForConflict, state.realName)
-	} else if p.versioner != nil {
-		// If we should use versioning, let the versioner archive the old
-		// file before we replace it. Archiving a non-existent file is not
-		// an error.
-		err = p.versioner.Archive(state.realName)
-	} else {
-		err = nil
-	}
-	if err != nil {
-		return err
+	if stat, err := osutil.Lstat(state.realName); err == nil {
+		// There is an old file or directory already in place. We need to
+		// handle that.
+
+		switch {
+		case stat.IsDir() || stat.Mode()&os.ModeSymlink != 0:
+			// It's a directory or a symlink. These are not versioned or
+			// archived for conflicts, only removed (which of course fails for
+			// non-empty directories).
+
+			// TODO: This is the place where we want to remove temporary files
+			// and future hard ignores before attempting a directory delete.
+			// Should share code with p.deletDir().
+
+			if err = osutil.InWritableDir(osutil.Remove, state.realName); err != nil {
+				return err
+			}
+
+		case p.inConflict(state.version, state.file.Version):
+			// The new file has been changed in conflict with the existing one. We
+			// should file it away as a conflict instead of just removing or
+			// archiving. Also merge with the version vector we had, to indicate
+			// we have resolved the conflict.
+
+			state.file.Version = state.file.Version.Merge(state.version)
+			if err = osutil.InWritableDir(moveForConflict, state.realName); err != nil {
+				return err
+			}
+
+		case p.versioner != nil:
+			// If we should use versioning, let the versioner archive the old
+			// file before we replace it. Archiving a non-existent file is not
+			// an error.
+
+			if err = p.versioner.Archive(state.realName); err != nil {
+				return err
+			}
+		}
 	}
 
-	// If the target path is a symlink or a directory, we cannot copy
-	// over it, hence remove it before proceeding.
-	stat, err := osutil.Lstat(state.realName)
-	if err == nil && (stat.IsDir() || stat.Mode()&os.ModeSymlink != 0) {
-		osutil.InWritableDir(osutil.Remove, state.realName)
-	}
 	// Replace the original content with the new one
-	if err = osutil.Rename(state.tempName, state.realName); err != nil {
+	if err := osutil.Rename(state.tempName, state.realName); err != nil {
 		return err
 	}