Correctly normalize Windows package paths. - #392
Conversation
When the name passed to util.NormalizeName contains backslashes from a Windows filepath, the backslashes must be first converted to forward slashes (used by all package names, including on Windows). This fixes the strings.TrimPrefix call used to strip the repo root from the subpackage. As a result of this, subpackages in lockfiles are written identically on Windows as other Unix platforms and no duplicate subpackages are introduced. Fixes Masterminds#389.
| } | ||
|
|
||
| name = filepath.ToSlash(name) | ||
| root := GetRootFromPackage(name) |
There was a problem hiding this comment.
What do we get from this? The first thing GetRootFromPackage does is run filepath.ToSlash. GetRootFromPackage is sometimes called outside NormalizeName.
Is there a problem in the latest code that needs to be addressed?
There was a problem hiding this comment.
The issue is that name contains backslashes. The root is normalized, but the full name is not, so the prefix trim doesn't work as intended.
With github.com\foo\bar\baz as input:
// name == "github.com\foo\bar\baz"
root := GetRootFromPackage(name) // root == "github.com/foo/bar"
extra := strings.TrimPrefix(name, root) // extra == "github.com\foo\bar\baz"
There was a problem hiding this comment.
I see the bug I introduced now.
|
Oops, intended to add a testcase for this and it fails because it just concats the root + extra, which no longer matches the initial input. Fixing shortly. |
|
So, this is entirely a micro-optimization but... Since the root name is already being normalized then the extra is the only part that needs to have it done. Can that happen here instead? |
Since both return values are now checked explicitly, remove check that the concatinated package path matches the original input, since it won't always (the point of this function is to normalize input).
|
No. That will convert the backslashes to forward slashes, but the returned subpackage will still contain the full root since the TrimPrefix just above it did not remove the root. |
|
I'm not sure why it's failing on Travis now. Passes locally. |
|
Oh.. I see. From filepath/path.go:
What do you want to do in this case? Should glide replace uses of |
|
Maybe we need to tweak the use of |
|
Looks like we could just use that instead of Suggestions on how to use it though? it's unexported in another package. Copy it into util? |
This one will replace backslashes with forward slashes on all platforms. If there exists a platform where os.PathSeparator is not a forward or backwards slash, also replace those.
| v = strings.Replace(v, "\\", "/", -1) | ||
| if os.PathSeparator != '\\' && os.PathSeparator != '/' { | ||
| v = strings.Replace(v, string(os.PathSeparator), "/", -1) | ||
| } |
There was a problem hiding this comment.
Is there an operating system with the path separator other than / or \ that Go supports? Or that's in wide use? If so that's a separate issue because other things will fail.
There was a problem hiding this comment.
Not to my knowledge. Should this only replace backslashes then?
There was a problem hiding this comment.
I think using v = strings.Replace(v, "\\", "/", -1) should be enough.
|
@jrick thanks for the contribution! |
2 participants
When the name passed to util.NormalizeName contains backslashes from a
Windows filepath, the backslashes must be first converted to forward
slashes (used by all package names, including on Windows). This fixes
the strings.TrimPrefix call used to strip the repo root from the
subpackage.
As a result of this, subpackages in lockfiles are written identically
on Windows as other Unix platforms and no duplicate subpackages are
introduced.
Fixes #389.