fix/gitindex: port selective config syncing onto current clone updates

Port the old idea from #635 onto current main so mirror updates stop rewriting every zoekt config value on every pass. The original patch shelled out to git config and never landed; moving the update path to go-git keeps it aligned with the rest of clone.go, makes it straightforward to detect real config changes, and lets CloneRepo return the repo path only when an existing clone actually needs reindexing.

While touching the port, also cover the stale-metadata case the old PR still missed. Empty settings now remove old zoekt.* keys instead of leaving outdated values behind, and remote.origin.url still stays in sync for existing clones.

Test Plan: go test ./gitindex

Author: keegancsmith

gitindex/clone.go

@@ -23,30 +23,167 @@ import (
 	"os/exec"
 	"path/filepath"
 	"sort"
+	"strings"
 
 	git "github.com/go-git/go-git/v5"
 	"github.com/go-git/go-git/v5/config"
+	formatconfig "github.com/go-git/go-git/v5/plumbing/format/config"
 )
 
+type gitConfigPath struct {
+	section    string
+	subsection string
+	key        string
+}
+
+func parseGitConfigPath(key string) (gitConfigPath, error) {
+	parts := strings.SplitN(key, ".", 3)
+	switch len(parts) {
+	case 2:
+		return gitConfigPath{section: parts[0], key: parts[1]}, nil
+	case 3:
+		return gitConfigPath{section: parts[0], subsection: parts[1], key: parts[2]}, nil
+	default:
+		return gitConfigPath{}, fmt.Errorf("invalid git config key %q", key)
+	}
+}
+
+func lookupRawConfigOption(cfg *formatconfig.Config, path gitConfigPath) (string, bool) {
+	if cfg == nil {
+		return "", false
+	}
+
+	for _, section := range cfg.Sections {
+		if !section.IsName(path.section) {
+			continue
+		}
+
+		if path.subsection == "" {
+			if !section.HasOption(path.key) {
+				return "", false
+			}
+			return section.Option(path.key), true
+		}
+
+		for _, subsection := range section.Subsections {
+			if !subsection.IsName(path.subsection) {
+				continue
+			}
+			if !subsection.HasOption(path.key) {
+				return "", false
+			}
+			return subsection.Option(path.key), true
+		}
+
+		return "", false
+	}
+
+	return "", false
+}
+
+func updateRawConfigOption(cfg *formatconfig.Config, path gitConfigPath, value string) bool {
+	current, ok := lookupRawConfigOption(cfg, path)
+	if value == "" {
+		if !ok {
+			return false
+		}
+
+		if path.subsection == "" {
+			cfg.Section(path.section).RemoveOption(path.key)
+		} else {
+			cfg.Section(path.section).Subsection(path.subsection).RemoveOption(path.key)
+		}
+
+		return true
+	}
+
+	if ok && current == value {
+		return false
+	}
+
+	if path.subsection == "" {
+		cfg.Section(path.section).SetOption(path.key, value)
+	} else {
+		cfg.Section(path.section).Subsection(path.subsection).SetOption(path.key, value)
+	}
+
+	return true
+}
+
+func updateRemoteURL(cfg *config.Config, path gitConfigPath, value string) (bool, error) {
+	if value == "" {
+		return false, fmt.Errorf("remote URL for %q cannot be empty", path.subsection)
+	}
+
+	remote := cfg.Remotes[path.subsection]
+	if remote == nil {
+		remote = &config.RemoteConfig{Name: path.subsection}
+		cfg.Remotes[path.subsection] = remote
+	}
+
+	current := ""
+	if len(remote.URLs) > 0 {
+		current = remote.URLs[0]
+	}
+	if current == value && len(remote.URLs) == 1 {
+		return false, nil
+	}
+
+	remote.URLs = []string{value}
+	return true, nil
+}
+
+func updateGitConfigOption(cfg *config.Config, key, value string) (bool, error) {
+	path, err := parseGitConfigPath(key)
+	if err != nil {
+		return false, err
+	}
+
+	if path.section == "remote" && path.subsection != "" && path.key == "url" {
+		return updateRemoteURL(cfg, path, value)
+	}
+
+	return updateRawConfigOption(cfg.Raw, path, value), nil
+}
+
 // Updates the zoekt.* git config options after a repo is cloned.
 // Once a repo is cloned, we can no longer use the --config flag to update all
 // of it's zoekt.* settings at once. `git config` is limited to one option at once.
-func updateZoektGitConfig(repoDest string, settings map[string]string) error {
+func updateZoektGitConfig(repoDest string, settings map[string]string) (bool, error) {
+	repo, err := git.PlainOpen(repoDest)
+	if err != nil {
+		return false, err
+	}
+
+	cfg, err := repo.Config()
+	if err != nil {
+		return false, err
+	}
+
 	var keys []string
 	for k := range settings {
 		keys = append(keys, k)
 	}
 	sort.Strings(keys)
 
+	var changed bool
 	for _, k := range keys {
-		if settings[k] != "" {
-			if err := exec.Command("git", "-C", repoDest, "config", k, settings[k]).Run(); err != nil {
-				return err
-			}
+		updated, err := updateGitConfigOption(cfg, k, settings[k])
+		if err != nil {
+			return false, err
 		}
+		changed = changed || updated
 	}
 
-	return nil
+	if !changed {
+		return false, nil
+	}
+
+	if err := repo.Storer.SetConfig(cfg); err != nil {
+		return false, err
+	}
+
+	return true, nil
 }
 
 // CloneRepo clones one repository, adding the given config
@@ -64,9 +201,13 @@ func CloneRepo(destDir, name, cloneURL string, settings map[string]string) (stri
 		// Repository exists, ensure settings are in sync including the clone URL
 		settings := maps.Clone(settings)
 		settings["remote.origin.url"] = cloneURL
-		if err := updateZoektGitConfig(repoDest, settings); err != nil {
+		hadUpdate, err := updateZoektGitConfig(repoDest, settings)
+		if err != nil {
 			return "", fmt.Errorf("failed to update repository settings: %w", err)
 		}
+		if hadUpdate {
+			return repoDest, nil
+		}
 		return "", nil
 	}
 

gitindex/clone_test.go

@@ -15,7 +15,11 @@
 package gitindex
 
 import (
+	"bytes"
+	"errors"
 	"os/exec"
+	"path/filepath"
+	"strings"
 	"testing"
 
 	git "github.com/go-git/go-git/v5"
@@ -56,3 +60,106 @@ git clone orig/.git clone.git
 		t.Fatalf("got %q want %q", got, want)
 	}
 }
+
+func gitConfigValue(t *testing.T, repoDir, key string) (string, bool) {
+	t.Helper()
+
+	cmd := exec.Command("git", "-C", repoDir, "config", "--get", key)
+	var stdout bytes.Buffer
+	cmd.Stdout = &stdout
+
+	err := cmd.Run()
+	if err == nil {
+		return strings.TrimSuffix(stdout.String(), "\n"), true
+	}
+
+	var exitErr *exec.ExitError
+	if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 {
+		return "", false
+	}
+
+	t.Fatalf("git config --get %s: %v", key, err)
+	return "", false
+}
+
+func TestCloneRepoReturnsDestinationWhenSettingsChange(t *testing.T) {
+	root := t.TempDir()
+	origin := filepath.Join(root, "origin.git")
+	runScript(t, root, "git init --bare "+origin)
+
+	destRoot := filepath.Join(root, "repos")
+	dest, err := CloneRepo(destRoot, "owner/repo", origin, map[string]string{
+		"zoekt.name":        "github.com/owner/repo",
+		"zoekt.description": "initial",
+	})
+	if err != nil {
+		t.Fatalf("CloneRepo initial clone: %v", err)
+	}
+
+	repoDest := filepath.Join(destRoot, "owner", "repo.git")
+	if dest != repoDest {
+		t.Fatalf("got %q want %q", dest, repoDest)
+	}
+
+	dest, err = CloneRepo(destRoot, "owner/repo", origin, map[string]string{
+		"zoekt.name":        "github.com/owner/repo",
+		"zoekt.description": "initial",
+	})
+	if err != nil {
+		t.Fatalf("CloneRepo no-op update: %v", err)
+	}
+	if dest != "" {
+		t.Fatalf("got %q want empty destination for unchanged settings", dest)
+	}
+
+	dest, err = CloneRepo(destRoot, "owner/repo", origin, map[string]string{
+		"zoekt.name":        "github.com/owner/repo",
+		"zoekt.description": "updated",
+	})
+	if err != nil {
+		t.Fatalf("CloneRepo changed settings: %v", err)
+	}
+	if dest != repoDest {
+		t.Fatalf("got %q want %q when settings changed", dest, repoDest)
+	}
+
+	if got, ok := gitConfigValue(t, repoDest, "zoekt.description"); !ok || got != "updated" {
+		t.Fatalf("got zoekt.description=%q exists=%t want updated/true", got, ok)
+	}
+}
+
+func TestCloneRepoRemovesEmptyZoektSettingsAndUpdatesOriginURL(t *testing.T) {
+	root := t.TempDir()
+	originA := filepath.Join(root, "origin-a.git")
+	originB := filepath.Join(root, "origin-b.git")
+	runScript(t, root, "git init --bare "+originA)
+	runScript(t, root, "git init --bare "+originB)
+
+	destRoot := filepath.Join(root, "repos")
+	if _, err := CloneRepo(destRoot, "owner/repo", originA, map[string]string{
+		"zoekt.name":        "github.com/owner/repo",
+		"zoekt.description": "present",
+	}); err != nil {
+		t.Fatalf("CloneRepo initial clone: %v", err)
+	}
+
+	dest, err := CloneRepo(destRoot, "owner/repo", originB, map[string]string{
+		"zoekt.name":        "github.com/owner/repo",
+		"zoekt.description": "",
+	})
+	if err != nil {
+		t.Fatalf("CloneRepo update: %v", err)
+	}
+
+	repoDest := filepath.Join(destRoot, "owner", "repo.git")
+	if dest != repoDest {
+		t.Fatalf("got %q want %q", dest, repoDest)
+	}
+
+	if got, ok := gitConfigValue(t, repoDest, "zoekt.description"); ok {
+		t.Fatalf("got stale zoekt.description=%q, want it removed", got)
+	}
+	if got, ok := gitConfigValue(t, repoDest, "remote.origin.url"); !ok || got != originB {
+		t.Fatalf("got remote.origin.url=%q exists=%t want %q/true", got, ok, originB)
+	}
+}