Improve readability of ifc.Loader's Repo() method (#4857)

* Improve ldr Repo() method readability

* Change Repo() implementations and calls

* Improve readability of conditions in ldr.New()

* Fix details

api/ifc/ifc.go

@@ -29,9 +29,9 @@ type KvLoader interface {
 // Loader interface exposes methods to read bytes.
 type Loader interface {
 
-	// Repo returns the repo location and true if this Loader
-	// was created from a url; otherwise the empty string and false.
-	Repo() (string, bool)
+	// Repo returns the repo location if this Loader was created from a url
+	// or the empty string otherwise.
+	Repo() string
 
 	// Root returns the root location for this Loader.
 	Root() string

api/internal/localizer/locloader.go

@@ -116,18 +116,12 @@ func (ll *locLoader) Load(path string) ([]byte, error) {
 // New returns a Loader at path if path is a valid localize root.
 // Otherwise, New returns an error.
 func (ll *locLoader) New(path string) (ifc.Loader, error) {
-	repoSpec, err := git.NewRepoSpecFromURL(path)
-	if err == nil && repoSpec.Ref == "" {
-		return nil, errors.Errorf("localize remote root '%s' missing ref query string parameter", path)
-	}
-
 	ldr, err := ll.Loader.New(path)
 	if err != nil {
 		return nil, errors.WrapPrefixf(err, "invalid root reference")
 	}
 
-	var isRemote bool
-	if _, isRemote = ldr.Repo(); !isRemote {
+	if repo := ldr.Repo(); repo == "" {
 		if ll.local && !filesys.ConfirmedDir(ldr.Root()).HasPrefix(ll.args.Scope) {
 			return nil, errors.Errorf("root '%s' outside localize scope '%s'", ldr.Root(), ll.args.Scope)
 		}
@@ -135,12 +129,14 @@ func (ll *locLoader) New(path string) (ifc.Loader, error) {
 			return nil, errors.Errorf(
 				"root '%s' references into localize destination '%s'", ldr.Root(), ll.args.NewDir)
 		}
+	} else if !hasRef(path) {
+		return nil, errors.Errorf("localize remote root %q missing ref query string parameter", path)
 	}
 
 	return &locLoader{
 		fSys:   ll.fSys,
 		args:   ll.args,
 		Loader: ldr,
-		local:  ll.local && !isRemote,
+		local:  ll.local && ldr.Repo() == "",
 	}, nil
 }

api/internal/localizer/locloader_test.go

@@ -38,9 +38,7 @@ func checkNewLocLoader(req *require.Assertions, ldr ifc.Loader, args *lclzr.LocA
 
 func checkLoader(req *require.Assertions, ldr ifc.Loader, root string) {
 	req.Equal(root, ldr.Root())
-	repo, isRemote := ldr.Repo()
-	req.Equal(false, isRemote)
-	req.Equal("", repo)
+	req.Empty(ldr.Repo())
 }
 
 func checkLocArgs(req *require.Assertions, args *lclzr.LocArgs, target string, scope string, newDir string, fSys filesys.FileSystem) {

api/internal/localizer/util.go

@@ -16,7 +16,7 @@ import (
 
 // establishScope returns the scope given localize arguments and targetLdr at targetArg
 func establishScope(scopeArg string, targetArg string, targetLdr ifc.Loader, fSys filesys.FileSystem) (filesys.ConfirmedDir, error) {
-	if _, isRemote := targetLdr.Repo(); isRemote {
+	if targetLdr.Repo() != "" {
 		if scopeArg != "" {
 			return "", errors.Errorf("scope '%s' specified for remote localize target '%s'", scopeArg, targetArg)
 		}
@@ -65,7 +65,7 @@ func createNewDir(newDirArg string, targetLdr ifc.Loader, spec *git.RepoSpec, fS
 // and spec of target, which is nil if target is local
 func defaultNewDir(targetLdr ifc.Loader, spec *git.RepoSpec) string {
 	targetDir := filepath.Base(targetLdr.Root())
-	if repo, isRemote := targetLdr.Repo(); isRemote {
+	if repo := targetLdr.Repo(); repo != "" {
 		// kustomize doesn't download repo into repo-named folder
 		// must find repo folder name from url
 		if repo == targetLdr.Root() {
@@ -89,3 +89,12 @@ func urlBase(url string) string {
 	}
 	return cleaned[i+1:]
 }
+
+// hasRef checks if repoURL has ref query string parameter
+func hasRef(repoURL string) bool {
+	repoSpec, err := git.NewRepoSpecFromURL(repoURL)
+	if err != nil {
+		log.Fatalf("unable to parse validated root url: %s", err.Error())
+	}
+	return repoSpec.Ref != ""
+}

api/loader/fileloader.go

@@ -121,13 +121,13 @@ func NewFileLoaderAtRoot(fSys filesys.FileSystem) *fileLoader {
 		RestrictionRootOnly, fSys, filesys.Separator)
 }
 
-// Repo returns the absolute path to the repo that contains Root and true
-// if this fileLoader was created from a url; otherwise, the empty string and false
-func (fl *fileLoader) Repo() (string, bool) {
+// Repo returns the absolute path to the repo that contains Root if this fileLoader was created from a url
+// or the empty string otherwise.
+func (fl *fileLoader) Repo() string {
 	if fl.repoSpec != nil {
-		return fl.repoSpec.Dir.String(), true
+		return fl.repoSpec.Dir.String()
 	}
-	return "", false
+	return ""
 }
 
 // Root returns the absolute path that is prepended to any

api/loader/fileloader_test.go

@@ -95,8 +95,8 @@ func TestLoaderLoad(t *testing.T) {
 	require := require.New(t)
 
 	l1 := makeLoader()
-	_, remote := l1.Repo()
-	require.False(remote)
+	repo := l1.Repo()
+	require.Empty(repo)
 	require.Equal("/", l1.Root())
 
 	for _, x := range testCases {
@@ -110,8 +110,8 @@ func TestLoaderLoad(t *testing.T) {
 	l2, err := l1.New("foo/project")
 	require.NoError(err)
 
-	_, remote = l2.Repo()
-	require.False(remote)
+	repo = l2.Repo()
+	require.Empty(repo)
 	require.Equal("/foo/project", l2.Root())
 
 	for _, x := range testCases {
@@ -361,8 +361,7 @@ whatever
 		repoSpec, fSys, nil,
 		git.DoNothingCloner(filesys.ConfirmedDir(coRoot)))
 	require.NoError(err)
-	repo, remote := l.Repo()
-	require.True(remote)
+	repo := l.Repo()
 	require.Equal(coRoot, repo)
 	require.Equal(coRoot+"/"+pathInRepo, l.Root())
 
@@ -378,8 +377,7 @@ whatever
 	l2, err := l.New(url)
 	require.NoError(err)
 
-	repo, remote = l2.Repo()
-	require.True(remote)
+	repo = l2.Repo()
 	require.Equal(coRoot, repo)
 	require.Equal(coRoot+"/"+pathInRepo, l2.Root())
 }
@@ -435,8 +433,8 @@ func TestLoaderDisallowsLocalBaseFromRemoteOverlay(t *testing.T) {
 	// This is okay.
 	l2, err = l1.New("../base")
 	require.NoError(err)
-	_, remote := l2.Repo()
-	require.False(remote)
+	repo := l2.Repo()
+	require.Empty(repo)
 	require.Equal(cloneRoot+"/foo/base", l2.Root())
 
 	// This is not okay.
@@ -462,8 +460,7 @@ func TestLocalLoaderReferencingGitBase(t *testing.T) {
 
 	l2, err := l1.New("github.com/someOrg/someRepo/foo/base")
 	require.NoError(err)
-	repo, remote := l2.Repo()
-	require.True(remote)
+	repo := l2.Repo()
 	require.Equal(cloneRoot, repo)
 	require.Equal(cloneRoot+"/foo/base", l2.Root())
 }

kustomize/commands/internal/util/util_test.go

@@ -102,8 +102,8 @@ type fakeLoader struct {
 	path string
 }
 
-func (l fakeLoader) Repo() (string, bool) {
-	return "", false
+func (l fakeLoader) Repo() string {
+	return ""
 }
 func (l fakeLoader) Root() string {
 	return ""