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

cmd/syncthing, lib/model: Handle rel/abs paths for config/protected paths (Fixes #3183) #3995

Closed
wants to merge 3 commits into from

Conversation

imsodin
Copy link
Member

@imsodin imsodin commented Feb 21, 2017

This ensures that the protected paths are relative to the folder root when checking if they are ignored.

While testing I did not get the "overwriting protected files" warning as I set the config directory via -home=./config. This path is then kept in relative form and therefore isn't considered as a subfolder (model.go:283). This is fixed by calling filepath.Abs on the path given on the command line.

@calmh
Copy link
Member

calmh commented Feb 22, 2017 via email

@imsodin
Copy link
Member Author

imsodin commented Feb 22, 2017

Your use case still works, this does not require an absolute path, it converts the relative one to an absolute path, by using filepath.Abs(relpath), which essentially prepends the current working dir. Consistent absolute paths seem to be expected in parts of the code, as the string.HasPrefix checks don't work with a mixture of relative and absolute paths.

That comment entered by copy&paste from folderconfig.go, can remove that of course.

@calmh
Copy link
Member

calmh commented Mar 4, 2017

Yeah, please do. Now that I read it again it makes sense, but it discusses a performance aspect that is irrelevant when called once in main. It seems to work fine though so my other concern is a non-issue 👍

@calmh
Copy link
Member

calmh commented Mar 4, 2017

@st-review merge it

@st-review
Copy link

@calmh: Build status is pending. I'll wait until it goes green and then merge!

@st-review
Copy link

👌 Merged as c20d612. Thanks, @imsodin!

@st-review st-review closed this Mar 4, 2017
st-review pushed a commit that referenced this pull request Mar 4, 2017
@st-review st-review added the pr-merged Legacy label used in the past for pull requests merged to the main tree label Jan 15, 2018
@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 Jul 15, 2018
@syncthing syncthing locked and limited conversation to collaborators Jul 15, 2018
@imsodin imsodin deleted the protectedIgnore branch May 25, 2019 15:09
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 pr-merged Legacy label used in the past for pull requests merged to the main tree
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants