Skip to content
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: Set UseLargeBlocks to true by default (fixes #5599) #5600

Merged
merged 1 commit into from
Mar 12, 2019

Conversation

imsodin
Copy link
Member

@imsodin imsodin commented Mar 12, 2019

Fixes #5599, followup to #5405.

@calmh
Copy link
Member

calmh commented Mar 12, 2019

It's unclear to me what this would accomplish. In the issue (forum link) it talks about the API. If a config comes in through the API it's expected to be complete. I'm pretty sure our setdefaults stuff doesn't take effect at all on that path. Maybe they could, but it seems tricky. We'd have to instantiate a corresponding object, then run setdefaults on it, then deserialize the JSON on top, to have missing attributes get a default value... In practice I guess you might get away with deserializing the config twice, with a setdefaults inbetween...

@imsodin
Copy link
Member Author

imsodin commented Mar 12, 2019

As I understood it, the thread in the forum talks about the default folder, which is generated in main.go through NewFolderconfiguration where defaults do apply.

Edit: I have to admit though, I did do no testing/reproducing on this at all, it seemed self-evident to me.

Copy link
Member

@AudriusButkevicius AudriusButkevicius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any way, this is the right thing to do even if it does not address the bigger fear of what @calmh suggested. Regardless, we want them to be on by default on the default folder. API problem is a separate problem.

Perhaps this should have been disableLargeBlocks which would then do the right thing when adding via API, but I guess would have broken backwards compatibility

@AudriusButkevicius AudriusButkevicius merged commit 50d8c43 into syncthing:master Mar 12, 2019
@calmh
Copy link
Member

calmh commented Mar 12, 2019

Right if this is for the default folder then I understand.

@calmh calmh added this to the v1.1.1 milestone Mar 12, 2019
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Mar 12, 2020
@syncthing syncthing locked and limited conversation to collaborators Mar 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default folder in v1.1.0 isn’t created with useLargeBlocks = true
4 participants