fix(git): retry the other URL form when a host answers with protocol v2

internal/git/git.go

@@ -11,8 +11,10 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
+	"net/url"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"github.com/go-git/go-git/v5"
 	gitconfig "github.com/go-git/go-git/v5/config"
@@ -61,31 +63,126 @@ func (s *RepositoryService) SyncBareRepository(ctx context.Context, remoteURL, l
 		return fmt.Errorf("unsupported repository URL '%s'. Only http and https clone URLs are allowed.", remoteURL)
 	}
 
+	syncedURL := remoteURL
+	err := mirrorRepository(ctx, remoteURL, localPath, credential, cache)
+	// A host may serve the modern protocol on one form of a repository URL and
+	// the older one on the other, and this client cannot read the modern one.
+	// The same repository is then reachable at the other form, so a failure
+	// that is only about the protocol is retried there rather than reported.
+	// The retry becomes the URL the rest of the sync uses, so a host that
+	// serves LFS on the answered form only is not failed after the mirror
+	// succeeded.
+	if alternate := versionTwoAlternate(remoteURL, err); alternate != "" {
+		slog.Info("Remote answered with Git protocol v2, which this client cannot read; retrying the other URL form.",
+			"repository", paths.RedactURL(remoteURL), "retrying", paths.RedactURL(alternate))
+		if err = mirrorRepository(ctx, alternate, localPath, credential, cache); err == nil {
+			syncedURL = alternate
+		}
+	}
+	if err != nil {
+		return err
+	}
+
+	if includeLFS {
+		return s.fetchLFS(ctx, syncedURL, localPath, credential)
+	}
+	return nil
+}
+
+// mirrorRepository clones or updates the mirror at localPath from remoteURL.
+func mirrorRepository(ctx context.Context, remoteURL, localPath string, credential *Credential, cache bool) error {
 	if cache && isBareRepository(localPath) {
 		// Update the existing mirror. The mirror refspec (+refs/*:refs/*)
 		// force-updates rewritten branches and prune drops refs deleted
 		// upstream, so the mirror tracks the remote exactly.
 		if err := fetchMirror(ctx, remoteURL, localPath, credential); err != nil {
-			if ctx.Err() != nil {
-				// A shutdown is not a corrupt mirror. Re-cloning here would
-				// delete the cached mirror and then fail on the same cancelled
-				// context, leaving nothing behind.
+			if ctx.Err() != nil || isProtocolVersionTwo(err) {
+				// A shutdown or a protocol v2 advertisement is not a corrupt
+				// mirror. Re-cloning would delete a mirror that is intact — and
+				// on the protocol path it would delete the mirror and then fail
+				// on the same form, turning a retry that could have fetched
+				// incrementally into a full clone.
 				return err
 			}
 			slog.Warn("Incremental mirror fetch failed; re-cloning from scratch.",
 				"localPath", localPath, "error", err.Error())
 			return freshClone(ctx, remoteURL, localPath, credential)
 		}
-	} else {
-		if err := freshClone(ctx, remoteURL, localPath, credential); err != nil {
-			return err
-		}
+		return nil
 	}
+	return freshClone(ctx, remoteURL, localPath, credential)
+}
 
-	if includeLFS {
-		return s.fetchLFS(ctx, remoteURL, localPath, credential)
+// pktLineTooShort and cannotReadHash are the two fragments go-git's v0/v1 ref
+// decoder produces when it is handed a protocol v2 advertisement: the version
+// announcement is read as a ref, and "version 2" is too short to be a hash.
+// Both are required so an unrelated decode failure cannot be mistaken for it.
+//
+// The failure is recognised by its text because go-git raises it from deep
+// inside the decoder and wraps it in no error this package can match on; there
+// is no sentinel to compare against.
+const (
+	pktLineTooShort = "pkt-line too short"
+	cannotReadHash  = "cannot read hash"
+)
+
+// versionTwoAlternate returns the other spelling of remoteURL when err says the
+// remote answered with a protocol v2 advertisement, and "" when there is
+// nothing to retry.
+//
+// A host decides whether a repository is served at its bare path or at the path
+// ending in ".git", and some hosts answer one of the two with the modern
+// protocol only. Since go-git v5 cannot read that protocol, the other form is
+// the same repository reached a way this client understands. A URL with no path
+// to alternate has nothing to offer, so it is reported rather than retried.
+func versionTwoAlternate(remoteURL string, err error) string {
+	if err == nil || !isProtocolVersionTwo(err) {
+		return ""
 	}
-	return nil
+	return alternateURLForm(remoteURL)
+}
+
+func isProtocolVersionTwo(err error) bool {
+	if err == nil {
+		return false
+	}
+	message := err.Error()
+	return strings.Contains(message, cannotReadHash) && strings.Contains(message, pktLineTooShort)
+}
+
+// alternateURLForm returns remoteURL with its ".git" suffix flipped, or "" when
+// there is no path to flip.
+//
+// The suffix is matched case-insensitively, as the rest of the codebase does, so
+// a URL that already ends in ".GIT" is recognised as suffixed rather than given
+// a second one. A trailing slash is normalised away first, because otherwise it
+// would end up inside the rewritten path. A fragment is dropped: it is a
+// client-side marker that never forms part of what is fetched, and carrying it
+// onto the other form would append it to a request the server never sent one
+// for. A query is kept, since it can be part of what the remote is asked for.
+//
+// The rewrite is driven by the decoded path, so an escaped separator is no
+// longer distinguished from a real one. A repository path containing a literal
+// slash cannot be spelled in a git URL anyway — the separator is structural —
+// so decoding is the more faithful reading of the URL, not a loss.
+func alternateURLForm(remoteURL string) string {
+	parsed, err := url.Parse(strings.TrimSpace(remoteURL))
+	if err != nil {
+		return ""
+	}
+
+	withoutSlash := strings.TrimSuffix(parsed.Path, "/")
+	if withoutSlash == "" {
+		return ""
+	}
+
+	parsed.Path = paths.TrimGitSuffix(withoutSlash)
+	if parsed.Path == withoutSlash {
+		parsed.Path += ".git"
+	}
+	parsed.Fragment = ""
+
+	return parsed.String()
 }
 
 // fetchLFS mirrors the remote's LFS objects. A remote can have Git LFS turned
@@ -100,7 +197,7 @@ func (s *RepositoryService) fetchLFS(ctx context.Context, remoteURL, localPath s
 
 	err := s.lfs.FetchAll(ctx, localPath, remoteURL, username, password)
 	if errors.Is(err, lfs.ErrDisabled) {
-		slog.Info("Skipped Git LFS fetch because it is disabled on the remote.", "repository", remoteURL)
+		slog.Info("Skipped Git LFS fetch because it is disabled on the remote.", "repository", paths.RedactURL(remoteURL))
 		return nil
 	}
 	if err != nil {

internal/git/git_test.go

@@ -28,19 +28,36 @@ import (
 // exercised without any external binary or network.
 const sourceURL = "http://gitbackup.test/source.git"
 
-var testLoader = &armedLoader{repositories: make(map[string]storer.Storer)}
+// protocolTwoMessage is the error go-git's v0/v1 decoder reports when a host
+// answers with a Git protocol v2 advertisement, built from the same fragments the
+// production check matches so the two cannot drift apart.
+var protocolTwoMessage = fmt.Sprintf("pkt-line 3: %s, %s (version 2)", cannotReadHash, pktLineTooShort)
+
+var testLoader = &armedLoader{
+	repositories: make(map[string]storer.Storer),
+	protocolTwo:  make(map[string]error),
+}
 
 // armedLoader serves the registered repositories but can be told to fail, so
-// tests can exercise the incremental-fetch failure and self-heal paths.
+// tests can exercise the incremental-fetch failure and self-heal paths. It can
+// also answer one endpoint the way a host does when it serves Git protocol v2
+// there: go-git's v0/v1 decoder reports the version announcement as a hash it
+// cannot read.
 type armedLoader struct {
 	repositories map[string]storer.Storer
 	fail         atomic.Bool
+	// protocolTwo maps an endpoint to the error its v2 answer produces, so a
+	// test can serve the modern protocol on one URL form only.
+	protocolTwo map[string]error
 }
 
 func (l *armedLoader) Load(ep *transport.Endpoint) (storer.Storer, error) {
 	if l.fail.Load() {
 		return nil, errors.New("server unavailable")
 	}
+	if err, ok := l.protocolTwo[ep.String()]; ok {
+		return nil, err
+	}
 	repository, ok := l.repositories[ep.String()]
 	if !ok {
 		return nil, transport.ErrRepositoryNotFound
@@ -140,6 +157,155 @@ func TestSyncBareRepositoryRejectsUnsupportedTransport(t *testing.T) {
 	}
 }
 
+// TestSyncBareRepositoryRetriesTheOtherURLForm covers a host that serves Git
+// protocol v2 at one form of a repository URL while the repository itself is
+// reachable at the other. This client speaks v0/v1 only, so the v2 answer is not
+// something it can read; the same repository at the other form is, so the sync
+// is retried there instead of failing the repository.
+func TestSyncBareRepositoryRetriesTheOtherURLForm(t *testing.T) {
+	const configuredURL = "http://gitbackup.test/protocol-two"
+	const servedURL = configuredURL + ".git"
+
+	source, _ := createSourceRepositoryAt(t, servedURL)
+	head, err := source.Head()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// The configured form answers with the modern protocol, which go-git
+	// reports the way v5's decoder does.
+	testLoader.protocolTwo[configuredURL] = errors.New(protocolTwoMessage)
+	t.Cleanup(func() { delete(testLoader.protocolTwo, configuredURL) })
+
+	mirrorPath := filepath.Join(t.TempDir(), "repositories", "mirror")
+	service := NewRepositoryService()
+	if err := service.SyncBareRepository(context.Background(), configuredURL, mirrorPath, nil, false, false); err != nil {
+		t.Fatalf("sync should have retried the other URL form: %v", err)
+	}
+	if !mirrorHasCommit(t, mirrorPath, head.Hash()) {
+		t.Error("mirror should contain the source commit from the form that answers")
+	}
+}
+
+// TestSyncBareRepositoryReportsProtocolTwoWithoutAnAlternate covers a URL with
+// no other form to try: the failure is reported rather than retried into the
+// same answer.
+func TestSyncBareRepositoryReportsProtocolTwoWithoutAnAlternate(t *testing.T) {
+	const configuredURL = "http://gitbackup.test/"
+
+	testLoader.protocolTwo[configuredURL] = errors.New(protocolTwoMessage)
+	t.Cleanup(func() { delete(testLoader.protocolTwo, configuredURL) })
+
+	mirrorPath := filepath.Join(t.TempDir(), "repositories", "mirror")
+	service := NewRepositoryService()
+	err := service.SyncBareRepository(context.Background(), configuredURL, mirrorPath, nil, false, false)
+	if err == nil {
+		t.Fatal("a host that only answers with protocol v2 should be reported")
+	}
+	if !strings.Contains(err.Error(), pktLineTooShort) {
+		t.Errorf("err = %v, want the protocol failure reported", err)
+	}
+}
+
+// TestSyncBareRepositoryRetriesWithoutDestroyingTheMirror covers a cached mirror
+// whose host starts answering with protocol v2. The mirror is intact and the
+// other URL form still serves the repository, so the retry must fetch into the
+// mirror rather than re-cloning: a destructive self-heal would delete a usable
+// mirror and then fail on the same form first, costing a full clone every run.
+func TestSyncBareRepositoryRetriesWithoutDestroyingTheMirror(t *testing.T) {
+	const configuredURL = "http://gitbackup.test/cached-two"
+	const servedURL = configuredURL + ".git"
+
+	createSourceRepositoryAt(t, servedURL)
+	mirrorPath := filepath.Join(t.TempDir(), "repositories", "mirror")
+	service := NewRepositoryService()
+
+	// Seed the cached mirror through the form that answers.
+	if err := service.SyncBareRepository(context.Background(), servedURL, mirrorPath, nil, true, false); err != nil {
+		t.Fatalf("seeding the mirror failed: %v", err)
+	}
+
+	// A file inside the mirror stands in for its contents: re-cloning removes
+	// the directory, so its survival is what separates a fetch from a clone.
+	marker := filepath.Join(mirrorPath, "seeded-marker")
+	if err := os.WriteFile(marker, []byte("seed"), 0o644); err != nil {
+		t.Fatalf("write marker: %v", err)
+	}
+
+	// The host now answers the configured form with the modern protocol while
+	// the other form still serves the repository.
+	testLoader.protocolTwo[configuredURL] = errors.New(protocolTwoMessage)
+	t.Cleanup(func() { delete(testLoader.protocolTwo, configuredURL) })
+
+	if err := service.SyncBareRepository(context.Background(), configuredURL, mirrorPath, nil, true, false); err != nil {
+		t.Fatalf("sync should have retried the other URL form: %v", err)
+	}
+	if _, err := os.Stat(marker); err != nil {
+		t.Errorf("the cached mirror was rebuilt instead of fetched into: %v", err)
+	}
+	if !isBareRepository(mirrorPath) {
+		t.Error("the cached mirror should still be a bare repository")
+	}
+}
+
+// TestAlternateURLForm covers how the other form of a repository URL is derived.
+func TestAlternateURLForm(t *testing.T) {
+	cases := []struct {
+		name     string
+		remote   string
+		expected string
+	}{
+		{"appends the suffix", "https://host/owner/repo", "https://host/owner/repo.git"},
+		{"removes the suffix", "https://host/owner/repo.git", "https://host/owner/repo"},
+		{"keeps the port", "https://host:8443/owner/repo", "https://host:8443/owner/repo.git"},
+		{"keeps userinfo", "https://user@host/owner/repo", "https://user@host/owner/repo.git"},
+		{"keeps a nested path", "https://host/a/b/c/repo", "https://host/a/b/c/repo.git"},
+		{"no path to alternate", "https://host", ""},
+		{"root path has nothing to alternate", "https://host/", ""},
+		{"only the last suffix is flipped", "https://host/owner/repo.git.git", "https://host/owner/repo.git"},
+		{"keeps a query", "https://host/owner/repo?foo=1", "https://host/owner/repo.git?foo=1"},
+		{"recognises an upper-case suffix", "https://host/owner/repo.GIT", "https://host/owner/repo"},
+		{"drops a trailing slash", "https://host/owner/repo/", "https://host/owner/repo.git"},
+		{"drops a trailing slash after the suffix", "https://host/owner/repo.git/", "https://host/owner/repo"},
+		{"drops a fragment", "https://host/owner/repo#main", "https://host/owner/repo.git"},
+		{"keeps an escaped path escaped", "https://host/owner/re%20po", "https://host/owner/re%20po.git"},
+		{"decodes an escaped separator", "https://host/owner/re%2Fpo", "https://host/owner/re/po.git"},
+	}
+	for _, testCase := range cases {
+		t.Run(testCase.name, func(t *testing.T) {
+			if got := alternateURLForm(testCase.remote); got != testCase.expected {
+				t.Errorf("alternateURLForm(%q) = %q, want %q", testCase.remote, got, testCase.expected)
+			}
+		})
+	}
+}
+
+// TestVersionTwoAlternate covers the decision to retry: only a protocol v2
+// advertisement leaves anything to try, and only when there is another form.
+func TestVersionTwoAlternate(t *testing.T) {
+	protocolTwo := errors.New(protocolTwoMessage)
+
+	cases := []struct {
+		name     string
+		remote   string
+		err      error
+		expected string
+	}{
+		{"retries the other form", "https://host/owner/repo", protocolTwo, "https://host/owner/repo.git"},
+		{"nothing to retry without a path", "https://host/", protocolTwo, ""},
+		{"a successful sync is not retried", "https://host/owner/repo", nil, ""},
+		{"an unrelated failure is not retried", "https://host/owner/repo", errors.New("connection refused"), ""},
+		{"an authentication failure is not retried", "https://host/owner/repo", transport.ErrAuthenticationRequired, ""},
+	}
+	for _, testCase := range cases {
+		t.Run(testCase.name, func(t *testing.T) {
+			if got := versionTwoAlternate(testCase.remote, testCase.err); got != testCase.expected {
+				t.Errorf("versionTwoAlternate(%q, %v) = %q, want %q", testCase.remote, testCase.err, got, testCase.expected)
+			}
+		})
+	}
+}
+
 func TestSyncBareRepositoryFreshClone(t *testing.T) {
 	source, _ := createSourceRepository(t)
 	head, err := source.Head()

internal/paths/paths_test.go

@@ -51,6 +51,35 @@ func TestTrimGitSuffix(t *testing.T) {
 	}
 }
 
+func TestRedactURL(t *testing.T) {
+	tests := []struct {
+		name string
+		raw  string
+		want string
+	}{
+		{name: "masks a password", raw: "https://user:token@host/owner/repo", want: "https://user:xxxxx@host/owner/repo"},
+		{name: "masks only the password", raw: "https://user:tok@host:8443/owner/repo.git", want: "https://user:xxxxx@host:8443/owner/repo.git"},
+		{name: "keeps a username", raw: "https://user@host/owner/repo", want: "https://user@host/owner/repo"},
+		{name: "leaves a url without userinfo alone", raw: "https://host/owner/repo", want: "https://host/owner/repo"},
+		{name: "masks a password in a value the parser rejects", raw: "https://user:token@exa mple.com/repo", want: "https://user:xxxxx@exa mple.com/repo"},
+		{name: "masks a password before a bad port", raw: "https://user:token@host:notaport/repo", want: "https://user:xxxxx@host:notaport/repo"},
+		{name: "masks a password with a space in it", raw: "https://user:p@ss word@host/repo", want: "https://user:xxxxx@host/repo"},
+		{name: "masks a password under another scheme", raw: "ssh://user:token@host:22/repo", want: "ssh://user:xxxxx@host:22/repo"},
+		{name: "masks a password containing a slash", raw: "https://user:tok/en@host/repo", want: "https://user:xxxxx@host/repo"},
+		{name: "masks a password containing a question mark", raw: "https://user:tok?en@host/repo", want: "https://user:xxxxx@host/repo"},
+		{name: "masks a password containing a hash", raw: "https://user:tok#en@host/repo", want: "https://user:xxxxx@host/repo"},
+		{name: "leaves a host and port without userinfo alone", raw: "https://host:8080/repo", want: "https://host:8080/repo"},
+		{name: "leaves an unparseable value without userinfo alone", raw: "http://[::1", want: "http://[::1"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := RedactURL(tt.raw); got != tt.want {
+				t.Errorf("RedactURL(%q) = %q, want %q", tt.raw, got, tt.want)
+			}
+		})
+	}
+}
+
 func TestParseHTTPURL(t *testing.T) {
 	ok := []string{"https://example.com/x", "http://10.0.0.1:9000", "HTTPS://EXAMPLE.COM", " https://padded.example.com "}
 	for _, value := range ok {