refactor: consolidate git:// url normalization

Addresses helm#11258 (comment) Addresses helm#11258 (comment) Signed-off-by: Dominykas Blyžė <hello@dominykas.com>
dominykas · Feb 6, 2024 · 7960f12 · 7960f12
1 parent f28781f
commit 7960f12
Show file tree

Hide file tree

Showing 6 changed files with 21 additions and 17 deletions.
diff --git a/internal/gitutil/gitutil.go b/internal/gitutil/gitutil.go
@@ -23,6 +23,7 @@ import (
 	"github.com/Masterminds/vcs"
 )
 
+// HasGitReference returns true if a git repository contains a specified ref (branch/tag)
 func HasGitReference(gitRepo, ref, repoName string) (bool, error) {
 	local, err := os.MkdirTemp("", repoName)
 	if err != nil {
@@ -41,7 +42,12 @@ func HasGitReference(gitRepo, ref, repoName string) (bool, error) {
 	return repo.IsReference(ref), nil
 }
 
-func IsGitURL(url string) bool {
-
+// IsGitRepository determines whether a URL is to be treated as a git repository URL
+func IsGitRepository(url string) bool {
 	return strings.HasPrefix(url, "git://")
 }
+
+// RepositoryURLToGitURL converts a repository URL into a URL that `git clone` could consume
+func RepositoryURLToGitURL(url string) string {
+	return strings.TrimPrefix(url, "git://")
+}
diff --git a/internal/gitutil/gitutil_test.go b/internal/gitutil/gitutil_test.go
@@ -21,7 +21,7 @@ import (
 )
 
 func TestIsGitUrl(t *testing.T) {
-	// Test table: Given url, IsGitURL should return expect.
+	// Test table: Given url, IsGitRepository should return expect.
 	tests := []struct {
 		url    string
 		expect bool
@@ -32,7 +32,7 @@ func TestIsGitUrl(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		if IsGitURL(test.url) != test.expect {
+		if IsGitRepository(test.url) != test.expect {
 			t.Errorf("Expected %t for %s", test.expect, test.url)
 		}
 	}

diff --git a/internal/resolver/resolver.go b/internal/resolver/resolver.go
@@ -110,9 +110,9 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string
 			continue
 		}
 
-		if gitutil.IsGitURL(d.Repository) {
+		if gitutil.IsGitRepository(d.Repository) {
 
-			found, err := hasGitReference(strings.TrimPrefix(d.Repository, "git://"), d.Version, d.Name)
+			found, err := hasGitReference(gitutil.RepositoryURLToGitURL(d.Repository), d.Version, d.Name)
 
 			if err != nil {
 				return nil, err

diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go
@@ -97,7 +97,7 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven
 
 	scheme := ""
 
-	if gitutil.IsGitURL(ref) {
+	if gitutil.IsGitRepository(ref) {
 		scheme = "git"
 	} else {
 		scheme = u.Scheme
@@ -114,7 +114,7 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven
 	}
 
 	name := filepath.Base(u.Path)
-	if u.Scheme == registry.OCIScheme || u.Scheme == "git" {
+	if u.Scheme == registry.OCIScheme {
 		idx := strings.LastIndexByte(name, ':')
 		name = fmt.Sprintf("%s-%s.tgz", name[:idx], name[idx+1:])
 	}
@@ -208,8 +208,8 @@ func (c *ChartDownloader) getOciURI(ref, version string, u *url.URL) (*url.URL,
 //   - If version is empty, this will return the URL for the latest version
 //   - If no version can be found, an error is returned
 func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, error) {
-	if gitutil.IsGitURL(ref) {
-		gitURL := strings.TrimPrefix(ref, "git://")
+	if gitutil.IsGitRepository(ref) {
+		gitURL := gitutil.RepositoryURLToGitURL(ref)
 		u, err := giturls.Parse(gitURL)
 		if err != nil {
 			return nil, errors.Errorf("invalid git URL format: %s", gitURL)

diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go
@@ -354,7 +354,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error {
 				getter.WithTagName(version))
 		}
 
-		if gitutil.IsGitURL(churl) {
+		if gitutil.IsGitRepository(churl) {
 			version = dep.Version
 
 			dl.Options = append(dl.Options, getter.WithTagName(version))
@@ -490,7 +490,7 @@ Loop:
 		}
 
 		// If repo is from git url, continue
-		if gitutil.IsGitURL(dd.Repository) {
+		if gitutil.IsGitRepository(dd.Repository) {
 			continue
 		}
 
@@ -614,7 +614,7 @@ func (m *Manager) resolveRepoNames(deps []*chart.Dependency) (map[string]string,
 		// if dep chart is from a git url, assume it is valid for now.
 		// 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.IsGitURL(dd.Repository) {
+		if gitutil.IsGitRepository(dd.Repository) {
 			if m.Debug {
 				fmt.Fprintf(m.Out, "Repository from git url: %s\n", strings.TrimPrefix(dd.Repository, "git:"))
 			}
@@ -732,7 +732,7 @@ func (m *Manager) parallelRepoUpdate(repos []*repo.Entry) error {
 //
 // If it finds a URL that is "relative", it will prepend the repoURL.
 func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]*repo.ChartRepository) (url, username, password string, insecureskiptlsverify, passcredentialsall bool, caFile, certFile, keyFile string, err error) {
-	if gitutil.IsGitURL(repoURL) {
+	if gitutil.IsGitRepository(repoURL) {
 		return repoURL, "", "", false, false, "", "", "", nil
 	}
 

diff --git a/pkg/getter/gitgetter.go b/pkg/getter/gitgetter.go
@@ -17,8 +17,6 @@ package getter
 
 import (
 	"bytes"
-	"strings"
-
 	"fmt"
 	"os"
 	"path/filepath"
@@ -62,7 +60,7 @@ func (g *GitGetter) Get(href string, options ...Option) (*bytes.Buffer, error) {
 }
 
 func (g *GitGetter) get(href string) (*bytes.Buffer, error) {
-	gitURL := strings.TrimPrefix(href, "git://")
+	gitURL := gitutil.RepositoryURLToGitURL(href)
 	version := g.opts.version
 	chartName := g.opts.chartName
 	if version == "" {