Merge pull request #2077 from calmh/conflictwins

Determine conflict winner based on change type and modification time (fixes #1848)

Godeps/Godeps.json

@@ -35,7 +35,7 @@
 		},
 		{
 			"ImportPath": "github.com/syncthing/protocol",
-			"Rev": "7996ef0d45b7743ff930048b6413b37b2c33cd85"
+			"Rev": "fcbd12b42176237d48985c1bfafbc43b2107aa31"
 		},
 		{
 			"ImportPath": "github.com/syndtr/goleveldb/leveldb",

Godeps/_workspace/src/github.com/syncthing/protocol/conflict_test.go

@@ -0,0 +1,23 @@
+// Copyright (C) 2015 The Protocol Authors.
+
+package protocol
+
+import "testing"
+
+func TestWinsConflict(t *testing.T) {
+	testcases := [][2]FileInfo{
+		// The first should always win over the second
+		{{Modified: 42}, {Modified: 41}},
+		{{Modified: 41}, {Modified: 42, Flags: FlagDeleted}},
+		{{Modified: 41, Version: Vector{{42, 2}, {43, 1}}}, {Modified: 41, Version: Vector{{42, 1}, {43, 2}}}},
+	}
+
+	for _, tc := range testcases {
+		if !tc[0].WinsConflict(tc[1]) {
+			t.Errorf("%v should win over %v", tc[0], tc[1])
+		}
+		if tc[1].WinsConflict(tc[0]) {
+			t.Errorf("%v should not win over %v", tc[1], tc[0])
+		}
+	}
+}

Godeps/_workspace/src/github.com/syncthing/protocol/message.go

@@ -58,6 +58,31 @@ func (f FileInfo) HasPermissionBits() bool {
 	return f.Flags&FlagNoPermBits == 0
 }
 
+// WinsConflict returns true if "f" is the one to choose when it is in
+// conflict with "other".
+func (f FileInfo) WinsConflict(other FileInfo) bool {
+	// If a modification is in conflict with a delete, we pick the
+	// modification.
+	if !f.IsDeleted() && other.IsDeleted() {
+		return true
+	}
+	if f.IsDeleted() && !other.IsDeleted() {
+		return false
+	}
+
+	// The one with the newer modification time wins.
+	if f.Modified > other.Modified {
+		return true
+	}
+	if f.Modified < other.Modified {
+		return false
+	}
+
+	// The modification times were equal. Use the device ID in the version
+	// vector as tie breaker.
+	return f.Version.Compare(other.Version) == ConcurrentGreater
+}
+
 type BlockInfo struct {
 	Offset int64 // noencode (cache only)
 	Size   int32

internal/db/leveldb.go

@@ -236,7 +236,7 @@ func ldbGenericReplace(db *leveldb.DB, folder, device []byte, fs []protocol.File
 			if fs[fsi].IsInvalid() {
 				ldbRemoveFromGlobal(snap, batch, folder, device, newName)
 			} else {
-				ldbUpdateGlobal(snap, batch, folder, device, newName, fs[fsi].Version)
+				ldbUpdateGlobal(snap, batch, folder, device, fs[fsi])
 			}
 			fsi++
 
@@ -259,7 +259,7 @@ func ldbGenericReplace(db *leveldb.DB, folder, device []byte, fs []protocol.File
 				if fs[fsi].IsInvalid() {
 					ldbRemoveFromGlobal(snap, batch, folder, device, newName)
 				} else {
-					ldbUpdateGlobal(snap, batch, folder, device, newName, fs[fsi].Version)
+					ldbUpdateGlobal(snap, batch, folder, device, fs[fsi])
 				}
 			} else if debugDB {
 				l.Debugln("generic replace; equal - ignore")
@@ -348,7 +348,7 @@ func ldbReplaceWithDelete(db *leveldb.DB, folder, device []byte, fs []protocol.F
 			}
 			batch.Put(dbi.Key(), bs)
 			mtimeRepo.DeleteMtime(tf.Name)
-			ldbUpdateGlobal(db, batch, folder, device, deviceKeyName(dbi.Key()), f.Version)
+			ldbUpdateGlobal(db, batch, folder, device, f)
 			return ts
 		}
 		return 0
@@ -392,7 +392,7 @@ func ldbUpdate(db *leveldb.DB, folder, device []byte, fs []protocol.FileInfo) in
 			if f.IsInvalid() {
 				ldbRemoveFromGlobal(snap, batch, folder, device, name)
 			} else {
-				ldbUpdateGlobal(snap, batch, folder, device, name, f.Version)
+				ldbUpdateGlobal(snap, batch, folder, device, f)
 			}
 			continue
 		}
@@ -411,7 +411,7 @@ func ldbUpdate(db *leveldb.DB, folder, device []byte, fs []protocol.FileInfo) in
 			if f.IsInvalid() {
 				ldbRemoveFromGlobal(snap, batch, folder, device, name)
 			} else {
-				ldbUpdateGlobal(snap, batch, folder, device, name, f.Version)
+				ldbUpdateGlobal(snap, batch, folder, device, f)
 			}
 		}
 
@@ -464,11 +464,12 @@ func ldbInsert(batch dbWriter, folder, device []byte, file protocol.FileInfo) in
 // ldbUpdateGlobal adds this device+version to the version list for the given
 // file. If the device is already present in the list, the version is updated.
 // If the file does not have an entry in the global list, it is created.
-func ldbUpdateGlobal(db dbReader, batch dbWriter, folder, device, file []byte, version protocol.Vector) bool {
+func ldbUpdateGlobal(db dbReader, batch dbWriter, folder, device []byte, file protocol.FileInfo) bool {
 	if debugDB {
-		l.Debugf("update global; folder=%q device=%v file=%q version=%d", folder, protocol.DeviceIDFromBytes(device), file, version)
+		l.Debugf("update global; folder=%q device=%v file=%q version=%d", folder, protocol.DeviceIDFromBytes(device), file.Name, file.Version)
 	}
-	gk := globalKey(folder, file)
+	name := []byte(file.Name)
+	gk := globalKey(folder, name)
 	svl, err := db.Get(gk, nil)
 	if err != nil && err != leveldb.ErrNotFound {
 		panic(err)
@@ -485,7 +486,7 @@ func ldbUpdateGlobal(db dbReader, batch dbWriter, folder, device, file []byte, v
 
 		for i := range fl.versions {
 			if bytes.Compare(fl.versions[i].device, device) == 0 {
-				if fl.versions[i].version.Equal(version) {
+				if fl.versions[i].version.Equal(file.Version) {
 					// No need to do anything
 					return false
 				}
@@ -497,21 +498,38 @@ func ldbUpdateGlobal(db dbReader, batch dbWriter, folder, device, file []byte, v
 
 	nv := fileVersion{
 		device:  device,
-		version: version,
+		version: file.Version,
 	}
+
+	// Find a position in the list to insert this file. The file at the front
+	// of the list is the newer, the "global".
 	for i := range fl.versions {
-		// We compare  against ConcurrentLesser as well here because we need
-		// to enforce a consistent ordering of versions even in the case of
-		// conflicts.
-		if comp := fl.versions[i].version.Compare(version); comp == protocol.Equal || comp == protocol.Lesser || comp == protocol.ConcurrentLesser {
-			t := append(fl.versions, fileVersion{})
-			copy(t[i+1:], t[i:])
-			t[i] = nv
-			fl.versions = t
+		switch fl.versions[i].version.Compare(file.Version) {
+		case protocol.Equal, protocol.Lesser:
+			// The version at this point in the list is equal to or lesser
+			// ("older") than us. We insert ourselves in front of it.
+			fl.versions = insertVersion(fl.versions, i, nv)
 			goto done
+
+		case protocol.ConcurrentLesser, protocol.ConcurrentGreater:
+			// The version at this point is in conflict with us. We must pull
+			// the actual file metadata to determine who wins. If we win, we
+			// insert ourselves in front of the loser here. (The "Lesser" and
+			// "Greater" in the condition above is just based on the device
+			// IDs in the version vector, which is not the only thing we use
+			// to determine the winner.)
+			of, ok := ldbGet(db, folder, fl.versions[i].device, name)
+			if !ok {
+				panic("file referenced in version list does not exist")
+			}
+			if file.WinsConflict(of) {
+				fl.versions = insertVersion(fl.versions, i, nv)
+				goto done
+			}
 		}
 	}
 
+	// We didn't find a position for an insert above, so append to the end.
 	fl.versions = append(fl.versions, nv)
 
 done:
@@ -524,6 +542,13 @@ done:
 	return true
 }
 
+func insertVersion(vl []fileVersion, i int, v fileVersion) []fileVersion {
+	t := append(vl, fileVersion{})
+	copy(t[i+1:], t[i:])
+	t[i] = v
+	return t
+}
+
 // ldbRemoveFromGlobal removes the device from the global version list for the
 // given file. If the version list is empty after this, the file entry is
 // removed entirely.
@@ -644,7 +669,7 @@ func ldbWithAllFolderTruncated(db *leveldb.DB, folder []byte, fn func(device []b
 	}
 }
 
-func ldbGet(db *leveldb.DB, folder, device, file []byte) (protocol.FileInfo, bool) {
+func ldbGet(db dbReader, folder, device, file []byte) (protocol.FileInfo, bool) {
 	nk := deviceKey(folder, device, file)
 	bs, err := db.Get(nk, nil)
 	if err == leveldb.ErrNotFound {

test/conflict_test.go

@@ -9,6 +9,7 @@
 package integration
 
 import (
+	"bytes"
 	"io/ioutil"
 	"log"
 	"os"
@@ -156,15 +157,23 @@ func TestConflictsDefault(t *testing.T) {
 	}
 	rc.AwaitSync("default", sender, receiver)
 
-	// The conflict should manifest on the s2 side again, where we should have
-	// moved the file to a conflict copy instead of just deleting it.
+	// The conflict is resolved to the advantage of the edit over the delete.
+	// As such, we get the edited content synced back to s1 where it was
+	// removed.
 
 	files, err = osutil.Glob("s2/*sync-conflict*")
 	if err != nil {
 		t.Fatal(err)
 	}
-	if len(files) != 2 {
-		t.Errorf("Expected 2 conflicted files instead of %d", len(files))
+	if len(files) != 1 {
+		t.Errorf("Expected 1 conflicted files instead of %d", len(files))
+	}
+	bs, err := ioutil.ReadFile("s1/testfile.txt")
+	if err != nil {
+		t.Error("reading file:", err)
+	}
+	if !bytes.Contains(bs, []byte("more text added to s2")) {
+		t.Error("s1/testfile.txt should contain data added in s2")
 	}
 }