Merge pull request #740 from monopole/replaceStringWithRepoSpec

Replace all repo uri strings with git.RepoSpec.

pkg/git/cloner.go

@@ -25,23 +25,19 @@ import (
 )
 
 // Cloner is a function that can clone a git repo.
-type Cloner func(url string) (*RepoSpec, error)
+type Cloner func(repoSpec *RepoSpec) error
 
 // ClonerUsingGitExec uses a local git install, as opposed
 // to say, some remote API, to obtain a local clone of
 // a remote repo.
-func ClonerUsingGitExec(spec string) (*RepoSpec, error) {
+func ClonerUsingGitExec(repoSpec *RepoSpec) error {
 	gitProgram, err := exec.LookPath("git")
 	if err != nil {
-		return nil, errors.Wrap(err, "no 'git' program on path")
+		return errors.Wrap(err, "no 'git' program on path")
-	}
-	repoSpec, err := NewRepoSpecFromUrl(spec)
-	if err != nil {
-		return nil, err
 	}
 	repoSpec.cloneDir, err = fs.NewTmpConfirmedDir()
 	if err != nil {
-		return nil, err
+		return err
 	}
 	cmd := exec.Command(
 		gitProgram,
@@ -52,17 +48,28 @@ func ClonerUsingGitExec(spec string) (*RepoSpec, error) {
 	cmd.Stdout = &out
 	err = cmd.Run()
 	if err != nil {
-		return nil, errors.Wrapf(err, "trouble cloning %s", spec)
+		return errors.Wrapf(err, "trouble cloning %s", repoSpec.raw)
 	}
 	if repoSpec.ref == "" {
-		return repoSpec, nil
+		return nil
 	}
 	cmd = exec.Command(gitProgram, "checkout", repoSpec.ref)
 	cmd.Dir = repoSpec.cloneDir.String()
 	err = cmd.Run()
 	if err != nil {
-		return nil, errors.Wrapf(
+		return errors.Wrapf(
 			err, "trouble checking out href %s", repoSpec.ref)
 	}
-	return repoSpec, nil
+	return nil
+}
+
+// DoNothingCloner returns a cloner that only sets
+// cloneDir field in the repoSpec.  It's assumed that
+// the cloneDir is associated with some fake filesystem
+// used in a test.
+func DoNothingCloner(dir fs.ConfirmedDir) Cloner {
+	return func(rs *RepoSpec) error {
+		rs.cloneDir = dir
+		return nil
+	}
 }

pkg/loader/fileloader.go

@@ -114,44 +114,45 @@ func (l *fileLoader) Root() string {
 }
 
 func newLoaderOrDie(fSys fs.FileSystem, path string) *fileLoader {
-	l, err := newLoaderAtConfirmedDir(
+	root, err := demandDirectoryRoot(fSys, path)
-		path, fSys, nil, git.ClonerUsingGitExec)
 	if err != nil {
 		log.Fatalf("unable to make loader at '%s'; %v", path, err)
 	}
-	return l
+	return newLoaderAtConfirmedDir(
+		root, fSys, nil, git.ClonerUsingGitExec)
 }
 
 // newLoaderAtConfirmedDir returns a new fileLoader with given root.
 func newLoaderAtConfirmedDir(
-	possibleRoot string, fSys fs.FileSystem,
+	root fs.ConfirmedDir, fSys fs.FileSystem,
-	referrer *fileLoader, cloner git.Cloner) (*fileLoader, error) {
+	referrer *fileLoader, cloner git.Cloner) *fileLoader {
-	if possibleRoot == "" {
-		return nil, fmt.Errorf(
-			"loader root cannot be empty")
-	}
-	root, f, err := fSys.CleanedAbs(possibleRoot)
-	if err != nil {
-		return nil, fmt.Errorf(
-			"absolute path error in '%s' : %v", possibleRoot, err)
-	}
-	if f != "" {
-		return nil, fmt.Errorf(
-			"got file '%s', but '%s' must be a directory to be a root",
-			f, possibleRoot)
-	}
-	if referrer != nil {
-		if err := referrer.errIfArgEqualOrHigher(root); err != nil {
-			return nil, err
-		}
-	}
 	return &fileLoader{
 		root:     root,
 		referrer: referrer,
 		fSys:     fSys,
 		cloner:   cloner,
 		cleaner:  func() error { return nil },
-	}, nil
+	}
+}
+
+// Assure that the given path is in fact a directory.
+func demandDirectoryRoot(
+	fSys fs.FileSystem, path string) (fs.ConfirmedDir, error) {
+	if path == "" {
+		return "", fmt.Errorf(
+			"loader root cannot be empty")
+	}
+	d, f, err := fSys.CleanedAbs(path)
+	if err != nil {
+		return "", fmt.Errorf(
+			"absolute path error in '%s' : %v", path, err)
+	}
+	if f != "" {
+		return "", fmt.Errorf(
+			"got file '%s', but '%s' must be a directory to be a root",
+			f, path)
+	}
+	return d, nil
 }
 
 // New returns a new Loader, rooted relative to current loader,
@@ -160,26 +161,34 @@ func (l *fileLoader) New(path string) (ifc.Loader, error) {
 	if path == "" {
 		return nil, fmt.Errorf("new root cannot be empty")
 	}
-	if git.IsRepoUrl(path) {
+	repoSpec, err := git.NewRepoSpecFromUrl(path)
-		// Avoid cycles.
+	if err == nil {
-		if err := l.errIfPreviouslySeenUri(path); err != nil {
+		// Treat this as git repo clone request.
+		if err := l.errIfRepoCycle(repoSpec); err != nil {
 			return nil, err
 		}
-		return newLoaderAtGitClone(path, l.fSys, l.referrer, l.cloner)
+		return newLoaderAtGitClone(repoSpec, l.fSys, l.referrer, l.cloner)
 	}
 	if filepath.IsAbs(path) {
 		return nil, fmt.Errorf("new root '%s' cannot be absolute", path)
 	}
+	root, err := demandDirectoryRoot(l.fSys, l.root.Join(path))
+	if err != nil {
+		return nil, err
+	}
+	if err := l.errIfArgEqualOrHigher(root); err != nil {
+		return nil, err
+	}
 	return newLoaderAtConfirmedDir(
-		l.root.Join(path), l.fSys, l, l.cloner)
+		root, l.fSys, l, l.cloner), nil
 }
 
 // newLoaderAtGitClone returns a new Loader pinned to a temporary
 // directory holding a cloned git repo.
 func newLoaderAtGitClone(
-	uri string, fSys fs.FileSystem,
+	repoSpec *git.RepoSpec, fSys fs.FileSystem,
 	referrer *fileLoader, cloner git.Cloner) (ifc.Loader, error) {
-	repoSpec, err := cloner(uri)
+	err := cloner(repoSpec)
 	if err != nil {
 		return nil, err
 	}
@@ -187,6 +196,10 @@ func newLoaderAtGitClone(
 	if err != nil {
 		return nil, err
 	}
+	// We don't know that the path requested in repoSpec
+	// is a directory until we actually clone it and look
+	// inside.  That just happened, hence the error check
+	// is here.
 	if f != "" {
 		return nil, fmt.Errorf(
 			"'%s' refers to file '%s'; expecting directory",
@@ -221,17 +234,17 @@ func (l *fileLoader) errIfArgEqualOrHigher(
 // I.e. Allow a distinction between git URI with
 // path foo and tag bar and a git URI with the same
 // path but a different tag?
-// TODO(monopole): Use parsed data instead of looking at Raw().
+func (l *fileLoader) errIfRepoCycle(newRepoSpec *git.RepoSpec) error {
-func (l *fileLoader) errIfPreviouslySeenUri(uri string) error {
+	// TODO(monopole): Use parsed data instead of Raw().
-	if strings.HasPrefix(l.repoSpec.Raw(), uri) {
+	if strings.HasPrefix(l.repoSpec.Raw(), newRepoSpec.Raw()) {
 		return fmt.Errorf(
 			"cycle detected: URI '%s' referenced by previous URI '%s'",
-			uri, l.repoSpec.Raw())
+			newRepoSpec.Raw(), l.repoSpec.Raw())
 	}
 	if l.referrer == nil {
 		return nil
 	}
-	return l.referrer.errIfPreviouslySeenUri(uri)
+	return l.referrer.errIfRepoCycle(newRepoSpec)
 }
 
 // Load returns content of file at the given relative path,

pkg/loader/fileloader_test.go

@@ -64,25 +64,6 @@ func MakeFakeFs(td []testData) fs.FileSystem {
 	return fSys
 }
 
-func TestNewLoaderAtConfirmedDir_DemandsDirectory(t *testing.T) {
-	fSys := MakeFakeFs(testCases)
-	_, err := newLoaderAtConfirmedDir("/foo", fSys, nil, nil)
-	if err != nil {
-		t.Fatalf("Unexpected error - a directory should work.")
-	}
-	_, err = newLoaderAtConfirmedDir("/foo/project", fSys, nil, nil)
-	if err != nil {
-		t.Fatalf("Unexpected error - a directory should work.")
-	}
-	_, err = newLoaderAtConfirmedDir("/foo/project/fileA.yaml", fSys, nil, nil)
-	if err == nil {
-		t.Fatalf("Expected error - a file should not work.")
-	}
-	if !strings.Contains(err.Error(), "must be a directory to be a root") {
-		t.Fatalf("unexpected err: %v", err)
-	}
-}
-
 func TestLoaderLoad(t *testing.T) {
 	l1 := NewFileLoaderAtRoot(MakeFakeFs(testCases))
 	if "/" != l1.Root() {
@@ -351,23 +332,6 @@ func TestSplit(t *testing.T) {
 	}
 }
 
-// makeFakeGitCloner returns a cloner that ignores the
-// URL argument and returns a path in a fake file system
-// that should already hold the 'repo' contents.
-func makeFakeGitCloner(t *testing.T, fSys fs.FileSystem, coRoot string) git.Cloner {
-	if !fSys.IsDir(coRoot) {
-		t.Fatalf("expecting a directory at '%s'", coRoot)
-	}
-	return func(url string) (*git.RepoSpec, error) {
-		_, path := splitOnNthSlash(url, 3)
-		if !fSys.IsDir(coRoot + "/" + path) {
-			t.Fatalf("expecting a directory at '%s'/'%s'",
-				coRoot, path)
-		}
-		return git.NewRepoSpec(url, fs.ConfirmedDir(coRoot), path), nil
-	}
-}
-
 func TestNewLoaderAtGitClone(t *testing.T) {
 	rootUrl := "github.com/someOrg/someRepo"
 	pathInRepo := "foo/base"
@@ -385,9 +349,14 @@ func TestNewLoaderAtGitClone(t *testing.T) {
 		[]byte(`
 whatever
 `))
+
+	repoSpec, err := git.NewRepoSpecFromUrl(url)
+	if err != nil {
+		t.Fatalf("unexpected err: %v\n", err)
+	}
 	l, err := newLoaderAtGitClone(
-		url, fSys, nil,
+		repoSpec, fSys, nil,
-		makeFakeGitCloner(t, fSys, coRoot))
+		git.DoNothingCloner(fs.ConfirmedDir(coRoot)))
 	if err != nil {
 		t.Fatalf("unexpected err: %v\n", err)
 	}

pkg/loader/loader.go

@@ -24,11 +24,16 @@ import (
 )
 
 // NewLoader returns a Loader.
-func NewLoader(root string, fSys fs.FileSystem) (ifc.Loader, error) {
+func NewLoader(path string, fSys fs.FileSystem) (ifc.Loader, error) {
-	if git.IsRepoUrl(root) {
+	repoSpec, err := git.NewRepoSpecFromUrl(path)
+	if err == nil {
 		return newLoaderAtGitClone(
-			root, fSys, nil, git.ClonerUsingGitExec)
+			repoSpec, fSys, nil, git.ClonerUsingGitExec)
+	}
+	root, err := demandDirectoryRoot(fSys, path)
+	if err != nil {
+		return nil, err
 	}
 	return newLoaderAtConfirmedDir(
-		root, fSys, nil, git.ClonerUsingGitExec)
+		root, fSys, nil, git.ClonerUsingGitExec), nil
 }