-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lib/model: Create root directory for paused folders (fixes #4094) #4095
Conversation
lib/model/model.go
Outdated
l.Warnln("Creating folder:", err) | ||
} | ||
} | ||
createRootFolder(cfg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved off the real computer for the evening so I'm not gong to test tonight, but I can bitch about the naming :D it's a folder root, not a root folder. ;)
Lgtm |
Sorry for letting stew. I want to test this (and the other PR) a bit before merging, and haven't had time to do so yet.
|
This looks sane and probably works as such, but it doesn't solve the underlying problem. That is, if I
the folder gets added, the directory is created, no error is shown, but no ignore patterns are created in the folder |
Friendly reminder here just to make sure it's not forgotten or anything :) |
Not forgotten, but off skitouring = offline. |
Ah, cool, have fun. Let me see if I can narrow it down and fix it.
|
Found the issue, there is a race condition in the javscript which I'll fix in a followup commit. |
@st-review merge |
Given the saveConfig() is async, it might not have happened before we try to save the ignores and unpause. Likewise must wait for saving ignores before unpausing or the scan might start before ignores are on disk. Javsacript <3
@@ -815,6 +815,7 @@ func syncthingMain(runtimeOptions RuntimeOptions) { | |||
// Add and start folders | |||
for _, folderCfg := range cfg.Folders() { | |||
if folderCfg.Paused { | |||
folderCfg.CreateRoot() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@imsodin Happened to be looking back at this... This is a problem if we're starting up, a folder is paused, and the folder is unmounted or otherwise missing. We will then create a new, empty, root for it. If the folder is then unpaused, we'll assume it became empty and sync a complete delete of it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@calmh I seem to remember that this came up before but was considered not a problem, as the missing folder marker prevents it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CreateRoot does not create the marker though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't, that's done by cfg.CreateMarker
.
see #4094
One possible caveat: Paused folders in config.xml are not handled (e.g. https://github.com/syncthing/syncthing/blob/master/cmd/syncthing/main.go#L817), but possibly other places too (?).