enable support of folders and files with colon in name on Unix#4959
enable support of folders and files with colon in name on Unix#4959mirichmo merged 4 commits intoPowerShell:masterfrom
Conversation
There was a problem hiding this comment.
This is inefficient to do unconditionally for an uncommon case, but it also doesn't fully fix the problem. Is it not possible to check if it looks like a drive, and if not, then it moist be a file? There are downsides to that approach though, so if it was rejected, I think it's worth noting why in the code.
There was a problem hiding this comment.
The problem with the current code is that it always checks for the colon you see above and if it's there, it assumes it's a drive. What I'm doing here is what you are saying. If the colon is after a path separator, it doesn't look like a drive. If you mean to see if the left of the colon is a drive, wouldn't that be a bigger perf hit? I could certainly only look for the separator only if there is a colon.
There was a problem hiding this comment.
With this change this still won't work:
1+1 > foo:bar.txtinstead you have to do:
1+1 > ./foo:bar.txtWhich seems fine to me
There was a problem hiding this comment.
At a minimum, yes, only look for the separator if there is a colon.
But the inconsistency of requiring a separator only if there is a colon - that seems silly and not easy to discover.
I guess I was suggesting to assume it's a drive (maybe not if there is a separator) because we're doing that already, and if it's not a drive, then don't error out, treat it as a filesystem path.
Of course this does introduce an ambiguity:
Set-Content -Path env:abc -Value 123This sets the environment variable, not the file content. In cases like this, you'd have to use a separator. This example might seem not interesting, but something like this is more problematic:
Set-Content -Path "${prefix}:${suffix}" -Value 123There was a problem hiding this comment.
Personally, I like having the distinction between a drive and a file with the separator.
92b947a to
6394373
Compare
|
CI failures due to #4958 |
6394373 to
5a62062
Compare
|
@lzybkr are you ok with the current changes? would like to get it in for beta.8 if possible |
| // We must have a drive specified | ||
|
|
||
| result = true; | ||
| int separator = path.IndexOf(StringLiterals.DefaultPathSeparator, 0, index); |
There was a problem hiding this comment.
Based on the documentation this does not look correct to me. It searches the string starting from the beginning though index count of characters for the matching separator. "C:\something.txt" Will always fail because the separator never matches the first character of the string ("C").
I think you might need:
separator = path.IndexOf(StringLiterals.AlternatePathSeparator, index, 1); This checks the character immediately after the index. But if you are going to do that, you might as well directly test index+1.
There was a problem hiding this comment.
index points to location of the colon. So this test is to determine if there is a path separator between the beginning and the colon. So it should match ./foo:bar but not match foo:bar where the former is a file/directory and the latter is a drive path. One optimization I can see is I only need to search up to index-1 since I already know there is a colon at index.
3a062d8 to
000a29c
Compare
|
@mirichmo can you review again? |
allow colons if it shows up after a path separator
fix #4889