Skip to content

Conversation

@SteveL-MSFT
Copy link
Member

allow colons if it shows up after a path separator

fix #4889

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

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.txt

instead you have to do:

1+1 > ./foo:bar.txt

Which seems fine to me

Copy link
Contributor

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 123

This 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 123

Copy link
Member Author

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.

@SteveL-MSFT
Copy link
Member Author

CI failures due to #4958

support folders and files with colon in name
only check separator in relation to colon if a colon is found
@SteveL-MSFT
Copy link
Member Author

@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);
Copy link
Member

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.

Copy link
Member Author

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.

added comment to clarify algorithm
@SteveL-MSFT
Copy link
Member Author

@mirichmo can you review again?

@mirichmo mirichmo merged commit 50607b9 into PowerShell:master Oct 6, 2017
@SteveL-MSFT SteveL-MSFT deleted the colon-path branch November 12, 2017 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Linux: cd and find commands fail when a colon is in the directory or file name

3 participants