fix #10324 #10336
fix #10324 #10336
Conversation
|
The Appveyor failure is unrelated. |
|
Awesome, thanks! |
| @@ -130,8 +130,12 @@ function complete_path(path::AbstractString, pos) | |||
| push!(matches, id ? file * (@windows? "\\\\" : "/") : file) | |||
| end | |||
| end | |||
| matches = UTF8String[replace(s, r"\s", "\\ ") for s in matches] | |||
| return matches, nextind(path, pos - sizeof(prefix) - length(matchall(r" ", prefix))):pos, length(matches) > 0 | |||
| matches = UTF8String[replace(s, r" ", "\\ ") for s in matches] | |||
There was a problem hiding this comment.
Are you sure we don't need the \s to escape all spaces?
There was a problem hiding this comment.
yes, this line's change is actually not for fixing the bug. The reason for this is to replace space in path with "\ " so it is correctly escaped for paths and files. I have incidentally introduced "\s" instead of " " in pull #8838. It do not seem to do any difference since file names cannot contain tabs, return carriage and new line. However I think it is more correct to only match " "
There was a problem hiding this comment.
It do not seem to do any difference since file names cannot contain tabs, return carriage and new line.
posix filenames can contain any character except / (i haven't looked at the rest of this PR to see how that fits in)
There was a problem hiding this comment.
Ok. The only case the code take into account since it replaces "\s" with "\\ " but it should then be replaced like: replace(s, r"(\s)", "\\1")? I will revert this line change and then correct it in another pull because there are multiple other places that then needs to be changed.
|
Bump this can be merged, unless I should revert 2b81792 as I discuss in #10357 (comment). I think I should revert because it is not correct to replace all occurrences of a whitespace char in a filename like with an escaped space like |
This fixes #10324.