do not create cachedir recursively

source
- main.go: Try to ensure directory for given `cachedir` path.
- context.go: Create the default cache directory, `$GOPATH/pkg/dep`, if the
  user did not override it.
- source_manager.go: Use `fs.EnsureDir` instead of `os.MkdirAll` for creating
  sources folder in cache directory.
- fs.go:
  - Add func `EnsureDir` to create a directory if it does not exist.
  - Remove func `IsValidPath`.

test
- integration_test.go: Improve tests for invalid cache directory.
- fs_test.go: Add test for `EnsureDir`, remove test for `IsValidPath`.
- manager_test.go: fix TestSourceManagerInit
  - Re-create cache directory before trying to call `NewSourceManager` the 2nd
    time and defer it's removal.
  - If `NewSourceManager` fails the 2nd time, exit the error using `t.Fatal` to
    avoid panic in `sm.Release`

misc
- language - {fallback => default} for cachedir

Author: sudo-suhas

cmd/dep/integration_test.go

@@ -7,6 +7,7 @@ package main
 import (
 	"fmt"
 	"io"
+	"io/ioutil"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -98,21 +99,36 @@ func TestDepCachedir(t *testing.T) {
 		testProj := integration.NewTestProject(t, initPath, wd, runMain)
 		defer testProj.Cleanup()
 
-		cachedir := "/invalid/path"
-		testProj.Setenv("DEPCACHEDIR", cachedir)
-		wantErr := fmt.Sprintf(
-			"dep: $DEPCACHEDIR set to an invalid or inaccessible path: %q", cachedir,
-		)
-
-		// Running `dep ensure` will pull in the dependency into cachedir.
-		err = testProj.DoRun([]string{"ensure"})
+		var d []byte
+		tmpFp := testProj.Path("tmp-file")
+		ioutil.WriteFile(tmpFp, d, 0644)
+		cases := []string{
+			// invalid path
+			"\000",
+			// parent directory does not exist
+			testProj.Path("non-existent-fldr", "cachedir"),
+			// path is a regular file
+			tmpFp,
+			// invalid path, tmp-file is a regular file
+			testProj.Path("tmp-file", "cachedir"),
+		}
 
-		if err == nil {
-			// Log the output from running `dep ensure`, could be useful.
-			t.Logf("test run output: \n%s\n%s", testProj.GetStdout(), testProj.GetStderr())
-			t.Error("unexpected result: \n\t(GOT) nil\n\t(WNT) exit status 1")
-		} else if gotErr := strings.TrimSpace(testProj.GetStderr()); gotErr != wantErr {
-			t.Errorf("unexpected error output: \n\t(GOT) %s\n\t(WNT) %s", gotErr, wantErr)
+		wantErr := "dep: $DEPCACHEDIR set to an invalid or inaccessible path"
+		for _, c := range cases {
+			testProj.Setenv("DEPCACHEDIR", c)
+
+			err = testProj.DoRun([]string{"ensure"})
+
+			if err == nil {
+				// Log the output from running `dep ensure`, could be useful.
+				t.Logf("test run output: \n%s\n%s", testProj.GetStdout(), testProj.GetStderr())
+				t.Error("unexpected result: \n\t(GOT) nil\n\t(WNT) exit status 1")
+			} else if stderr := testProj.GetStderr(); !strings.Contains(stderr, wantErr) {
+				t.Errorf(
+					"unexpected error output: \n\t(GOT) %s\n\t(WNT) %s",
+					strings.TrimSpace(stderr), wantErr,
+				)
+			}
 		}
 	})
 

cmd/dep/main.go

@@ -182,13 +182,16 @@ func (c *Config) Run() int {
 			}
 
 			// Cachedir is loaded from env if present. `$GOPATH/pkg/dep` is used as the
-			// fallback cache location.
+			// default cache location.
 			cachedir := getEnv(c.Env, "DEPCACHEDIR")
-			if cachedir != "" && !fs.IsValidPath(cachedir) {
-				errLogger.Printf(
-					"dep: $DEPCACHEDIR set to an invalid or inaccessible path: %q\n", cachedir,
-				)
-				return errorExitCode
+			if cachedir != "" {
+				if err := fs.EnsureDir(cachedir, 0777); err != nil {
+					errLogger.Printf(
+						"dep: $DEPCACHEDIR set to an invalid or inaccessible path: %q\n", cachedir,
+					)
+					errLogger.Printf("dep: failed to ensure cache directory: %v\n", err)
+					return errorExitCode
+				}
 			}
 
 			// Set up dep context.

context.go

@@ -90,8 +90,12 @@ func defaultGOPATH() string {
 func (c *Ctx) SourceManager() (*gps.SourceMgr, error) {
 	cachedir := c.Cachedir
 	if cachedir == "" {
-		// When `DEPCACHEDIR` isn't set in the env, fallback to `$GOPATH/pkg/dep`.
+		// When `DEPCACHEDIR` isn't set in the env, use the default - `$GOPATH/pkg/dep`.
 		cachedir = filepath.Join(c.GOPATH, "pkg", "dep")
+		// Create the default cachedir if it does not exist.
+		if err := os.MkdirAll(cachedir, 0777); err != nil {
+			return nil, errors.Wrap(err, "failed to create default cache directory")
+		}
 	}
 
 	return gps.NewSourceManager(gps.SourceManagerConfig{

context_test.go

@@ -491,7 +491,7 @@ func TestDepCachedir(t *testing.T) {
 	defer h.Cleanup()
 
 	h.TempDir("cache")
-	// Create the directory for fallback cachedir location.
+	// Create the directory for default cachedir location.
 	h.TempDir(filepath.Join("go", "pkg", "dep"))
 
 	testCachedir := h.Path("cache")

gps/manager_test.go

@@ -121,17 +121,23 @@ func TestSourceManagerInit(t *testing.T) {
 		t.Fatalf("Global cache lock file not cleared correctly on Release()")
 	}
 
+	err = os.MkdirAll(cpath, 0777)
+	if err != nil {
+		t.Errorf("Failed to re-create temp dir: %s", err)
+	}
+	defer func() {
+		err = os.RemoveAll(cpath)
+		if err != nil {
+			t.Errorf("removeAll failed: %s", err)
+		}
+	}()
 	// Set another one up at the same spot now, just to be sure
 	sm, err = NewSourceManager(cfg)
 	if err != nil {
-		t.Errorf("Creating a second SourceManager should have succeeded when the first was released, but failed with err %s", err)
+		t.Fatalf("Creating a second SourceManager should have succeeded when the first was released, but failed with err %s", err)
 	}
 
 	sm.Release()
-	err = os.RemoveAll(cpath)
-	if err != nil {
-		t.Errorf("removeAll failed: %s", err)
-	}
 }
 
 func TestSourceInit(t *testing.T) {

gps/source_manager.go

@@ -20,6 +20,7 @@ import (
 	"time"
 
 	"github.com/golang/dep/gps/pkgtree"
+	"github.com/golang/dep/internal/fs"
 	"github.com/nightlyone/lockfile"
 	"github.com/pkg/errors"
 	"github.com/sdboyer/constext"
@@ -197,7 +198,7 @@ func NewSourceManager(c SourceManagerConfig) (*SourceMgr, error) {
 		c.Logger = log.New(ioutil.Discard, "", 0)
 	}
 
-	err := os.MkdirAll(filepath.Join(c.Cachedir, "sources"), 0777)
+	err := fs.EnsureDir(filepath.Join(c.Cachedir, "sources"), 0777)
 	if err != nil {
 		return nil, err
 	}

internal/fs/fs.go

@@ -480,22 +480,21 @@ func cloneSymlink(sl, dst string) error {
 	return os.Symlink(resolved, dst)
 }
 
-// IsValidPath checks if the given string is a valid path.
-func IsValidPath(fp string) bool {
-	// See https://stackoverflow.com/questions/35231846/golang-check-if-string-is-valid-path
-	// Check if file/dir already exists
-	if _, err := os.Stat(fp); err == nil {
-		return true
-	}
+// EnsureDir tries to ensure that a directory is present at the given path. It first
+// checks if the directory already exists at the given path. If there isn't one, it tries
+// to create it with the given permissions. However, it does not try to create the
+// directory recursively.
+func EnsureDir(path string, perm os.FileMode) error {
+	_, err := IsDir(path)
 
-	// Attempt to create it
-	var d []byte
-	if err := ioutil.WriteFile(fp, d, 0644); err == nil {
-		os.Remove(fp) // And delete it
-		return true
+	if os.IsNotExist(err) {
+		err = os.Mkdir(path, perm)
+		if err != nil {
+			return errors.Wrapf(err, "failed to ensure directory at %q", path)
+		}
 	}
 
-	return false
+	return err
 }
 
 // IsDir determines is the path given is a directory or not.

internal/fs/fs_test.go

@@ -837,28 +837,32 @@ func setupInaccessibleDir(t *testing.T, op func(dir string) error) func() {
 	return cleanup
 }
 
-func TestIsValidPath(t *testing.T) {
-	wd, err := os.Getwd()
-	if err != nil {
-		t.Fatal(err)
-	}
+func TestEnsureDir(t *testing.T) {
+	h := test.NewHelper(t)
+	defer h.Cleanup()
+	h.TempDir(".")
+	h.TempFile("file", "")
 
-	var dn string
+	tmpPath := h.Path(".")
 
+	var dn string
 	cleanup := setupInaccessibleDir(t, func(dir string) error {
 		dn = filepath.Join(dir, "dir")
 		return os.Mkdir(dn, 0777)
 	})
 	defer cleanup()
 
 	tests := map[string]bool{
-		wd: true,
-		filepath.Join(wd, "testdata"):                       true,
-		filepath.Join(wd, "main.go"):                        true,
-		filepath.Join(wd, "this_file_does_not_exist.thing"): true,
-		dn:              false,
-		"":              false,
-		"/invalid/path": false,
+		// [success] A dir already exists for the given path.
+		tmpPath: true,
+		// [success] Dir does not exist but parent dir exists, so should get created.
+		filepath.Join(tmpPath, "testdir"): true,
+		// [failure] Dir and parent dir do not exist, should return an error.
+		filepath.Join(tmpPath, "notexist", "testdir"): false,
+		// [failure] Regular file present at given path.
+		h.Path("file"): false,
+		// [failure] Path inaccessible.
+		dn: false,
 	}
 
 	if runtime.GOOS == "windows" {
@@ -869,11 +873,17 @@ func TestIsValidPath(t *testing.T) {
 		delete(tests, dn)
 	}
 
-	for fp, want := range tests {
-		got := IsValidPath(fp)
-
-		if got != want {
-			t.Fatalf("expected %t for %s, got %t", want, fp, got)
+	for path, shouldEnsure := range tests {
+		err := EnsureDir(path, 0777)
+		if shouldEnsure {
+			if err != nil {
+				t.Fatalf("unexpected error %q for %q", err, path)
+			} else if ok, err := IsDir(path); !ok {
+				t.Fatalf("expected directory to be preset at %q", path)
+				t.Fatal(err)
+			}
+		} else if err == nil {
+			t.Fatalf("expected error for path %q, got none", path)
 		}
 	}
 }