refactor: extract RepoPathFromURL to deduplicate Git URL normalization

internal/gitclone/manager.go

@@ -160,7 +160,10 @@ func (m *Manager) GetOrCreate(_ context.Context, upstreamURL string) (*Repositor
 		return repo, nil
 	}
 
-	clonePath := m.clonePathForURL(upstreamURL)
+	clonePath, err := m.clonePathForURL(upstreamURL)
+	if err != nil {
+		return nil, err
+	}
 
 	repo = &Repository{
 		state:              StateEmpty,
@@ -259,14 +262,22 @@ func (m *Manager) DiscoverExisting(ctx context.Context) ([]*Repository, error) {
 	return discovered, nil
 }
 
-func (m *Manager) clonePathForURL(upstreamURL string) string {
+// RepoPathFromURL extracts a normalised "host/path" from an upstream Git URL,
+// stripping any ".git" suffix.
+func RepoPathFromURL(upstreamURL string) (string, error) {
 	parsed, err := url.Parse(upstreamURL)
 	if err != nil {
-		return filepath.Join(m.config.MirrorRoot, "unknown")
+		return "", errors.Wrap(err, "parse upstream URL")
 	}
+	return filepath.Join(parsed.Host, strings.TrimSuffix(parsed.Path, ".git")), nil
+}
 
-	repoPath := strings.TrimSuffix(parsed.Path, ".git")
-	return filepath.Join(m.config.MirrorRoot, parsed.Host, repoPath)
+func (m *Manager) clonePathForURL(upstreamURL string) (string, error) {
+	repoPath, err := RepoPathFromURL(upstreamURL)
+	if err != nil {
+		return "", err
+	}
+	return filepath.Join(m.config.MirrorRoot, repoPath), nil
 }
 
 func (r *Repository) State() State {

internal/strategy/git/git.go

@@ -10,7 +10,6 @@ import (
 	"log/slog"
 	"net/http"
 	"net/http/httputil"
-	"net/url"
 	"os"
 	"path/filepath"
 	"strings"
@@ -241,25 +240,28 @@ func SpoolKeyForRequest(pathValue string, r *http.Request) (string, error) {
 	return "upload-pack-" + hex.EncodeToString(h[:8]), nil
 }
 
-func spoolDirForURL(mirrorRoot, upstreamURL string) string {
-	parsed, err := url.Parse(upstreamURL)
+func spoolDirForURL(mirrorRoot, upstreamURL string) (string, error) {
+	repoPath, err := gitclone.RepoPathFromURL(upstreamURL)
 	if err != nil {
-		return filepath.Join(mirrorRoot, ".spools", "unknown")
+		return "", errors.Wrap(err, "resolve spool directory")
 	}
-	repoPath := strings.TrimSuffix(parsed.Path, ".git")
-	return filepath.Join(mirrorRoot, ".spools", parsed.Host, repoPath)
+	return filepath.Join(mirrorRoot, ".spools", repoPath), nil
 }
 
-func (s *Strategy) getOrCreateRepoSpools(upstreamURL string) *RepoSpools {
+func (s *Strategy) getOrCreateRepoSpools(upstreamURL string) (*RepoSpools, error) {
 	s.spoolsMu.Lock()
 	defer s.spoolsMu.Unlock()
 	rp, exists := s.spools[upstreamURL]
-	if !exists {
-		dir := spoolDirForURL(s.cloneManager.Config().MirrorRoot, upstreamURL)
-		rp = NewRepoSpools(dir)
-		s.spools[upstreamURL] = rp
+	if exists {
+		return rp, nil
+	}
+	dir, err := spoolDirForURL(s.cloneManager.Config().MirrorRoot, upstreamURL)
+	if err != nil {
+		return nil, err
 	}
-	return rp
+	rp = NewRepoSpools(dir)
+	s.spools[upstreamURL] = rp
+	return rp, nil
 }
 
 func (s *Strategy) cleanupSpools(upstreamURL string) {
@@ -294,7 +296,13 @@ func (s *Strategy) serveWithSpool(w http.ResponseWriter, r *http.Request, host,
 		return
 	}
 
-	rp := s.getOrCreateRepoSpools(upstreamURL)
+	rp, err := s.getOrCreateRepoSpools(upstreamURL)
+	if err != nil {
+		logger.WarnContext(ctx, "Failed to resolve spool directory, forwarding to upstream",
+			slog.String("error", err.Error()))
+		s.forwardToUpstream(w, r, host, pathValue)
+		return
+	}
 	spool, isWriter, err := rp.GetOrCreate(key)
 	if err != nil {
 		logger.WarnContext(ctx, "Failed to create spool, forwarding to upstream",

internal/strategy/git/snapshot.go

@@ -4,11 +4,9 @@ import (
 	"context"
 	"log/slog"
 	"net/http"
-	"net/url"
 	"os"
 	"os/exec"
 	"path/filepath"
-	"strings"
 	"time"
 
 	"github.com/alecthomas/errors"
@@ -19,13 +17,12 @@ import (
 	"github.com/block/cachew/internal/snapshot"
 )
 
-func snapshotDirForURL(mirrorRoot, upstreamURL string) string {
-	parsed, err := url.Parse(upstreamURL)
+func snapshotDirForURL(mirrorRoot, upstreamURL string) (string, error) {
+	repoPath, err := gitclone.RepoPathFromURL(upstreamURL)
 	if err != nil {
-		return filepath.Join(mirrorRoot, ".snapshots", "unknown")
+		return "", errors.Wrap(err, "resolve snapshot directory")
 	}
-	repoPath := strings.TrimSuffix(parsed.Path, ".git")
-	return filepath.Join(mirrorRoot, ".snapshots", parsed.Host, repoPath)
+	return filepath.Join(mirrorRoot, ".snapshots", repoPath), nil
 }
 
 func (s *Strategy) generateAndUploadSnapshot(ctx context.Context, repo *gitclone.Repository) error {
@@ -35,7 +32,10 @@ func (s *Strategy) generateAndUploadSnapshot(ctx context.Context, repo *gitclone
 	logger.InfoContext(ctx, "Snapshot generation started", slog.String("upstream", upstream))
 
 	mirrorRoot := s.cloneManager.Config().MirrorRoot
-	snapshotDir := snapshotDirForURL(mirrorRoot, upstream)
+	snapshotDir, err := snapshotDirForURL(mirrorRoot, upstream)
+	if err != nil {
+		return err
+	}
 
 	// Clean any previous snapshot working directory.
 	if err := os.RemoveAll(snapshotDir); err != nil {
@@ -57,7 +57,7 @@ func (s *Strategy) generateAndUploadSnapshot(ctx context.Context, repo *gitclone
 	ttl := 7 * 24 * time.Hour
 	excludePatterns := []string{"*.lock"}
 
-	err := snapshot.Create(ctx, s.cache, cacheKey, snapshotDir, ttl, excludePatterns)
+	err = snapshot.Create(ctx, s.cache, cacheKey, snapshotDir, ttl, excludePatterns)
 
 	// Always clean up the snapshot working directory.
 	if rmErr := os.RemoveAll(snapshotDir); rmErr != nil {