-
Notifications
You must be signed in to change notification settings - Fork 380
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
[bugfix] Windows paths don’t work with Server #445
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -288,9 +288,9 @@ func cleanPath(p string) string { | |
} | ||
|
||
func cleanPathWithBase(base, p string) string { | ||
p = filepath.ToSlash(p) | ||
p = filepath.ToSlash(filepath.Clean(p)) | ||
if !path.IsAbs(p) { | ||
return path.Join(base, p) | ||
} | ||
return path.Clean(p) | ||
return p | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did some basic tests and all worked, anyway since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm… the point here in removing this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we could have the same issue if the path is not absolute and we hit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, but this should only apply in the case where the base being added is a UNC address. So, for example I think in this case, we’re kind of a bit stuck on “having a non-default base paths is not particularly robust and a little bit of a hack”. 🤔 For this particular case, I’m a bit more willing to let the inconsistent behavior slide. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Allowing non cleaned paths could break some usages, let's merge this patch and see if someone complaints. I'll try to do some tests myself using wsl (I never tested it) as soon as I have some free time, it could take a while There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, this is why I think keeping the It’s at least pretty nice that we can do UNC paths with a “local” datastore with WSL, if I had to setup a remote SMB server to test any of this, I might very well tear my hair out. 😆 |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,3 +21,7 @@ func testOsSys(sys interface{}) error { | |
} | ||
return nil | ||
} | ||
|
||
func toLocalPath(p string) string { | ||
return p | ||
} |
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.
The order here really doesn’t matter except for Windows, and only for UNC paths. “Let’s just not support UNC paths“ doesn’t work, because OpenSSH’s client does not sanitize the path first, so UNC paths would still work with that client anyways. 😭
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.
🤷♀️ alright.