-
Notifications
You must be signed in to change notification settings - Fork 8.1k
enable support of folders and files with colon in name on Unix #4959
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
Conversation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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