Use glide list as the primary source of package dependencies

The old glide.lock behavior is still available via --use-lock-file.
This also refactors the cleanup function to hopefully be easier to follow.
Also adds more tests around getLastVendorPath and isParentDirectory.

.travis.yml

@@ -8,6 +8,7 @@ go:
 sudo: false
 
 install:
+  - go get github.com/Masterminds/glide
   - GO15VENDOREXPERIMENT=1 go build
 
 script:

README.md

@@ -8,7 +8,7 @@ For a detailed explanation on why Glide doesn't do this see [here](http://engine
 
 ## Description
 
-This tool will help you removing from the project vendor directories all the files not needed for building your project. By default it'll keep all the files provided by packages listed in the `glide.lock` file.
+This tool will help you removing from the project vendor directories all the files not needed for building your project. By default it'll keep all the files provided by packages returned by the `glide list` command.
 If you want to keep only go (including tests) files you can provide the `--only-code` option.
 If you want to remove also the go test files you can add the `--no-tests` option.
 

appveyor.yml

@@ -12,11 +12,12 @@ platform:
 install:
 - go version
 - go env
+- go get github.com/Masterminds/glide
 
 build_script:
 - go build
 
 test_script:
-- go test .
+- PATH=$GOPATH/bin:$PATH go test .
 
 deploy: off

gvc.go

@@ -1,9 +1,11 @@
 package main
 
 import (
+	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"strings"
 
@@ -20,12 +22,21 @@ var cmd = &cobra.Command{
 }
 
 type options struct {
-	dryrun        bool
-	onlyCode      bool
-	noTests       bool
+	dryrun       bool
+	onlyCode     bool
+	noTests      bool
+	noLegalFiles bool
+	keepPatterns []string
+
+	// Deprecated
+	useLockFile   bool
 	noTestImports bool
-	noLegalFiles  bool
-	keepPatterns  []string
+}
+
+type packages struct {
+	Installed []string `json:"installed"`
+	Missing   []string `json:"missing"`
+	Gopath    []string `json:"gopath"`
 }
 
 var (
@@ -40,10 +51,12 @@ const (
 func init() {
 	cmd.PersistentFlags().BoolVar(&opts.dryrun, "dryrun", false, "just output what will be removed")
 	cmd.PersistentFlags().BoolVar(&opts.onlyCode, "only-code", false, "keep only go files (including go test files)")
-	cmd.PersistentFlags().BoolVar(&opts.noTestImports, "no-test-imports", false, "remove also testImport vendor directories")
 	cmd.PersistentFlags().BoolVar(&opts.noTests, "no-tests", false, "remove also go test files (requires --only-code)")
 	cmd.PersistentFlags().BoolVar(&opts.noLegalFiles, "no-legal-files", false, "remove also licenses and legal files")
 	cmd.PersistentFlags().StringSliceVar(&opts.keepPatterns, "keep", []string{}, "A pattern to keep additional files inside needed packages. The pattern match will be relative to the deeper vendor dir. Supports double star (**) patterns. (see https://golang.org/pkg/path/filepath/#Match and https://github.com/bmatcuk/doublestar). Can be specified multiple times. For example to keep all the files with json extension use the '**/*.json' pattern.")
+
+	cmd.PersistentFlags().BoolVar(&opts.useLockFile, "use-lock-file", false, "use glide.lock instead of glide list to determine imports")
+	cmd.PersistentFlags().BoolVar(&opts.noTestImports, "no-test-imports", false, "remove also testImport vendor directories")
 }
 
 func main() {
@@ -56,53 +69,87 @@ func glidevc(cmd *cobra.Command, args []string) {
 		os.Exit(1)
 	}
 
-	if err := cleanup(".", opts); err != nil {
+	if err := cleanup("."); err != nil {
 		fmt.Print(err)
 		os.Exit(1)
 	}
-	return
 }
 
-func cleanup(path string, opts options) error {
-	lock, err := LoadGlideLockfile(path)
+func glideLockImports(path string) ([]string, error) {
+	yml, err := ioutil.ReadFile(filepath.Join(path, gpath.LockFile))
 	if err != nil {
-		return fmt.Errorf("Could not load lockfile: %v", err)
+		return nil, err
 	}
 
-	// The package list already have the path converted to the os specific
-	// path separator, needed for future comparisons.
-	pkgList := []string{}
-	repoList := []string{}
-	adder := func(lock cfg.Locks) {
-		for _, imp := range lock {
-			// This converts pkg separator "/" to os specific separator
-			repoList = append(repoList, filepath.FromSlash(imp.Name))
+	lock, err := cfg.LockfileFromYaml(yml)
+	if err != nil {
+		return nil, err
+	}
 
-			if len(imp.Subpackages) > 0 {
-				for _, sp := range imp.Subpackages {
-					// This converts pkg separator "/" to os specific separator
-					pkgList = append(pkgList, filepath.Join(imp.Name, sp))
-				}
+	var imports []string
+	adder := func(locks cfg.Locks) {
+		for _, lock := range locks {
+			for _, subpackage := range lock.Subpackages {
+				imports = append(imports, lock.Name+"/"+subpackage)
 			}
-			// TODO(sgotti) we cannot skip the base import if it has subpackages
-			// because glide doesn't write "." as a subpackage, otherwise if some
-			// files in the base import are needed they will be removed.
-
-			// This converts pkg separator "/" to os specific separator
-			pkgList = append(pkgList, filepath.FromSlash(imp.Name))
+			imports = append(imports, lock.Name)
 		}
 	}
+
 	adder(lock.Imports)
 	if !opts.noTestImports {
 		adder(lock.DevImports)
 	}
 
+	return imports, nil
+}
+
+func glideListImports(path string) ([]string, error) {
+	cmd := exec.Command("glide", "list", "-output", "json", path)
+	out, err := cmd.Output()
+	if err != nil {
+		return nil, err
+	}
+
+	list := &packages{}
+	err = json.Unmarshal(out, list)
+	if err != nil {
+		return nil, err
+	}
+
+	return list.Installed, nil
+}
+
+func cleanup(path string) error {
+	var (
+		packages []string
+		err      error
+	)
+
+	if opts.useLockFile {
+		packages, err = glideLockImports(path)
+	} else {
+		packages, err = glideListImports(path)
+	}
+
+	// The package list already have the path converted to the os specific
+	// path separator, needed for future comparisons.
+	pkgList := []string{}
+	pkgMap := map[string]struct{}{}
+	for _, imp := range packages {
+		if _, found := pkgMap[imp]; !found {
+			// This converts pkg separator "/" to os specific separator
+			pkgList = append(pkgList, filepath.FromSlash(imp))
+			pkgMap[imp] = struct{}{}
+		}
+	}
+
 	vpath, err := gpath.Vendor()
 	if err != nil {
 		return err
 	}
 	if vpath == "" {
-		return fmt.Errorf("cannot fine vendor dir")
+		return fmt.Errorf("cannot find vendor dir")
 	}
 
 	type pathData struct {
@@ -124,83 +171,63 @@ func cleanup(path string, opts options) error {
 			return nil
 		}
 
+		// Short-circuit for test files
+		if opts.noTests && strings.HasSuffix(path, "_test.go") {
+			return nil
+		}
+
 		localPath := strings.TrimPrefix(path, searchPath)
 
 		lastVendorPath, err := getLastVendorPath(localPath)
 		if err != nil {
 			return err
 		}
-		if lastVendorPath == "" {
-			lastVendorPath = localPath
-		}
+		lastVendorPathDir := filepath.Dir(lastVendorPath)
 
 		keep := false
+
 		for _, name := range pkgList {
-			// If the file's parent directory is a needed package, keep it.
-			if !info.IsDir() && filepath.Dir(lastVendorPath) == name {
-				if opts.onlyCode {
-					if opts.noTests && strings.HasSuffix(path, "_test.go") {
-						keep = false
-						continue
-					}
-					for _, suffix := range codeSuffixes {
-						if strings.HasSuffix(path, suffix) {
-							keep = true
-						}
-					}
-					// Match keep patterns
-					for _, keepPattern := range opts.keepPatterns {
-						ok, err := doublestar.Match(keepPattern, lastVendorPath)
-						// TODO(sgotti) if a bad pattern is encountered stop here. Actually there's no function to verify a pattern before using it, perhaps just a fake match at the start will work.
-						if err != nil {
-							return fmt.Errorf("bad pattern: %q", keepPattern)
-						}
-						if ok {
-							keep = true
-						}
-					}
-				} else {
-					keep = true
-				}
+			// If a directory is a needed package then keep it
+			keep = keep || info.IsDir() && name == lastVendorPath
+
+			// The remaining tests are only for files
+			if info.IsDir() {
+				continue
 			}
-		}
 
-		// Keep all the legal files inside top repo dir and required package dirs
-		for _, name := range append(repoList, pkgList...) {
-			if !info.IsDir() && filepath.Dir(lastVendorPath) == name {
-				if !opts.noLegalFiles {
-					if IsLegalFile(path) {
-						keep = true
-					}
-				}
+			// Keep legal files in directories that are the parent of a needed package
+			keep = keep || !opts.noLegalFiles && IsLegalFile(path) && isParentDirectory(lastVendorPathDir, name)
+
+			// The remaining tests only apply if the file is in a needed package
+			if name != lastVendorPathDir {
+				continue
 			}
-		}
 
-		// If a directory is a needed package then keep it
-		if keep == false && info.IsDir() {
-			for _, name := range pkgList {
-				if name == lastVendorPath {
-					keep = true
+			// Keep everything unless --only-code was specified
+			keep = keep || !opts.onlyCode
+
+			// Always keep code files
+			for _, suffix := range codeSuffixes {
+				keep = keep || strings.HasSuffix(path, suffix)
+			}
+
+			// Match keep patterns
+			for _, keepPattern := range opts.keepPatterns {
+				ok, err := doublestar.Match(keepPattern, lastVendorPath)
+				// TODO(sgotti) if a bad pattern is encountered stop here. Actually there's no function to verify a pattern before using it, perhaps just a fake match at the start will work.
+				if err != nil {
+					return fmt.Errorf("bad pattern: %q", keepPattern)
 				}
+				keep = keep || ok
 			}
 		}
 
 		if keep {
-			// Keep also all parents of current path
-			curpath := localPath
-			for {
-				curpath = filepath.Dir(curpath)
-				if curpath == "." {
-					break
-				}
-				if _, ok := markForKeep[curpath]; ok {
-					// Already marked for keep
-					break
-				}
+			// Keep all parent directories of current path
+			for curpath := localPath; curpath != "."; curpath = filepath.Dir(curpath) {
 				markForKeep[curpath] = pathData{curpath, true}
 			}
-
-			// Mark for keep
+			// Fix isDir property for current path
 			markForKeep[localPath] = pathData{localPath, info.IsDir()}
 		}
 
@@ -255,30 +282,22 @@ func cleanup(path string, opts options) error {
 }
 
 func getLastVendorPath(path string) (string, error) {
-	curpath := path
-	for {
-		if curpath == "." {
-			return "", nil
-		}
+	for curpath := path; curpath != "."; curpath = filepath.Dir(curpath) {
 		if filepath.Base(curpath) == "vendor" {
 			return filepath.Rel(curpath, path)
 		}
-		curpath = filepath.Dir(curpath)
 	}
+	return path, nil
 }
 
-// LoadLockfile loads the contents of a glide.lock file.
-func LoadGlideLockfile(base string) (*cfg.Lockfile, error) {
-	yml, err := ioutil.ReadFile(filepath.Join(base, gpath.LockFile))
-	if err != nil {
-		return nil, err
+func isParentDirectory(parent, child string) bool {
+	if !strings.HasSuffix(parent, "/") {
+		parent += "/"
 	}
-	lock, err := cfg.LockfileFromYaml(yml)
-	if err != nil {
-		return nil, err
+	if !strings.HasSuffix(child, "/") {
+		child += "/"
 	}
-
-	return lock, nil
+	return strings.HasPrefix(child, parent)
 }
 
 // File lists and code took from https://github.com/client9/gosupplychain/blob/master/license.go
@@ -316,7 +335,7 @@ func IsLegalFile(path string) bool {
 		}
 	}
 	for _, substring := range LegalFileSubstring {
-		if strings.Index(lowerfile, substring) != -1 && !strings.HasSuffix(lowerfile, goTestSuffix) {
+		if strings.Contains(lowerfile, substring) && !strings.HasSuffix(lowerfile, goTestSuffix) {
 			return true
 		}
 	}