refactor: introduce a struct for git repository URL encapsulation

This will make it easier to extend the functionality to be able to use subdirectories for charts

Signed-off-by: Dominykas Blyžė <[email protected]>

Author: dominykas

internal/gitutil/gitutil.go

@@ -17,15 +17,24 @@ limitations under the License.
 package gitutil
 
 import (
+	"net/url"
 	"os"
 	"regexp"
 	"strings"
 
+	"github.com/pkg/errors"
+	giturls "github.com/whilp/git-urls"
+
 	"github.com/Masterminds/vcs"
 )
 
 var gitRepositoryURLRe = regexp.MustCompile(`^git(\+\w+)?://`)
 
+type GitRepositoryURL struct {
+	RepositoryURL string
+	GitRemoteURL  *url.URL
+}
+
 // HasGitReference returns true if a git repository contains a specified ref (branch/tag)
 func HasGitReference(gitRepo, ref string) (bool, error) {
 	local, err := os.MkdirTemp("", "helm-git-")
@@ -50,7 +59,20 @@ func IsGitRepository(url string) bool {
 	return gitRepositoryURLRe.MatchString(url)
 }
 
-// RepositoryURLToGitURL converts a repository URL into a URL that `git clone` could consume
-func RepositoryURLToGitURL(url string) string {
-	return strings.TrimPrefix(url, "git+")
+// ParseGitRepositoryURL creates a new GitRepositoryURL from a string
+func ParseGitRepositoryURL(repositoryURL string) (GitRepositoryURL, error) {
+	gitRemoteURL, err := giturls.Parse(strings.TrimPrefix(repositoryURL, "git+"))
+
+	if err != nil {
+		return GitRepositoryURL{}, err
+	}
+
+	if gitRemoteURL.User != nil {
+		return GitRepositoryURL{}, errors.Errorf("git repository URL should not contain credentials - please use git credential helpers")
+	}
+
+	return GitRepositoryURL{
+		RepositoryURL: repositoryURL,
+		GitRemoteURL:  gitRemoteURL,
+	}, err
 }

internal/gitutil/gitutil_test.go

@@ -38,19 +38,32 @@ func TestIsGitUrl(t *testing.T) {
 	}
 }
 
-func TestRepositoryURLToGitURL(t *testing.T) {
-	// Test table: Given url, RepositoryURLToGitURL should return expect.
+func TestParseGitRepositoryURL(t *testing.T) {
+	// Test table: Given url, ParseGitRepositoryURL should return expect.
 	tests := []struct {
-		url    string
-		expect string
+		url                    string
+		expectRepositoryURL    string
+		expectGitRepositoryURL string
 	}{
-		{"git://example.com/example/chart", "git://example.com/example/chart"},
-		{"git+https://example.com/example/chart", "https://example.com/example/chart"},
+		{
+			url:                    "git://example.com/example/chart",
+			expectRepositoryURL:    "git://example.com/example/chart",
+			expectGitRepositoryURL: "git://example.com/example/chart",
+		},
+		{
+			url:                    "git+https://example.com/example/chart",
+			expectRepositoryURL:    "git+https://example.com/example/chart",
+			expectGitRepositoryURL: "https://example.com/example/chart",
+		},
 	}
 
 	for _, test := range tests {
-		if RepositoryURLToGitURL(test.url) != test.expect {
-			t.Errorf("Expected %s for %s", test.expect, test.url)
+		parsed, _ := ParseGitRepositoryURL(test.url)
+		if parsed.RepositoryURL != test.expectRepositoryURL {
+			t.Errorf("Expected RepositoryURL %s for %s, but got %s", test.expectRepositoryURL, test.url, parsed)
+		}
+		if parsed.GitRemoteURL.String() != test.expectGitRepositoryURL {
+			t.Errorf("Expected GitRemoteURL %s for %s, but got %s", test.expectGitRepositoryURL, test.url, parsed)
 		}
 	}
 }

internal/resolver/resolver.go

@@ -117,8 +117,12 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string
 
 		if gitutil.IsGitRepository(d.Repository) {
 
-			found, err := hasGitReference(gitutil.RepositoryURLToGitURL(d.Repository), d.Version)
+			gitURL, err := gitutil.ParseGitRepositoryURL(d.Repository)
+			if err != nil {
+				return nil, err
+			}
 
+			found, err := hasGitReference(gitURL.GitRemoteURL.String(), d.Version)
 			if err != nil {
 				return nil, err
 			}

pkg/downloader/chart_downloader.go

@@ -26,8 +26,6 @@ import (
 	"github.com/Masterminds/semver/v3"
 	"github.com/pkg/errors"
 
-	giturls "github.com/whilp/git-urls"
-
 	"helm.sh/helm/v3/internal/fileutil"
 	"helm.sh/helm/v3/internal/gitutil"
 	"helm.sh/helm/v3/internal/urlutil"
@@ -210,14 +208,11 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, er
 	}
 
 	if gitutil.IsGitRepository(ref) {
-		gitURL, gitURLErr := giturls.Parse(gitutil.RepositoryURLToGitURL(ref))
-		if gitURLErr != nil {
-			return nil, errors.Errorf("invalid git URL format: %s", ref)
-		}
-		if gitURL.User != nil {
-			return nil, errors.Errorf("git repository URL should not contain credentials - please use git credential helpers")
+		_, err := gitutil.ParseGitRepositoryURL(ref)
+		if err != nil {
+			return nil, errors.Wrapf(err, "invalid git URL format: %s", ref)
 		}
-		return u, nil
+		return u, err
 	}
 
 	rf, err := loadRepoConfig(c.RepositoryConfig)

pkg/downloader/chart_downloader_test.go

@@ -43,7 +43,7 @@ func TestResolveChartRef(t *testing.T) {
 		{name: "full URL, with authentication", ref: "http://username:[email protected]/foo-1.2.3.tgz", expect: "http://username:[email protected]/foo-1.2.3.tgz"},
 		{name: "helmchart", ref: "git+https://github.com/helmchart/helmchart.git", expect: "git+https://github.com/helmchart/helmchart.git"},
 		{name: "helmchart", ref: "git://github.com/helmchart/helmchart.git", expect: "git://github.com/helmchart/helmchart.git"},
-		{name: "helmchart", ref: "git+https://username:[email protected]/helmchart/helmchart.git", expectError: "git repository URL should not contain credentials - please use git credential helpers"},
+		{name: "helmchart", ref: "git+https://username:[email protected]/helmchart/helmchart.git", expectError: "invalid git URL format: git+https://username:[email protected]/helmchart/helmchart.git: git repository URL should not contain credentials - please use git credential helpers"},
 		{name: "reference, testing repo", ref: "testing/alpine", expect: "http://example.com/alpine-1.2.3.tgz"},
 		{name: "reference, version, testing repo", ref: "testing/alpine", version: "0.2.0", expect: "http://example.com/alpine-0.2.0.tgz"},
 		{name: "reference, version, malformed repo", ref: "malformed/alpine", version: "1.2.3", expect: "http://dl.example.com/alpine-1.2.3.tgz"},

pkg/downloader/manager.go

@@ -610,9 +610,6 @@ func (m *Manager) resolveRepoNames(deps []*chart.Dependency) (map[string]string,
 		// if the repo does not exist then it will later error when we try to fetch branches and tags.
 		// we could check for the repo existence here, but trying to avoid another git request.
 		if gitutil.IsGitRepository(dd.Repository) {
-			if m.Debug {
-				fmt.Fprintf(m.Out, "Repository from git url: %s\n", strings.TrimPrefix(dd.Repository, "git:"))
-			}
 			reposMap[dd.Name] = dd.Repository
 			continue
 		}

pkg/getter/gitgetter.go

@@ -47,7 +47,10 @@ func (g *GitGetter) Get(href string, options ...Option) (*bytes.Buffer, error) {
 }
 
 func (g *GitGetter) get(href string) (*bytes.Buffer, error) {
-	gitURL := gitutil.RepositoryURLToGitURL(href)
+	gitURL, err := gitutil.ParseGitRepositoryURL(href)
+	if err != nil {
+		return nil, err
+	}
 	version := g.opts.version
 	chartName := g.opts.chartName
 	if version == "" {
@@ -64,7 +67,7 @@ func (g *GitGetter) get(href string) (*bytes.Buffer, error) {
 	}
 	defer os.RemoveAll(tmpDir)
 
-	repo, err := vcs.NewRepo(gitURL, chartTmpDir)
+	repo, err := vcs.NewRepo(gitURL.GitRemoteURL.String(), chartTmpDir)
 	if err != nil {
 		return nil, err
 	}