-
Notifications
You must be signed in to change notification settings - Fork 275
Add better handling of windows-style paths for io_binary #923
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
Add better handling of windows-style paths for io_binary #923
Conversation
86573c4 to
03e0c1f
Compare
internal/cmd/io_binary.go
Outdated
| } | ||
|
|
||
| // Strip any leading '\\' if the path contains a hard drive in it. | ||
| if strings.Contains(sanitized, ":\\") { |
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.
I'm confused about this section. Could :\\ be anywhere in the path?
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.
like if we have multiple volumes, e.g. C:\path\to\something and D:\path\to\something. or am I missing something?
internal/cmd/io_binary.go
Outdated
| if uri.Host != "" { | ||
| return "\\" + uri.Host + sanitized | ||
| } |
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.
Is it possible that uri.Host could already have a leading \\ like in the hard drive case above?
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.
url.Parse will complain about back slashes in hostname, so things should fail before we even get here.
03e0c1f to
54adb4d
Compare
kevpar
left a comment
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.
Left some comments
8e5e6b5 to
f2e1d8c
Compare
f2e1d8c to
91416a5
Compare
Signed-off-by: Maksim An <[email protected]>
91416a5 to
bbae6da
Compare
kevpar
left a comment
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.
LGTM
Related work items: microsoft#173, microsoft#839, microsoft#856, microsoft#877, microsoft#881, microsoft#886, microsoft#887, microsoft#888, microsoft#889, microsoft#890, microsoft#893, microsoft#894, microsoft#896, microsoft#899, microsoft#900, microsoft#902, microsoft#904, microsoft#905, microsoft#906, microsoft#907, microsoft#908, microsoft#910, microsoft#912, microsoft#913, microsoft#914, microsoft#916, microsoft#918, microsoft#923, microsoft#925, microsoft#926, microsoft#928, microsoft#929, microsoft#932, microsoft#933, microsoft#934, microsoft#938, microsoft#939, microsoft#942, microsoft#943, microsoft#945, microsoft#946, microsoft#947, microsoft#949, microsoft#951, microsoft#952, microsoft#954
Signed-off-by: Maksim An <[email protected]>
Do a better job at cleaning the logging driver path.
Add support for non-default hard drives.