lib/scanner: Prevent sync-conflict for receive-only local modifications #9323
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jul13579
commented
Jan 5, 2024
Comment on lines
-419
to
+426
| 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. |
Contributor
Author
There was a problem hiding this comment.
During my debugging session I could only see the issue happening during walkRegular. However I found similar checks in walkDir and walkSymlink, which I applied the same changes to - just for good measure.
0bf76a8 to
49032ec
Compare
imsodin
reviewed
Jan 8, 2024
Member
imsodin
left a comment
There was a problem hiding this comment.
Thanks, nice catch!
One tiny nit about a test comment, otherwise lgtm.
Co-authored-by: Simon Frei <freisim93@gmail.com>
imsodin
reviewed
Jan 8, 2024
imsodin
approved these changes
Jan 8, 2024
calmh
added a commit
to calmh/syncthing
that referenced
this pull request
Jan 14, 2024
* main: (24 commits) lib/ignore: Refactor out result type (syncthing#9343) build: Testing infra images for infra-* branches lib/versioner: Expand tildes in version directory (fixes syncthing#9241) (syncthing#9327) lib/scanner: Prevent sync-conflict for receive-only local modifications (syncthing#9323) gui, man, authors: Update docs, translations, and contributors Fix website security link in README.md (syncthing#9325) cmd/syncthing: Add CLI completion functionality (fixes syncthing#8616) (syncthing#9226) lib/api: Save session & CSRF tokens to database, add option to stay logged in (fixes syncthing#9151) (syncthing#9284) Update dependencies (syncthing#9321) gui: Always inform about loading data in Restore Versions modal (syncthing#9317) lib/build: Allow semver build in version regex (fixes syncthing#9267) (syncthing#9316) gui: Keep short deviceID length consistent + xrefs (fixes syncthing#9313) (syncthing#9314) build(deps): bump actions/download-artifact from 3 to 4 (syncthing#9294) build(deps): bump actions/upload-artifact from 3 to 4 (syncthing#9293) gui, man, authors: Update docs, translations, and contributors gui, lib/scanner: Improve scan progress indication (ref syncthing#8331) (syncthing#9308) lib/protocol: handle empty names in unixOwnershipEqual (fixes syncthing#9039) (syncthing#9306) gui, man, authors: Update docs, translations, and contributors etc/linux-desktop: use double dash for long options (syncthing#9301) lib/connections: Skip allocation in check for missing port (syncthing#9297) ...
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
This PR changes behaviour of syncthing related to
receive-onlyfolders, which I believe to be a bug since I wouldn't expect the current behaviour. With the current syncthing codebase, a file of areceive-onlyfolder that is only modified locally can cause the creation of a.sync-conflictfile.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 bereceive-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-conflictfile that contains the modified contents, while the regularTest.txtfile is reset to the cluster's provided contents. This is due to aprotocol.FileInfo#ShouldConflictcheck, 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!