lib/config: Support symlinked root (fixes #4542, fixes #4353) #4545
Conversation
|
Also, interestingly: Is it a regular: No |
lib/config/folderconfiguration.go
Outdated
|
|
||
| // Users might have the root directory as a symlink or reparse point. | ||
| // Furthermore, OneDrive bullcrap uses a magic reparse point to the cloudz... | ||
| if !fi.IsDir() && !fi.IsSymlink() { | ||
| return errPathMissing |
There was a problem hiding this comment.
Given that there is now a separate check I think it would be nice to have an errPathNotDir or something that is more descriptive than "missing", as users will not consider it missing.
Maybe even check if the first error os.IsNotExist and only then return errPathMissing, otherwise the error as is? In the name of getting a possibly more concrete and specific error in front of the user.
Oh, and how about a test for this? Just something that creates a directory, creates a symlink to it, and verifies that checkpath on the symlink succeeds. If goos=="windows" and the symlink create fails, t.Skip
lib/config/config_test.go
Outdated
| @@ -430,6 +431,76 @@ func TestFolderPath(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestFolderCheckPath(t *testing.T) { | |||
| if runtime.GOOS == "windows" { | |||
| t.Skip("windows") | |||
There was a problem hiding this comment.
The test should run on Windows, this already worked afaik on other platforms. The build server runs in developer mode and we should be able to create symlinks just fine, so this test should pass. Not everywhere is the same of course which is why I suggest a t.Skip on error from os.Symlink
There was a problem hiding this comment.
... in theory. I tested it now, and while I can mklink just fine from a command prompt without any elevation I still get symlink foo ~windows-symlink-test-8366981916958631613: A required privilege is not held by the client. from tests which is annoying. I even rebooted it for good measure with no effect.
There was a problem hiding this comment.
Ah. Not only must symlinks be enabled for normal users, the application must also say "please let me create a symlink even though I'm just a normal process", using the SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE flag in the symlink call. That's so fucking stupid.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa363866(v=vs.85).aspx
There was a problem hiding this comment.
So I guess this is fine, once working, I filed the other one on Go. Maybe you want to contribute the fix there. :)
|
With #4548 merged, this test could run on Windows as well. |
AudriusButkevicius
force-pushed
the
reparse
branch
from
060a091
to
a1be55c
Compare
|
@st-review lgtm |
|
@imsodin: Noted! Need another LGTM or explicit merge command. |
|
@st-review lgtm |
|
👌 Merged as 95a65bf. Thanks, @AudriusButkevicius! |
st-review
added
the
pr-merged
st-review
added
the
frozen-due-to-age
Well Tested (TM)