-
-
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/config: Add file inside folder marker directory #9525
Conversation
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.
Looks good code-wise. Just a few questions.
I could see more checks being done when finding the marker (e.g. checking whether the folder ID matches, maybe even record and check the device ID?). But it's absolutely OK to skip that in this first step.
func (f *FolderConfiguration) markerContents() []byte { | ||
var buf bytes.Buffer | ||
buf.WriteString("# This directory is a Syncthing folder marker.\n# Do not delete.\n\n") | ||
fmt.Fprintf(&buf, "folderID: %s\n", f.ID) |
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.
Also folderLabel
, or do you think this info is too fragile (not guaranteed to be immutable)?
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.
Yeah that, it can change and this file won't.
if build.IsWindows { | ||
// Windows has no umask so we must chose a safer set of bits to | ||
// begin with. | ||
permBits = 0o700 | ||
} |
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.
If this is no longer needed, it should be noted in the commit message. I suspect it stems from the time we transitioned from a marker file to a marker directory?
Isn't the same logic needed for the content file then?
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's based on a misunderstanding to begin with. The only thing these bits do on Windows is set the read only flag, which we don't do here. Having the defaults remove group & other write bits on unixes is best practice. I'll make a note of it.
* main: build: Generalise S3 push options gui, man, authors: Update docs, translations, and contributors lib/config: Add file inside folder marker directory (#9525) lib/config, lib/watchaggregator: Add config for max FS watcher delay (#9558) lib/fs: Watcher should react to xattr-only events on Darwin build: Update dependencies (#9553) gui, man, authors: Update docs, translations, and contributors lib/geoip, cmd/relaypoolsrv, cmd/ursrv: Automatically manage GeoIP updates (#9342) lib/db: Correct function name in comments (#9520) lib/connections: Use proper errors.Is check (#9538) build(deps): bump github.com/quic-go/quic-go from 0.42.0 to 0.43.0 (#9522)
Purpose
Avoid the issue where the folder marker is deleted by overzealous cleanup tools because it's just a useless, empty directory.
We create a small file containing a an admonishment to not delete the directory, and some metadata that is just for human consumption at the moment. (But it would parse as a valid yaml file if we wanted to read this, at some point.)
This will only apply when creating a folder marker, that is, existing setups will not gain the file automatically. Obviously, when using a custom folder marker none of this applies.
Testing
I've created and deleted a few folders and it appears to behave as I expect.
Screenshots