From b50a8b56bc5afeb65e39b9a08a5dc3db28f4eb89 Mon Sep 17 00:00:00 2001 From: Ibrahim AshShohail Date: Sun, 10 Dec 2017 13:22:26 +0300 Subject: [PATCH] gps: handle symlinks properly We now delete anything that looks like a symlink if its called "vendor" while pruning. Hopefully, you didn't make a bad decision by relying on the magical properties of symlinks. Signed-off-by: Ibrahim AshShohail --- gps/filesystem.go | 8 +- gps/filesystem_test.go | 106 ++++++++++- gps/prune_test.go | 424 ++++++++++++++++++++--------------------- 3 files changed, 308 insertions(+), 230 deletions(-) diff --git a/gps/filesystem.go b/gps/filesystem.go index 98803d35cb..fd683f325c 100644 --- a/gps/filesystem.go +++ b/gps/filesystem.go @@ -104,13 +104,9 @@ func deriveFilesystemState(root string) (filesystemState, error) { l := fsLink{path: relPath} l.to, err = filepath.EvalSymlinks(path) - if strings.HasSuffix(err.Error(), "too many links") { + if err != nil && strings.HasSuffix(err.Error(), "too many links") { l.circular = true - } else if err != nil { - return err - } - - if _, err := os.Stat(l.to); os.IsNotExist(err) { + } else if err != nil && os.IsNotExist(err) { l.broken = true } else if err != nil { return err diff --git a/gps/filesystem_test.go b/gps/filesystem_test.go index fcf3311816..af29846fd0 100644 --- a/gps/filesystem_test.go +++ b/gps/filesystem_test.go @@ -5,6 +5,7 @@ package gps import ( + "fmt" "os" "path/filepath" "reflect" @@ -99,28 +100,121 @@ func (tc fsTestCase) setup(t *testing.T) { func TestDeriveFilesystemState(t *testing.T) { testcases := []struct { - name string - state filesystemState + name string + fs fsTestCase }{ { - name: "simple-case", - state: filesystemState{}, + name: "simple-case", + fs: fsTestCase{ + before: filesystemState{ + dirs: []string{ + "simple-dir", + }, + files: []string{ + "simple-file", + }, + }, + after: filesystemState{ + dirs: []string{ + "simple-dir", + }, + files: []string{ + "simple-file", + }, + }, + }, + }, + { + name: "simple-symlink-case", + fs: fsTestCase{ + before: filesystemState{ + dirs: []string{ + "simple-dir", + }, + files: []string{ + "simple-file", + }, + links: []fsLink{ + fsLink{ + path: "link", + to: "nonexisting", + broken: true, + }, + }, + }, + after: filesystemState{ + dirs: []string{ + "simple-dir", + }, + files: []string{ + "simple-file", + }, + links: []fsLink{ + fsLink{ + path: "link", + to: "", + broken: true, + }, + }, + }, + }, + }, + { + name: "complex-symlink-case", + fs: fsTestCase{ + before: filesystemState{ + links: []fsLink{ + fsLink{ + path: "link1", + to: "link2", + circular: true, + }, + fsLink{ + path: "link2", + to: "link1", + circular: true, + }, + }, + }, + after: filesystemState{ + links: []fsLink{ + fsLink{ + path: "link1", + to: "", + circular: true, + }, + fsLink{ + path: "link2", + to: "", + circular: true, + }, + }, + }, + }, }, } for _, tc := range testcases { h := test.NewHelper(t) - defer h.Cleanup() h.TempDir(tc.name) + tc.fs.before.root = h.Path(tc.name) + tc.fs.after.root = h.Path(tc.name) + + tc.fs.setup(t) + state, err := deriveFilesystemState(h.Path(tc.name)) if err != nil { t.Fatal(err) } - if !reflect.DeepEqual(tc.state, state) { + if !reflect.DeepEqual(tc.fs.after, state) { + fmt.Println(tc.fs.after) + fmt.Println(state) t.Fatal("filesystem state mismatch") } + + h.Cleanup() } } diff --git a/gps/prune_test.go b/gps/prune_test.go index 19dcfeac5c..417d88892f 100644 --- a/gps/prune_test.go +++ b/gps/prune_test.go @@ -52,19 +52,6 @@ func TestPruneProject(t *testing.T) { } } -// func TestPruneVendorDirs(t *testing.T) { -// h := test.NewHelper(t) -// defer h.Cleanup() - -// h.TempDir(".") -// baseDir := h.Path(".") - -// err := pruneVendorDirs(baseDir) -// if err != nil { -// t.Fatal(err) -// } -// } - func TestPruneUnusedPackages(t *testing.T) { h := test.NewHelper(t) defer h.Cleanup() @@ -391,241 +378,242 @@ func TestPruneGoTestFiles(t *testing.T) { } } -func pruneVendorDirsTestCase(tc fsTestCase) func(*testing.T) { - return func(t *testing.T) { - tempDir, err := ioutil.TempDir("", "pruneVendorDirsTestCase") - if err != nil { - t.Fatalf("ioutil.TempDir err=%q", err) - } - defer func() { - if err := os.RemoveAll(tempDir); err != nil { - t.Errorf("os.RemoveAll(%q) err=%q", tempDir, err) - } - }() - - tc.before.root = tempDir - tc.after.root = tempDir - - tc.setup(t) - - fs, err := deriveFilesystemState(tempDir) - if err != nil { - t.Fatalf("deriveFilesystemState failed: %s", err) - } - - if err := pruneVendorDirs(fs); err != nil { - t.Errorf("pruneVendorDirs err=%q", err) - } - - tc.assert(t) - } -} - func TestPruneVendorDirs(t *testing.T) { - t.Run("vendor directory", pruneVendorDirsTestCase(fsTestCase{ - before: filesystemState{ - dirs: []string{ - "package", - "package/vendor", - }, - }, - after: filesystemState{ - dirs: []string{ - "package", - }, - }, - })) - - t.Run("vendor file", pruneVendorDirsTestCase(fsTestCase{ - before: filesystemState{ - dirs: []string{ - "package", - }, - files: []string{ - "package/vendor", - }, - }, - after: filesystemState{ - dirs: []string{ - "package", - }, - files: []string{ - "package/vendor", - }, - }, - })) -} - -func TestPruneVendorSymlinks(t *testing.T) { - // On windows, we skip symlinks, even if they're named 'vendor', because - // they're too hard to distinguish from junctions. - t.Run("vendor symlink", pruneVendorDirsTestCase(fsTestCase{ - before: filesystemState{ - dirs: []string{ - "package", - "package/_vendor", - }, - links: []fsLink{ - { - path: "package/vendor", - to: "_vendor", + tests := []struct { + name string + test fsTestCase + }{ + { + name: "vendor directory", + test: fsTestCase{ + before: filesystemState{ + dirs: []string{ + "package", + "package/vendor", + }, }, - }, - }, - after: filesystemState{ - dirs: []string{ - "package", - "package/_vendor", - }, - links: []fsLink{ - { - path: "package/vendor", - to: "_vendor", + after: filesystemState{ + dirs: []string{ + "package", + }, }, }, }, - })) - - t.Run("nonvendor symlink", pruneVendorDirsTestCase(fsTestCase{ - before: filesystemState{ - dirs: []string{ - "package", - "package/_vendor", - }, - links: []fsLink{ - { - path: "package/link", - to: "_vendor", + { + name: "vendor file", + test: fsTestCase{ + before: filesystemState{ + dirs: []string{ + "package", + }, + files: []string{ + "package/vendor", + }, }, - }, - }, - after: filesystemState{ - dirs: []string{ - "package", - "package/_vendor", - }, - links: []fsLink{ - { - path: "package/link", - to: "_vendor", + after: filesystemState{ + dirs: []string{ + "package", + }, + files: []string{ + "package/vendor", + }, }, }, }, - })) - - t.Run("vendor symlink to file", pruneVendorDirsTestCase(fsTestCase{ - before: filesystemState{ - files: []string{ - "file", - }, - links: []fsLink{ - { - path: "vendor", - to: "file", + { + name: "vendor symlink", + test: fsTestCase{ + before: filesystemState{ + dirs: []string{ + "package", + "package/_vendor", + }, + links: []fsLink{ + { + path: "package/vendor", + to: "_vendor", + }, + }, }, - }, - }, - after: filesystemState{ - files: []string{ - "file", - }, - links: []fsLink{ - { - path: "vendor", - to: "file", + after: filesystemState{ + dirs: []string{ + "package", + "package/_vendor", + }, }, }, }, - })) - - t.Run("broken vendor symlink", pruneVendorDirsTestCase(fsTestCase{ - before: filesystemState{ - dirs: []string{ - "package", - }, - links: []fsLink{ - { - path: "package/vendor", - to: "nonexistence", + { + name: "nonvendor symlink", + test: fsTestCase{ + before: filesystemState{ + dirs: []string{ + "package", + "package/_vendor", + }, + links: []fsLink{ + { + path: "package/link", + to: "_vendor", + }, + }, }, - }, - }, - after: filesystemState{ - dirs: []string{ - "package", - }, - links: []fsLink{ - { - path: "package/vendor", - to: "nonexistence", + after: filesystemState{ + dirs: []string{ + "package", + "package/_vendor", + }, + links: []fsLink{ + { + path: "package/link", + to: "_vendor", + }, + }, }, }, }, - })) - - t.Run("chained symlinks", pruneVendorDirsTestCase(fsTestCase{ - // Curiously, if a symlink on windows points to *another* symlink which - // eventually points at a directory, we'll correctly remove that first - // symlink, because the first symlink doesn't appear to Go to be a - // directory. - before: filesystemState{ - dirs: []string{ - "_vendor", - }, - links: []fsLink{ - { - path: "vendor", - to: "vendor2", + { + name: "vendor symlink to file", + test: fsTestCase{ + before: filesystemState{ + files: []string{ + "file", + }, + links: []fsLink{ + { + path: "vendor", + to: "file", + }, + }, }, - { - path: "vendor2", - to: "_vendor", + after: filesystemState{ + files: []string{ + "file", + }, }, }, }, - after: filesystemState{ - dirs: []string{ - "_vendor", - }, - links: []fsLink{ - { - path: "vendor2", - to: "_vendor", + { + name: "broken vendor symlink", + test: fsTestCase{ + before: filesystemState{ + dirs: []string{ + "package", + }, + links: []fsLink{ + { + path: "package/vendor", + to: "nonexistence", + }, + }, + }, + after: filesystemState{ + dirs: []string{ + "package", + }, + links: []fsLink{}, }, }, }, - })) - - t.Run("circular symlinks", pruneVendorDirsTestCase(fsTestCase{ - before: filesystemState{ - dirs: []string{ - "package", - }, - links: []fsLink{ - { - path: "package/link1", - to: "link2", + { + name: "chained symlinks", + test: fsTestCase{ + before: filesystemState{ + dirs: []string{ + "_vendor", + }, + links: []fsLink{ + { + path: "vendor", + to: "vendor2", + }, + { + path: "vendor2", + to: "_vendor", + }, + }, }, - { - path: "package/link2", - to: "link1", + after: filesystemState{ + dirs: []string{ + "_vendor", + }, + links: []fsLink{ + { + path: "vendor2", + to: "_vendor", + }, + }, }, }, }, - after: filesystemState{ - dirs: []string{ - "package", - }, - links: []fsLink{ - { - path: "package/link1", - to: "link2", + { + name: "circular symlinks", + test: fsTestCase{ + before: filesystemState{ + dirs: []string{ + "package", + }, + links: []fsLink{ + { + path: "package/link1", + to: "link2", + }, + { + path: "package/link2", + to: "link1", + }, + }, }, - { - path: "package/link2", - to: "link1", + after: filesystemState{ + dirs: []string{ + "package", + }, + links: []fsLink{ + { + path: "package/link1", + to: "link2", + }, + { + path: "package/link2", + to: "link1", + }, + }, }, }, }, - })) + } + + for _, test := range tests { + t.Run(test.name, pruneVendorDirsTestCase(test.test)) + } +} + +func pruneVendorDirsTestCase(tc fsTestCase) func(*testing.T) { + return func(t *testing.T) { + tempDir, err := ioutil.TempDir("", "pruneVendorDirsTestCase") + if err != nil { + t.Fatalf("ioutil.TempDir err=%q", err) + } + defer func() { + if err := os.RemoveAll(tempDir); err != nil { + t.Errorf("os.RemoveAll(%q) err=%q", tempDir, err) + } + }() + + tc.before.root = tempDir + tc.after.root = tempDir + + tc.setup(t) + + fs, err := deriveFilesystemState(tempDir) + if err != nil { + t.Fatalf("deriveFilesystemState failed: %s", err) + } + + if err := pruneVendorDirs(fs); err != nil { + t.Errorf("pruneVendorDirs err=%q", err) + } + + tc.assert(t) + } }