diff options
author | Julian Lehrhuber <jul13579@users.noreply.github.com> | 2024-01-08 10:29:20 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-01-08 10:29:20 +0100 |
commit | 8edd67a569594bec06c395041acb7eb18f8e3b13 (patch) | |
tree | 4e03e66f6f1b42f7cb59c2bd4323b613c8c9c88e | |
parent | e829a6329587ba0d65f0a53165361c1ab165103e (diff) | |
download | syncthing-8edd67a569594bec06c395041acb7eb18f8e3b13.tar.gz syncthing-8edd67a569594bec06c395041acb7eb18f8e3b13.zip |
lib/scanner: Prevent sync-conflict for receive-only local modifications (#9323)
### Purpose
This PR changes behaviour of syncthing related to `receive-only`
folders, which I believe to be a bug since I wouldn't expect the current
behaviour. With the current syncthing codebase, a file of a
`receive-only` folder that is only modified locally can cause the
creation of a `.sync-conflict` file.
### Testing
Consider this szenario: Setup two paired clients that sync a folder with
a given file (e.g. `Test.txt`). One of the clients configures the folder
to be `receive-only`. Now, change the contents of the file for the
receive-only client **_twice_**.
With the current syncthing codebase, this leads to the creation of a
`.sync-conflict` file that contains the modified contents, while the
regular `Test.txt` file is reset to the cluster's provided contents.
This is due to a `protocol.FileInfo#ShouldConflict` check, that is
succeeding on the locally modified file.
This PR changes this behaviour to not reset the file and not cause the
creation of a `.sync-conflict`. Instead, the second content update is
treated the same as the first content update.
This PR also contains a test that fails on the current codebase and
succeeds with the changes introduced in this PR.
### Screenshots
This is not a GUI change
### Documentation
This is not a user visible change.
## Authorship
Your name and email will be added automatically to the AUTHORS file
based on the commit metadata.
#### Thanks to all the syncthing folks for this awesome piece of
software!
-rw-r--r-- | lib/model/folder_recvonly_test.go | 68 | ||||
-rw-r--r-- | lib/scanner/walk.go | 18 |
2 files changed, 80 insertions, 6 deletions
diff --git a/lib/model/folder_recvonly_test.go b/lib/model/folder_recvonly_test.go index 83c2bcd15..72383bd62 100644 --- a/lib/model/folder_recvonly_test.go +++ b/lib/model/folder_recvonly_test.go @@ -480,6 +480,74 @@ func TestRecvOnlyRevertOwnID(t *testing.T) { } } +func TestRecvOnlyLocalChangeDoesNotCauseConflict(t *testing.T) { + // Get us a model up and running + + m, f, wcfgCancel := setupROFolder(t) + defer wcfgCancel() + ffs := f.Filesystem(nil) + defer cleanupModel(m) + conn := addFakeConn(m, device1, f.ID) + + // Create some test data + + must(t, ffs.MkdirAll(".stfolder", 0o755)) + oldData := []byte("hello\n") + knownFiles := setupKnownFiles(t, ffs, oldData) + + // Send an index update for the known stuff + + must(t, m.Index(conn, "ro", knownFiles)) + f.updateLocalsFromScanning(knownFiles) + + // Scan the folder. + + must(t, m.ScanFolder("ro")) + + // Everything should be in sync. + + size := globalSize(t, m, "ro") + if size.Files != 1 || size.Directories != 1 { + t.Fatalf("Global: expected 1 file and 1 directory: %+v", size) + } + size = localSize(t, m, "ro") + if size.Files != 1 || size.Directories != 1 { + t.Fatalf("Local: expected 1 file and 1 directory: %+v", size) + } + size = needSizeLocal(t, m, "ro") + if size.Files+size.Directories > 0 { + t.Fatalf("Need: expected nothing: %+v", size) + } + size = receiveOnlyChangedSize(t, m, "ro") + if size.Files+size.Directories > 0 { + t.Fatalf("ROChanged: expected nothing: %+v", size) + } + + // Modify the file + + writeFilePerm(t, ffs, "knownDir/knownFile", []byte("change1\n"), 0o644) + + must(t, m.ScanFolder("ro")) + + size = receiveOnlyChangedSize(t, m, "ro") + if size.Files != 1 { + t.Fatalf("Receive only: expected 1 file: %+v", size) + } + + // Perform another modification. This should not cause the file to be needed. + // This is a regression test: Previously on scan the file version was changed to conflict with the global + // version, thus being needed and creating a conflict copy on next pull. + + writeFilePerm(t, ffs, "knownDir/knownFile", []byte("change2\n"), 0o644) + + must(t, m.ScanFolder("ro")) + + size = needSizeLocal(t, m, "ro") + if size.Files != 0 { + t.Fatalf("Need: expected nothing: %+v", size) + } +} + func setupKnownFiles(t *testing.T, ffs fs.Filesystem, data []byte) []protocol.FileInfo { t.Helper() diff --git a/lib/scanner/walk.go b/lib/scanner/walk.go index 074d034aa..5f4a59d23 100644 --- a/lib/scanner/walk.go +++ b/lib/scanner/walk.go @@ -426,12 +426,14 @@ func (w *walker) walkRegular(ctx context.Context, relPath string, info fs.FileIn l.Debugln(w, "unchanged:", curFile) return nil } - if curFile.ShouldConflict() { + if curFile.ShouldConflict() && !f.ShouldConflict() { // The old file was invalid for whatever reason and probably not // up to date with what was out there in the cluster. Drop all // others from the version vector to indicate that we haven't // taken their version into account, and possibly cause a - // conflict. + // conflict. However, only do this if the new file is not also + // invalid. This would indicate that the new file is not part + // of the cluster, but e.g. a local change. f.Version = f.Version.DropOthers(w.ShortID) } l.Debugln(w, "rescan:", curFile) @@ -471,12 +473,14 @@ func (w *walker) walkDir(ctx context.Context, relPath string, info fs.FileInfo, l.Debugln(w, "unchanged:", curFile) return nil } - if curFile.ShouldConflict() { + if curFile.ShouldConflict() && !f.ShouldConflict() { // The old file was invalid for whatever reason and probably not // up to date with what was out there in the cluster. Drop all // others from the version vector to indicate that we haven't // taken their version into account, and possibly cause a - // conflict. + // conflict. However, only do this if the new file is not also + // invalid. This would indicate that the new file is not part + // of the cluster, but e.g. a local change. f.Version = f.Version.DropOthers(w.ShortID) } l.Debugln(w, "rescan:", curFile) @@ -524,12 +528,14 @@ func (w *walker) walkSymlink(ctx context.Context, relPath string, info fs.FileIn l.Debugln(w, "unchanged:", curFile, info.ModTime().Unix(), info.Mode()&fs.ModePerm) return nil } - if curFile.ShouldConflict() { + if curFile.ShouldConflict() && !f.ShouldConflict() { // The old file was invalid for whatever reason and probably not // up to date with what was out there in the cluster. Drop all // others from the version vector to indicate that we haven't // taken their version into account, and possibly cause a - // conflict. + // conflict. However, only do this if the new file is not also + // invalid. This would indicate that the new file is not part + // of the cluster, but e.g. a local change. f.Version = f.Version.DropOthers(w.ShortID) } l.Debugln(w, "rescan:", curFile) |