[common/system/filesystem] common ancestor fixes and more tests.
Some cases weren't well tested in the previous CL. This one adds some
tests and fixes some revealed bugs.
* slashesMatch was incorrect due to an off by one error (it would
compare [0, 12] vs [0, 12] for whichSlash==2, but really should have
compared [0, 12, 15] vs [0, 12, 19].
* We now explicitly check that all paths share the same Volume. If
they do not, then they cannot have a common ancestor.
R=rgw, vadimsh
Change-Id: I06b22e0358879f613a40ab298c8beb43b7a2cd96
Reviewed-on: https://chromium-review.googlesource.com/c/infra/luci/luci-go/+/4062996
Reviewed-by: Riley Wong <rgw@google.com>
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>
Commit-Queue: Robbie Iannucci <iannucci@chromium.org>
diff --git a/common/system/filesystem/filesystem.go b/common/system/filesystem/filesystem.go
index e6b3804..453b6a8 100644
--- a/common/system/filesystem/filesystem.go
+++ b/common/system/filesystem/filesystem.go
@@ -502,7 +502,13 @@
//
// Returns an error if any of the provided paths does not exist.
// If successful, will return a path ending with PathSeparator.
+//
+// If no paths are prodvided, returns ("", nil)
func GetCommonAncestor(paths []string, rootSentinels []string) (string, error) {
+ if len(paths) == 0 {
+ return "", nil
+ }
+
const sep = string(os.PathSeparator)
type cleanPath struct {
@@ -515,6 +521,8 @@
}
cleanPaths := make([]cleanPath, len(paths))
+ var commonVolume *string
+
// Clean all the paths, make them absolute.
// Find all the slashes in the paths.
//
@@ -526,18 +534,32 @@
if err := AbsPath(&path); err != nil {
return "", err
}
+ vol := filepath.VolumeName(path)
+
+ if commonVolume != nil {
+ // note: we check this first to allow testing; otherwise we would need to
+ // run tests on a machine with multiple volumes.
+ if vol != *commonVolume {
+ return "", errors.Reason("provided paths originate on different volumes: path[0]:%q, path[%d]:%q", *commonVolume, i, vol).Err()
+ }
+ } else {
+ commonVolume = &vol
+ }
+
fi, err := os.Lstat(path)
if err != nil {
- return "", err
+ return "", errors.Annotate(err, "reading path[%d]: %q", i, path).Err()
}
if !fi.IsDir() {
path = filepath.Dir(path)
fi, err := os.Lstat(path)
if err != nil {
- return "", err
+ // given that we know that the original `path` exists, this SHOULD be
+ // impossible, but FUSE exists so... idk.
+ return "", errors.Annotate(err, "reading Dir(path[%d]): %q", i, path).Err()
}
if !fi.IsDir() {
- // this SHOULD be impossible, but FUSE exists so... idk.
+ // this SHOULD ALSO be impossible...
return "", errors.Annotate(err, "path %q could not be resolved to parent dir", path).Err()
}
}
@@ -570,7 +592,9 @@
// However, ain't nobody got time for that.
slashesMatch := func(whichSlash int) bool {
for _, other := range cleanPaths[1:] {
- for i, slashIdx := range candidate.slashes[:whichSlash] {
+ // whichSlash+1 because we want to include everything up to, and
+ // including, whichSlash.
+ for i, slashIdx := range candidate.slashes[:whichSlash+1] {
if other.slashes[i] != slashIdx {
return false
}
@@ -646,5 +670,13 @@
return curPath, nil
}
}
- return "", errors.Reason("hit filesystem root").Err()
+
+ // Should never get here:
+ // * We are on unix and so whichSlash==0 is always the path "/". Either we
+ // found the root sentinel above, or returned "/" successfully.
+ // * We are on !unix and so whichSlash==0 is always a Volume (e.g. "C:\\").
+ // However we already checked when resolving `paths` at the top that all
+ // the cleanPaths share the SAME volume. Thus we should have either found
+ // the root sentinel or returned this same volume (i.e. `commonVolume`).
+ panic("impossible")
}
diff --git a/common/system/filesystem/filesystem_test.go b/common/system/filesystem/filesystem_test.go
index 3a29315..8af9e5d 100644
--- a/common/system/filesystem/filesystem_test.go
+++ b/common/system/filesystem/filesystem_test.go
@@ -24,6 +24,7 @@
"go.chromium.org/luci/common/errors"
. "github.com/smartystreets/goconvey/convey"
+ . "go.chromium.org/luci/common/testing/assertions"
)
func TestIsNotExist(t *testing.T) {
@@ -516,62 +517,113 @@
const sep = string(filepath.Separator)
Convey(`GetCommonAncestor`, t, func() {
- tdir := t.TempDir()
- So(os.MkdirAll(filepath.Join(tdir, "a", "b", "c", "d"), 0o777), ShouldBeNil)
- if fsCaseSensitive {
- So(os.MkdirAll(filepath.Join(tdir, "a", "B", "c"), 0o777), ShouldBeNil)
- So(os.MkdirAll(filepath.Join(tdir, "A", "b", "c", "d"), 0o777), ShouldBeNil)
- }
- So(os.MkdirAll(filepath.Join(tdir, "a", "1", "2", "3"), 0o777), ShouldBeNil)
- So(os.MkdirAll(filepath.Join(tdir, "r", "o", "o", "t"), 0o777), ShouldBeNil)
+ Convey(`common ancestor logic`, func() {
+ Convey(`common`, func() {
+ tdir := t.TempDir()
+ So(os.MkdirAll(filepath.Join(tdir, "a", "b", "c", "d"), 0o777), ShouldBeNil)
+ if fsCaseSensitive {
+ So(os.MkdirAll(filepath.Join(tdir, "a", "B", "c"), 0o777), ShouldBeNil)
+ So(os.MkdirAll(filepath.Join(tdir, "A", "b", "c", "d"), 0o777), ShouldBeNil)
+ }
+ So(os.MkdirAll(filepath.Join(tdir, "a", "1", "2", "3"), 0o777), ShouldBeNil)
+ So(os.MkdirAll(filepath.Join(tdir, "r", "o", "o", "t"), 0o777), ShouldBeNil)
- So(Touch(filepath.Join(tdir, "a", "B", "c", "something"), time.Now(), 0o666), ShouldBeNil)
- So(Touch(filepath.Join(tdir, "a", "b", "else"), time.Now(), 0o666), ShouldBeNil)
+ So(Touch(filepath.Join(tdir, "a", "B", "c", "something"), time.Now(), 0o666), ShouldBeNil)
+ So(Touch(filepath.Join(tdir, "a", "b", "else"), time.Now(), 0o666), ShouldBeNil)
- Convey(`regular`, func() {
- common, err := GetCommonAncestor([]string{
- filepath.Join(tdir, "a", "b", "c", "d"),
- filepath.Join(tdir, "a", "B", "c", "something"),
- filepath.Join(tdir, "A", "b", "c", "d"),
- filepath.Join(tdir, "a", "b", "else"),
- }, []string{".git"})
- So(err, ShouldBeNil)
- if fsCaseSensitive {
+ Convey(`stops at 'local' common ancestor`, func() {
+ common, err := GetCommonAncestor([]string{
+ filepath.Join(tdir, "a", "b", "c", "d"),
+ filepath.Join(tdir, "a", "B", "c", "something"),
+ filepath.Join(tdir, "A", "b", "c", "d"),
+ filepath.Join(tdir, "a", "b", "else"),
+ }, []string{".git"})
+ So(err, ShouldBeNil)
+ if fsCaseSensitive {
+ So(common, ShouldResemble, tdir+sep)
+ } else {
+ So(common, ShouldResemble, filepath.Join(tdir, "a", "b")+sep)
+ }
+ })
+
+ Convey(`walks up to fs root`, func() {
+ common, err := GetCommonAncestor([]string{
+ filepath.VolumeName(tdir) + sep,
+ filepath.Join(tdir, "a", "B", "c", "something"),
+ filepath.Join(tdir, "A", "b", "c", "d"),
+ filepath.Join(tdir, "a", "b", "else"),
+ }, []string{".git"})
+ So(err, ShouldBeNil)
+ So(common, ShouldResemble, filepath.VolumeName(tdir)+sep)
+ })
+ })
+
+ Convey(`slash comparison`, func() {
+ tdir := t.TempDir()
+ So(os.MkdirAll(filepath.Join(tdir, "longpath", "a", "c", "d"), 0o777), ShouldBeNil)
+ So(os.MkdirAll(filepath.Join(tdir, "a", "longpath", "c", "d"), 0o777), ShouldBeNil)
+
+ common, err := GetCommonAncestor([]string{
+ filepath.Join(tdir, "longpath", "a", "c", "d"),
+ filepath.Join(tdir, "a", "longpath", "c", "d"),
+ }, []string{".git"})
+ So(err, ShouldBeNil)
So(common, ShouldResemble, tdir+sep)
- } else {
- So(common, ShouldResemble, filepath.Join(tdir, "a", "b")+sep)
+ })
+
+ Convey(`non exist`, func() {
+ _, err := GetCommonAncestor([]string{
+ filepath.Join("i", "dont", "exist"),
+ }, []string{".git"})
+ So(err, ShouldErrLike, "reading path[0]")
+ })
+
+ Convey(`sentinel`, func() {
+ tdir := t.TempDir()
+ So(os.MkdirAll(filepath.Join(tdir, "repoA", ".git"), 0o777), ShouldBeNil)
+ So(os.MkdirAll(filepath.Join(tdir, "repoA", "something"), 0o777), ShouldBeNil)
+ So(os.MkdirAll(filepath.Join(tdir, "repoB", ".git"), 0o777), ShouldBeNil)
+ So(os.MkdirAll(filepath.Join(tdir, "repoB", "else"), 0o777), ShouldBeNil)
+
+ _, err := GetCommonAncestor([]string{
+ filepath.Join(tdir, "repoA", "something"),
+ filepath.Join(tdir, "repoB", "else"),
+ }, []string{".git"})
+ So(err, ShouldErrLike, "hit root sentinel")
+ })
+
+ if runtime.GOOS == "windows" {
+ Convey(`different volumes`, func() {
+ tdir := t.TempDir()
+ _, err := GetCommonAncestor([]string{
+ tdir,
+ `\\fake\network\share`,
+ }, nil)
+ So(err, ShouldErrLike, "originate on different volumes")
+ })
}
})
- Convey(`root`, func() {
- common, err := GetCommonAncestor([]string{
- filepath.VolumeName(tdir) + sep,
- filepath.Join(tdir, "a", "B", "c", "something"),
- filepath.Join(tdir, "A", "b", "c", "d"),
- filepath.Join(tdir, "a", "b", "else"),
- }, []string{".git"})
- So(err, ShouldBeNil)
- So(common, ShouldResemble, filepath.VolumeName(tdir)+sep)
+ Convey(`helpers`, func() {
+ if runtime.GOOS == "windows" {
+ Convey(`windows paths`, func() {
+ So(findPathSeparators(`D:\something\`), ShouldResemble, []int{2, 12})
+ So(findPathSeparators(`D:\`), ShouldResemble, []int{2})
+ So(findPathSeparators(`\\some\host\something\`),
+ ShouldResemble, []int{11, 21})
+ So(findPathSeparators(`\\some\host\`),
+ ShouldResemble, []int{11})
+ So(findPathSeparators(`\\?\C:\Test\`),
+ ShouldResemble, []int{6, 11})
+ })
+ } else {
+ Convey(`*nix paths`, func() {
+ So(findPathSeparators(`/something/`),
+ ShouldResemble, []int{0, 10})
+ So(findPathSeparators(`/`),
+ ShouldResemble, []int{0})
+ })
+ }
})
-
- if runtime.GOOS == "windows" {
- Convey(`windows paths`, func() {
- So(findPathSeparators(`D:\something\`), ShouldResemble, []int{2, 12})
- So(findPathSeparators(`D:\`), ShouldResemble, []int{2})
- So(findPathSeparators(`\\some\host\something\`),
- ShouldResemble, []int{11, 21})
- So(findPathSeparators(`\\some\host\`),
- ShouldResemble, []int{11})
- So(findPathSeparators(`\\?\C:\Test\`),
- ShouldResemble, []int{6, 11})
- })
- } else {
- Convey(`*nix paths`, func() {
- So(findPathSeparators(`/something/`),
- ShouldResemble, []int{0, 10})
- So(findPathSeparators(`/`),
- ShouldResemble, []int{0})
- })
- }
})
}