[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})
-			})
-		}
 	})
 }