From eb2ffabc0eb6f20d58a23503906943105af5a7e2 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Fri, 8 Dec 2017 13:48:11 -0800 Subject: [PATCH 1/5] filepath/clean: Handle 'a/..' -> '.' case This catches us up with how the Linux stdlib implementation handles this case: $ cat test.go package main import ( "fmt" "path/filepath" ) func main() { fmt.Println(filepath.Clean("a/..")) } $ go run test . by implementing the rule mentioned in [1]. [1]: https://golang.org/pkg/path/filepath/#Clean Signed-off-by: W. Trevor King --- filepath/clean.go | 7 +++++++ filepath/clean_test.go | 5 +++++ 2 files changed, 12 insertions(+) diff --git a/filepath/clean.go b/filepath/clean.go index b70c575f2..6d733e3b6 100644 --- a/filepath/clean.go +++ b/filepath/clean.go @@ -49,6 +49,13 @@ func Clean(os, path string) string { if abs { cleaned = fmt.Sprintf("%c%s", sep, cleaned) } + + // If the result of this process is an empty string, Clean returns + // the string ".". + if len(cleaned) == 0 { + cleaned = "." + } + if cleaned == path { return path } diff --git a/filepath/clean_test.go b/filepath/clean_test.go index b65e7c096..b4be2c181 100644 --- a/filepath/clean_test.go +++ b/filepath/clean_test.go @@ -56,6 +56,11 @@ func TestClean(t *testing.T) { path: "a/../b", expected: "b", }, + { + os: "linux", + path: "a/..", + expected: ".", + }, } { t.Run( fmt.Sprintf("Clean(%q,%q)", test.os, test.path), From f2e8be2130fabc6fd583838ca2e88cf8f00b1077 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Fri, 8 Dec 2017 13:56:17 -0800 Subject: [PATCH 2/5] filepath/clean: Avoid a panic on abs-path with trailing .. And add tests to protect us from that in the future. Also throw in a /.. test with a non-.. tail for good measure. Signed-off-by: W. Trevor King --- filepath/clean.go | 4 ++-- filepath/clean_test.go | 10 ++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/filepath/clean.go b/filepath/clean.go index 6d733e3b6..4e90260ce 100644 --- a/filepath/clean.go +++ b/filepath/clean.go @@ -39,8 +39,8 @@ func Clean(os, path string) string { // Eliminate .. elements that begin a rooted path: // that is, replace "/.." by "/" at the beginning of a path, // assuming Separator is '/'. - if abs && len(elements) > 0 { - for elements[0] == ".." { + if abs { + for len(elements) > 0 && elements[0] == ".." { elements = elements[1:] } } diff --git a/filepath/clean_test.go b/filepath/clean_test.go index b4be2c181..cc9c3a39f 100644 --- a/filepath/clean_test.go +++ b/filepath/clean_test.go @@ -36,6 +36,16 @@ func TestClean(t *testing.T) { path: "//a", expected: "/a", }, + { + os: "linux", + path: "/..", + expected: "/", + }, + { + os: "linux", + path: "/../a", + expected: "/a", + }, { os: "linux", path: ".", From 60df7680c148ab72189e3988946eae0a11fddb55 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Fri, 8 Dec 2017 14:01:47 -0800 Subject: [PATCH 3/5] filepath/clean_test: Compare with the standard library For the test cases that match the current GOOS. This ensures we stay consistent (at least for platforms where we run tests). Also drop an unecessary trailing newline from the old error message. Signed-off-by: W. Trevor King --- filepath/clean_test.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/filepath/clean_test.go b/filepath/clean_test.go index cc9c3a39f..b74ea3b10 100644 --- a/filepath/clean_test.go +++ b/filepath/clean_test.go @@ -2,6 +2,8 @@ package filepath import ( "fmt" + "path/filepath" + "runtime" "testing" ) @@ -77,7 +79,13 @@ func TestClean(t *testing.T) { func(t *testing.T) { clean := Clean(test.os, test.path) if clean != test.expected { - t.Errorf("unexpected result: %q (expected %q)\n", clean, test.expected) + t.Errorf("unexpected result: %q (expected %q)", clean, test.expected) + } + if runtime.GOOS == test.os { + stdClean := filepath.Clean(test.path) + if clean != stdClean { + t.Errorf("non-standard result: %q (%q is standard)", clean, stdClean) + } } }, ) From 17ce13aa745df724a1addac95ab07de56580f22f Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Fri, 8 Dec 2017 14:03:30 -0800 Subject: [PATCH 4/5] filepath/abs_test: Compare IsAbs with the standard library For the test cases that match the current GOOS. This ensures we stay consistent (at least for platforms where we run tests). Also drop an unecessary trailing newline from the old error message. I don't compare for Abs, because the stdlib doesn't have a configurable cwd. We could check the current working directory [1,2,...], but I don't want to use the frozen syscall package and am not interested enough to import lots of per-OS x/sys packages. Also, the odds of the test working directory matching one of the cwd values (or even of those cwd values actually existing on the host) are small, and it would be even more work to temporarily change to (and possibly create) those directories per test. [1]: https://golang.org/pkg/syscall/#Getcwd [2]: https://godoc.org/golang.org/x/sys/unix#Getcwd Signed-off-by: W. Trevor King --- filepath/abs_test.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/filepath/abs_test.go b/filepath/abs_test.go index 7e48c5fa2..439d97cf6 100644 --- a/filepath/abs_test.go +++ b/filepath/abs_test.go @@ -2,6 +2,8 @@ package filepath import ( "fmt" + "path/filepath" + "runtime" "testing" ) @@ -74,7 +76,7 @@ func TestAbs(t *testing.T) { if err != nil { t.Error(err) } else if abs != test.expected { - t.Errorf("unexpected result: %q (expected %q)\n", abs, test.expected) + t.Errorf("unexpected result: %q (expected %q)", abs, test.expected) } }, ) @@ -163,7 +165,13 @@ func TestIsAbs(t *testing.T) { func(t *testing.T) { abs := IsAbs(test.os, test.path) if abs != test.expected { - t.Errorf("unexpected result: %t (expected %t)\n", abs, test.expected) + t.Errorf("unexpected result: %t (expected %t)", abs, test.expected) + } + if runtime.GOOS == test.os { + stdAbs := filepath.IsAbs(test.path) + if abs != stdAbs { + t.Errorf("non-standard result: %t (%t is standard)", abs, stdAbs) + } } }, ) From 2f211800cc259db1a439334d514489842f69a176 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Fri, 8 Dec 2017 14:15:34 -0800 Subject: [PATCH 5/5] filepath/clean: Add Windows support And once we have Windows support for Clean, we can remove the guard from Abs. I don't have a Windows machine around to test with, so the expected values are my best guesses. Signed-off-by: W. Trevor King --- filepath/abs.go | 4 -- filepath/abs_test.go | 54 +++++++++++++++++++++ filepath/ancestor_test.go | 98 +++++++++++++++++++++++++++++++++++++++ filepath/clean.go | 17 +++++-- filepath/clean_test.go | 60 ++++++++++++++++++++++++ 5 files changed, 226 insertions(+), 7 deletions(-) diff --git a/filepath/abs.go b/filepath/abs.go index c19bba26a..e4ab7453f 100644 --- a/filepath/abs.go +++ b/filepath/abs.go @@ -1,7 +1,6 @@ package filepath import ( - "errors" "regexp" "strings" ) @@ -11,9 +10,6 @@ var windowsAbs = regexp.MustCompile(`^[a-zA-Z]:\\.*$`) // Abs is a version of path/filepath's Abs with an explicit operating // system and current working directory. func Abs(os, path, cwd string) (_ string, err error) { - if os == "windows" { - return "", errors.New("Abs() does not support windows yet") - } if IsAbs(os, path) { return Clean(os, path), nil } diff --git a/filepath/abs_test.go b/filepath/abs_test.go index 439d97cf6..a06135838 100644 --- a/filepath/abs_test.go +++ b/filepath/abs_test.go @@ -68,6 +68,60 @@ func TestAbs(t *testing.T) { cwd: "/cwd", expected: "/b", }, + { + os: "windows", + path: "c:\\", + cwd: "/cwd", + expected: "c:\\", + }, + { + os: "windows", + path: "c:\\a", + cwd: "c:\\cwd", + expected: "c:\\a", + }, + { + os: "windows", + path: "c:\\a\\", + cwd: "c:\\cwd", + expected: "c:\\a", + }, + { + os: "windows", + path: "c:\\\\a", + cwd: "c:\\cwd", + expected: "c:\\a", + }, + { + os: "windows", + path: ".", + cwd: "c:\\cwd", + expected: "c:\\cwd", + }, + { + os: "windows", + path: ".\\c", + cwd: "c:\\a\\b", + expected: "c:\\a\\b\\c", + }, + { + os: "windows", + path: ".\\\\c", + cwd: "c:\\a\\b", + expected: "c:\\a\\b\\c", + }, + { + os: "windows", + path: "..\\a", + cwd: "c:\\cwd", + expected: "c:\\a", + }, + { + os: "windows", + path: "..\\..\\b", + cwd: "c:\\cwd", + expected: "c:\\b", + }, } { t.Run( fmt.Sprintf("Abs(%q,%q,%q)", test.os, test.path, test.cwd), diff --git a/filepath/ancestor_test.go b/filepath/ancestor_test.go index bcbe8345a..4a9117692 100644 --- a/filepath/ancestor_test.go +++ b/filepath/ancestor_test.go @@ -97,6 +97,104 @@ func TestIsAncestor(t *testing.T) { cwd: "/cwd", expected: true, }, + { + os: "windows", + pathA: "c:\\", + pathB: "c:\\a", + cwd: "c:\\cwd", + expected: true, + }, + { + os: "windows", + pathA: "c:\\", + pathB: "d:\\a", + cwd: "c:\\cwd", + expected: false, + }, + { + os: "windows", + pathA: "c:\\", + pathB: ".", + cwd: "d:\\cwd", + expected: false, + }, + { + os: "windows", + pathA: "c:\\a", + pathB: "c:\\a", + cwd: "c:\\cwd", + expected: false, + }, + { + os: "windows", + pathA: "c:\\a", + pathB: "c:\\", + cwd: "c:\\cwd", + expected: false, + }, + { + os: "windows", + pathA: "c:\\a", + pathB: "c:\\ab", + cwd: "c:\\cwd", + expected: false, + }, + { + os: "windows", + pathA: "c:\\a\\", + pathB: "c:\\a", + cwd: "c:\\cwd", + expected: false, + }, + { + os: "windows", + pathA: "c:\\\\a", + pathB: "c:\\a", + cwd: "c:\\cwd", + expected: false, + }, + { + os: "windows", + pathA: "c:\\\\a", + pathB: "c:\\a\\b", + cwd: "c:\\cwd", + expected: true, + }, + { + os: "windows", + pathA: "c:\\a", + pathB: "c:\\a\\", + cwd: "c:\\cwd", + expected: false, + }, + { + os: "windows", + pathA: "c:\\a", + pathB: ".", + cwd: "c:\\cwd", + expected: false, + }, + { + os: "windows", + pathA: "c:\\a", + pathB: "b", + cwd: "c:\\a", + expected: true, + }, + { + os: "windows", + pathA: "c:\\a", + pathB: "..\\a", + cwd: "c:\\cwd", + expected: false, + }, + { + os: "windows", + pathA: "c:\\a", + pathB: "..\\a\\b", + cwd: "c:\\cwd", + expected: true, + }, } { t.Run( fmt.Sprintf("IsAncestor(%q,%q,%q,%q)", test.os, test.pathA, test.pathB, test.cwd), diff --git a/filepath/clean.go b/filepath/clean.go index 4e90260ce..d5dd65ae1 100644 --- a/filepath/clean.go +++ b/filepath/clean.go @@ -30,6 +30,9 @@ func Clean(os, path string) string { // Eliminate each inner .. path name element (the parent directory) // along with the non-.. element that precedes it. for i := 1; i < len(elements); i++ { + if i == 1 && abs && sep == '\\' { + continue + } if i > 0 && elements[i] == ".." { elements = append(elements[:i-1], elements[i+1:]...) i -= 2 @@ -39,15 +42,23 @@ func Clean(os, path string) string { // Eliminate .. elements that begin a rooted path: // that is, replace "/.." by "/" at the beginning of a path, // assuming Separator is '/'. + offset := 0 + if sep == '\\' { + offset = 1 + } if abs { - for len(elements) > 0 && elements[0] == ".." { - elements = elements[1:] + for len(elements) > offset && elements[offset] == ".." { + elements = append(elements[:offset], elements[offset+1:]...) } } cleaned := strings.Join(elements, string(sep)) if abs { - cleaned = fmt.Sprintf("%c%s", sep, cleaned) + if sep == '/' { + cleaned = fmt.Sprintf("%c%s", sep, cleaned) + } else if len(elements) == 1 { + cleaned = fmt.Sprintf("%s%c", cleaned, sep) + } } // If the result of this process is an empty string, Clean returns diff --git a/filepath/clean_test.go b/filepath/clean_test.go index b74ea3b10..40cbaea75 100644 --- a/filepath/clean_test.go +++ b/filepath/clean_test.go @@ -73,6 +73,66 @@ func TestClean(t *testing.T) { path: "a/..", expected: ".", }, + { + os: "windows", + path: "c:\\", + expected: "c:\\", + }, + { + os: "windows", + path: "c:\\\\", + expected: "c:\\", + }, + { + os: "windows", + path: "c:\\a", + expected: "c:\\a", + }, + { + os: "windows", + path: "c:\\a\\", + expected: "c:\\a", + }, + { + os: "windows", + path: "c:\\\\a", + expected: "c:\\a", + }, + { + os: "windows", + path: "c:\\..", + expected: "c:\\", + }, + { + os: "windows", + path: "c:\\..\\a", + expected: "c:\\a", + }, + { + os: "windows", + path: ".", + expected: ".", + }, + { + os: "windows", + path: ".\\c", + expected: "c", + }, + { + os: "windows", + path: "..\\.\\a", + expected: "..\\a", + }, + { + os: "windows", + path: "a\\..\\b", + expected: "b", + }, + { + os: "windows", + path: "a\\..", + expected: ".", + }, } { t.Run( fmt.Sprintf("Clean(%q,%q)", test.os, test.path),