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, lib/fs: Make junction behaviour configurable (ref #6606) #6907

Merged
merged 6 commits into from Aug 19, 2020

Conversation

imsodin
Copy link
Member

@imsodin imsodin commented Aug 19, 2020

@AudriusButkevicius
Copy link
Member

I think we do a config migration of this, setting to true on all existing folders, and defaulting to false.

@imsodin
Copy link
Member Author

imsodin commented Aug 19, 2020

Passing fs options as url-like query parameters might not be the best idea: A filesystem path does not necessarily conform to what url.Parse expects. An alternative is adding the usual ...opts Option thing to NewFilesystem and then have WithJunctionsAsDirs() Option, though I believe Audrius has a bit of a disliking to that pattern (?).

@AudriusButkevicius
Copy link
Member

I don't mind options, I mind functions thrown around like they are variables, yet I think query string stuff is the right way to go here.

@imsodin
Copy link
Member Author

imsodin commented Aug 19, 2020

Well these options are functions disguised as options ;)

Anyway my problem is that almost anything is allowed in paths, i.e. you need to do parsing yourself (url.Parse fails). At which point I'd rather not do the detour of encoding options into a pseudo-uri and then decoding the same options from that pseudo-uri, but pass them directly.

@AudriusButkevicius
Copy link
Member

so my original idea was to turn C:\foo\bar into file://c/foo/bar, but it never happened. If it did happen, this would be much easier.

This also would have allowed getting rid fo the type, as the schema dictates the type.

@imsodin
Copy link
Member Author

imsodin commented Aug 19, 2020

That makes sense, but being an unrelated change and quite possible to turn up weird cornercases, I used options, It's still easy enough to switch to "all uri initialisation" from here.

@@ -23,13 +23,20 @@ var (
ErrNotRelative = errors.New("not a relative path")
)

func WithJunctionsAsDirs() Option {
return func(fs Filesystem) {
fs.(*BasicFilesystem).junctionsAsDirs = true
Copy link
Member

Choose a reason for hiding this comment

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

Panic if I ask for a fakefs with junctionsAsDirs?

@calmh calmh merged commit fc2c46e into syncthing:main Aug 19, 2020
@imsodin imsodin deleted the lib/junctionOption branch August 19, 2020 18:18
@calmh calmh added this to the v1.9.0 milestone Aug 20, 2020
@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 Aug 20, 2021
@syncthing syncthing locked and limited conversation to collaborators Aug 20, 2021
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.

None yet

4 participants