Merge pull request #737 from monopole/moreGitUrlParsingCoverage

Add better error reporting to NewRepoSpecFromUrl

pkg/git/cloner.go

@@ -46,7 +46,7 @@ func ClonerUsingGitExec(spec string) (*RepoSpec, error) {
 	cmd := exec.Command(
 		gitProgram,
 		"clone",
-		repoSpec.repo,
+		repoSpec.CloneSpec(),
 		repoSpec.cloneDir.String())
 	var out bytes.Buffer
 	cmd.Stdout = &out

pkg/git/repospec.go

@@ -17,27 +17,30 @@ limitations under the License.
 package git
 
 import (
+	"fmt"
 	"path/filepath"
-	"regexp"
 	"strings"
 
-	"github.com/pkg/errors"
 	"sigs.k8s.io/kustomize/pkg/fs"
 )
 
-// RepoSpec specifies a git repo and a branch and path therein.
+// RepoSpec specifies a git repository and a branch and path therein.
 type RepoSpec struct {
 	// Raw, original spec, used to look for cycles.
 	// TODO(monopole): Drop raw, use processed fields instead.
 	raw string
 
-	// Repo, e.g. github.com/kubernetes-sigs/kustomize
+	// Host, e.g. github.com
-	repo string
+	host string
 
-	// ConfirmedDir where the repo is cloned to.
+	// orgRepo name (organization/repoName),
+	// e.g. kubernetes-sigs/kustomize
+	orgRepo string
+
+	// ConfirmedDir where the orgRepo is cloned to.
 	cloneDir fs.ConfirmedDir
 
-	// Relative path in the repo, and in the cloneDir,
+	// Relative path in the repository, and in the cloneDir,
 	// to a Kustomization.
 	path string
 
@@ -45,6 +48,14 @@ type RepoSpec struct {
 	ref string
 }
 
+// CloneSpec returns a string suitable for "git clone {spec}".
+func (x *RepoSpec) CloneSpec() string {
+	if isAzureHost(x.host) || isAWSHost(x.host) {
+		return x.host + x.orgRepo
+	}
+	return x.host + x.orgRepo + gitSuffix
+}
+
 func (x *RepoSpec) CloneDir() fs.ConfirmedDir {
 	return x.cloneDir
 }
@@ -74,17 +85,23 @@ func IsRepoUrl(arg string) bool {
 			isAzureHost(arg) || isAWSHost(arg))
 }
 
+// From strings like git@github.com:someOrg/someRepo.git or
+// https://github.com/someOrg/someRepo?ref=someHash, extract
+// the parts.
 func NewRepoSpecFromUrl(n string) (*RepoSpec, error) {
-	host, repo, path, gitRef, err := parseGithubUrl(n)
+	if filepath.IsAbs(n) {
-	if err != nil {
+		return nil, fmt.Errorf("uri looks like abs path: %s", n)
-		return nil, err
 	}
-	if isAzureHost(host) || isAWSHost(host) {
+	host, orgRepo, path, gitRef := parseGithubUrl(n)
-		repo = host + repo
+	if orgRepo == "" {
-	} else {
+		return nil, fmt.Errorf("url lacks orgRepo: %s", n)
-		repo = host + repo + ".git"
 	}
-	return &RepoSpec{raw: n, repo: repo, path: path, ref: gitRef}, nil
+	if host == "" {
+		return nil, fmt.Errorf("url lacks host: %s", n)
+	}
+	return &RepoSpec{
+		raw: n, host: host, orgRepo: orgRepo,
+		path: path, ref: gitRef}, nil
 }
 
 func NewRepoSpec(
@@ -101,33 +118,29 @@ const (
 // https://github.com/someOrg/someRepo?ref=someHash, extract
 // the parts.
 func parseGithubUrl(n string) (
-	host string, repo string, path string, gitRef string, err error) {
+	host string, orgRepo string, path string, gitRef string) {
 	host, n = parseHostSpec(n)
-	host = normalizeGitHostSpec(host)
 
-	if strings.HasSuffix(n, gitSuffix) {
-		repo = n[0 : len(n)-len(gitSuffix)]
-		return
-	}
 	if strings.Contains(n, gitSuffix) {
 		index := strings.Index(n, gitSuffix)
-		repo = n[0:index]
+		orgRepo = n[0:index]
 		n = n[index+len(gitSuffix):]
 		path, gitRef = peelQuery(n)
 		return
 	}
+
 	i := strings.Index(n, "/")
 	if i < 1 {
-		return "", "", "", "", errors.New("no separator")
+		return "", "", "", ""
 	}
 	j := strings.Index(n[i+1:], "/")
 	if j >= 0 {
 		j += i + 1
-		repo = n[:j]
+		orgRepo = n[:j]
 		path, gitRef = peelQuery(n[j+1:])
 	} else {
 		path = ""
-		repo, gitRef = peelQuery(n)
+		orgRepo, gitRef = peelQuery(n)
 	}
 	return
 }
@@ -142,28 +155,45 @@ func peelQuery(arg string) (string, string) {
 
 func parseHostSpec(n string) (string, string) {
 	var host string
+	// Start accumulating the host part.
 	for _, p := range []string{
 		// Order matters here.
 		"git::", "gh:", "ssh://", "https://", "http://",
 		"git@", "github.com:", "github.com/"} {
-		if strings.ToLower(n[:len(p)]) == p {
+		if len(p) < len(n) && strings.ToLower(n[:len(p)]) == p {
 			n = n[len(p):]
-			host = host + p
+			host += p
 		}
 	}
+	if host == "git@" {
+		i := strings.Index(n, "/")
+		if i > -1 {
+			host += n[:i+1]
+			n = n[i+1:]
+		} else {
+			i = strings.Index(n, ":")
+			if i > -1 {
+				host += n[:i+1]
+				n = n[i+1:]
+			}
+		}
+		return host, n
+	}
 
 	// If host is a http(s) or ssh URL, grab the domain part.
 	for _, p := range []string{
 		"ssh://", "https://", "http://"} {
-		if strings.HasSuffix(strings.ToLower(host), p) {
+		if strings.HasSuffix(host, p) {
-			index := regexp.MustCompile("^(.*?)/").FindStringIndex(n)
+			i := strings.Index(n, "/")
-			if len(index) > 0 {
+			if i > -1 {
-				host = host + n[0:index[len(index)-1]]
+				host = host + n[0:i+1]
-				n = n[index[len(index)-1]:]
+				n = n[i+1:]
 			}
+			break
 		}
 	}
-	return host, n
+
+	return normalizeGitHostSpec(host), n
 }
 
 func normalizeGitHostSpec(host string) string {

pkg/git/repospec_test.go

@@ -19,6 +19,7 @@ package git
 import (
 	"fmt"
 	"path/filepath"
+	"strings"
 	"testing"
 )
 
@@ -101,15 +102,16 @@ func TestIsRepoURL(t *testing.T) {
 	}
 }
 
-var repoNames = []string{"someOrg/someRepo", "kubernetes/website"}
+var orgRepos = []string{"someOrg/someRepo", "kubernetes/website"}
 
-var paths = []string{"README.md", "foo/krusty.txt", ""}
+var pathNames = []string{"README.md", "foo/krusty.txt", ""}
 
-var hrefArgs = []string{"someBranch", ""}
+var hrefArgs = []string{"someBranch", "master", "v0.1.0", ""}
 
-var extractFmts = map[string]string{
+var hostNamesRawAndNormalizedOld = map[string]string{
 	"gh:%s":                           "gh:",
 	"GH:%s":                           "gh:",
+	"git@github.com:%s":               "git@github.com:",
 	"gitHub.com/%s":                   "https://github.com/",
 	"https://github.com/%s":           "https://github.com/",
 	"hTTps://github.com/%s":           "https://github.com/",
@@ -118,40 +120,132 @@ var extractFmts = map[string]string{
 	"git::http://git.example.com/%s":  "http://git.example.com/",
 	"git::https://git.example.com/%s": "https://git.example.com/",
 	"ssh://git.example.com:7999/%s":   "ssh://git.example.com:7999/",
+	"https://git-codecommit.us-east-2.amazonaws.com/%s": "https://git-codecommit.us-east-2.amazonaws.com/",
 }
 
-func TestParseGithubUrl(t *testing.T) {
+var hostNamesRawAndNormalized = [][]string{
-	for _, repoName := range repoNames {
+	{"gh:", "gh:"},
-		for _, pathName := range paths {
+	{"GH:", "gh:"},
-			for extractFmt, hostSpec := range extractFmts {
+	{"gitHub.com/", "https://github.com/"},
+	{"github.com:", "https://github.com/"},
+	{"http://github.com/", "https://github.com/"},
+	{"https://github.com/", "https://github.com/"},
+	{"hTTps://github.com/", "https://github.com/"},
+	{"https://git-codecommit.us-east-2.amazonaws.com/", "https://git-codecommit.us-east-2.amazonaws.com/"},
+	{"https://fabrikops2.visualstudio.com/", "https://fabrikops2.visualstudio.com/"},
+	{"ssh://git.example.com:7999/", "ssh://git.example.com:7999/"},
+	{"git::https://gitlab.com/", "https://gitlab.com/"},
+	{"git::http://git.example.com/", "http://git.example.com/"},
+	{"git::https://git.example.com/", "https://git.example.com/"},
+	{"git@github.com:", "git@github.com:"},
+	{"git@github.com/", "git@github.com:"},
+	{"git@gitlab2.sqtools.ru:10022/", "git@gitlab2.sqtools.ru:10022/"},
+}
+
+func makeUrlOld(hostFmt, orgRepo, path, href string) string {
+	if len(path) > 0 {
+		orgRepo = filepath.Join(orgRepo, path)
+	}
+	url := fmt.Sprintf(hostFmt, orgRepo)
+	if href != "" {
+		url += refQuery + href
+	}
+	return url
+}
+
+func makeUrl(hostFmt, orgRepo, path, href string) string {
+	if len(path) > 0 {
+		orgRepo = filepath.Join(orgRepo, path)
+	}
+	url := hostFmt + orgRepo
+	if href != "" {
+		url += refQuery + href
+	}
+	return url
+}
+
+func TestNewRepoSpecFromUrl(t *testing.T) {
+	var bad [][]string
+	for _, tuple := range hostNamesRawAndNormalized {
+		hostRaw := tuple[0]
+		hostSpec := tuple[1]
+		for _, orgRepo := range orgRepos {
+			for _, pathName := range pathNames {
 				for _, hrefArg := range hrefArgs {
-					spec := repoName
+					uri := makeUrl(hostRaw, orgRepo, pathName, hrefArg)
-					if len(pathName) > 0 {
+					rs, err := NewRepoSpecFromUrl(uri)
-						spec = filepath.Join(spec, pathName)
-					}
-					input := fmt.Sprintf(extractFmt, spec)
-					if hrefArg != "" {
-						input = input + refQuery + hrefArg
-					}
-					if !IsRepoUrl(input) {
-						t.Errorf("Should smell like github arg: %s\n", input)
-						continue
-					}
-					host, repo, path, gitRef, err := parseGithubUrl(input)
 					if err != nil {
 						t.Errorf("problem %v", err)
 					}
+					if rs.host != hostSpec {
+						bad = append(bad, []string{"host", uri, rs.host, hostSpec})
+					}
+					if rs.orgRepo != orgRepo {
+						bad = append(bad, []string{"orgRepo", uri, rs.orgRepo, orgRepo})
+					}
+					if rs.path != pathName {
+						bad = append(bad, []string{"path", uri, rs.path, pathName})
+					}
+					if rs.ref != hrefArg {
+						bad = append(bad, []string{"ref", uri, rs.ref, hrefArg})
+					}
+				}
+			}
+		}
+	}
+	if len(bad) > 0 {
+		for _, tuple := range bad {
+			fmt.Printf("\n"+
+				"     from uri: %s\n"+
+				"  actual %4s: %s\n"+
+				"expected %4s: %s\n",
+				tuple[1], tuple[0], tuple[2], tuple[0], tuple[3])
+		}
+		t.Fail()
+	}
+}
+
+var badData = [][]string{
+	{"/tmp", "uri looks like abs path"},
+	{"iauhsdiuashduas", "url lacks orgRepo"},
+	{"htxxxtp://github.com/", "url lacks host"},
+	{"ssh://git.example.com", "url lacks orgRepo"},
+	{"git::___", "url lacks orgRepo"},
+}
+
+func TestNewRepoSpecFromUrlErrors(t *testing.T) {
+	for _, tuple := range badData {
+		_, err := NewRepoSpecFromUrl(tuple[0])
+		if err == nil {
+			t.Error("expected error")
+		}
+		if !strings.Contains(err.Error(), tuple[1]) {
+			t.Errorf("unexpected error: %s", err)
+		}
+	}
+}
+
+func TestParseGithubUrl(t *testing.T) {
+	for hostRawFmt, hostSpec := range hostNamesRawAndNormalizedOld {
+		for _, orgRepo := range orgRepos {
+			for _, pathName := range pathNames {
+				for _, hrefArg := range hrefArgs {
+					input := makeUrlOld(hostRawFmt, orgRepo, pathName, hrefArg)
+					if !IsRepoUrl(input) {
+						t.Errorf("Should smell like github arg: %s\n", input)
+					}
+					host, repo, path, gitRef := parseGithubUrl(input)
 					if host != hostSpec {
 						t.Errorf("\n"+
 							"         from %s\n"+
 							"  actual host %s\n"+
 							"expected host %s\n", input, host, hostSpec)
 					}
-					if repo != repoName {
+					if repo != orgRepo {
 						t.Errorf("\n"+
 							"         from %s\n"+
 							"  actual Repo %s\n"+
-							"expected Repo %s\n", input, repo, repoName)
+							"expected Repo %s\n", input, repo, orgRepo)
 					}
 					if path != pathName {
 						t.Errorf("\n"+
@@ -171,7 +265,7 @@ func TestParseGithubUrl(t *testing.T) {
 	}
 }
 
-func TestNewRepoSpecFromUrl(t *testing.T) {
+func TestNewRepoSpecFromUrlOld(t *testing.T) {
 	testcases := []struct {
 		input string
 		repo  string
@@ -220,9 +314,9 @@ func TestNewRepoSpecFromUrl(t *testing.T) {
 		if err != nil {
 			t.Errorf("Unexpected error: %v", err)
 		}
-		if rs.repo != testcase.repo {
+		if rs.CloneSpec() != testcase.repo {
-			t.Errorf("repo expected to be %v, but got %v on %s",
+			t.Errorf("CloneSpec expected to be %v, but got %v on %s",
-				testcase.repo, rs.repo, testcase.input)
+				testcase.repo, rs.CloneSpec(), testcase.input)
 		}
 		if rs.path != testcase.path {
 			t.Errorf("path expected to be %v, but got %v on %s",