Skip to content

Commit 47e0ba3

Browse files
keegancsmithampagent
keegancsmith
and
ampagent
committed
refactor/gitindex: simplify clone config syncing for readability
The config sync path in CloneRepo had grown dense, with low-level go-git raw config parsing mixed into clone orchestration. That made the intent harder to follow and increased the surface area of tests that were mostly implementation-detail checks. This refactor moves config synchronization helpers into clone_config.go, keeps clone.go focused on clone/update flow, and uses a small git-config command wrapper to apply only real changes while still supporting removals and remote URL updates. The test suite is also trimmed to a single integration-style update test that covers the behavior we care about (no-op updates, removal of empty settings, and remote.origin.url changes) without overfitting to helper internals. Test Plan: go test ./... Amp-Thread-ID: https://ampcode.com/threads/T-019d0be8-9ecb-7004-acaa-a53513692639 Co-authored-by: Amp <amp@ampcode.com>
1 parent b360720 commit 47e0ba3

3 files changed

Lines changed: 123 additions & 269 deletions

File tree

‎gitindex/clone.go‎

Lines changed: 1 addition & 190 deletions
Original file line numberDiff line numberDiff line change
@@ -22,187 +22,11 @@ import (
2222
"os"
2323
"os/exec"
2424
"path/filepath"
25-
"sort"
26-
"strings"
2725

2826
git "github.com/go-git/go-git/v5"
2927
"github.com/go-git/go-git/v5/config"
30-
formatconfig "github.com/go-git/go-git/v5/plumbing/format/config"
3128
)
3229

33-
type gitConfigPath struct {
34-
section string
35-
subsection string
36-
key string
37-
}
38-
39-
func parseGitConfigPath(key string) (gitConfigPath, error) {
40-
firstDot := strings.IndexByte(key, '.')
41-
lastDot := strings.LastIndexByte(key, '.')
42-
if firstDot <= 0 || lastDot == len(key)-1 {
43-
return gitConfigPath{}, fmt.Errorf("invalid git config key %q", key)
44-
}
45-
46-
if firstDot == lastDot {
47-
return gitConfigPath{section: key[:firstDot], key: key[firstDot+1:]}, nil
48-
}
49-
50-
path := gitConfigPath{
51-
section: key[:firstDot],
52-
subsection: key[firstDot+1 : lastDot],
53-
key: key[lastDot+1:],
54-
}
55-
if path.subsection == "" {
56-
return gitConfigPath{}, fmt.Errorf("invalid git config key %q", key)
57-
}
58-
59-
return path, nil
60-
}
61-
62-
func lookupConfigOption(cfg *formatconfig.Config, path gitConfigPath) (string, bool) {
63-
if cfg == nil {
64-
return "", false
65-
}
66-
67-
section := findConfigSection(cfg, path.section)
68-
if section == nil {
69-
return "", false
70-
}
71-
72-
if path.subsection == "" {
73-
if !section.HasOption(path.key) {
74-
return "", false
75-
}
76-
return section.Option(path.key), true
77-
}
78-
79-
subsection := findConfigSubsection(section, path.subsection)
80-
if subsection == nil {
81-
return "", false
82-
}
83-
if !subsection.HasOption(path.key) {
84-
return "", false
85-
}
86-
87-
return subsection.Option(path.key), true
88-
}
89-
90-
func findConfigSection(cfg *formatconfig.Config, name string) *formatconfig.Section {
91-
for _, section := range cfg.Sections {
92-
if section.IsName(name) {
93-
return section
94-
}
95-
}
96-
97-
return nil
98-
}
99-
100-
func findConfigSubsection(section *formatconfig.Section, name string) *formatconfig.Subsection {
101-
for _, subsection := range section.Subsections {
102-
if subsection.IsName(name) {
103-
return subsection
104-
}
105-
}
106-
107-
return nil
108-
}
109-
110-
func updateGitConfigOption(cfg *config.Config, key, value string) (bool, error) {
111-
path, err := parseGitConfigPath(key)
112-
if err != nil {
113-
return false, err
114-
}
115-
116-
// remote.<name>.url must go through typed remotes; SetConfig marshals
117-
// cfg.Remotes into Raw and would otherwise overwrite a raw-only update.
118-
if path.section == "remote" && path.subsection != "" && path.key == "url" {
119-
return updateRemoteURL(cfg, path.subsection, value)
120-
}
121-
122-
current, ok := lookupConfigOption(cfg.Raw, path)
123-
if value == "" {
124-
if !ok {
125-
return false, nil
126-
}
127-
if path.subsection == "" {
128-
cfg.Raw.Section(path.section).RemoveOption(path.key)
129-
} else {
130-
cfg.Raw.Section(path.section).Subsection(path.subsection).RemoveOption(path.key)
131-
}
132-
133-
return true, nil
134-
}
135-
136-
if ok && current == value {
137-
return false, nil
138-
}
139-
140-
subsection := formatconfig.NoSubsection
141-
if path.subsection != "" {
142-
subsection = path.subsection
143-
}
144-
cfg.Raw.SetOption(path.section, subsection, path.key, value)
145-
return true, nil
146-
}
147-
148-
func updateRemoteURL(cfg *config.Config, remoteName, value string) (bool, error) {
149-
if value == "" {
150-
return false, fmt.Errorf("remote URL for %q cannot be empty", remoteName)
151-
}
152-
153-
remote := cfg.Remotes[remoteName]
154-
if remote == nil {
155-
remote = &config.RemoteConfig{Name: remoteName}
156-
cfg.Remotes[remoteName] = remote
157-
}
158-
159-
if len(remote.URLs) == 1 && remote.URLs[0] == value {
160-
return false, nil
161-
}
162-
163-
remote.URLs = []string{value}
164-
return true, nil
165-
}
166-
167-
// updateZoektGitConfig applies zoekt.* settings and remote.origin.url updates
168-
// to an existing clone. It returns whether the repository config changed.
169-
func updateZoektGitConfig(repoDest string, settings map[string]string) (bool, error) {
170-
repo, err := git.PlainOpen(repoDest)
171-
if err != nil {
172-
return false, err
173-
}
174-
175-
cfg, err := repo.Config()
176-
if err != nil {
177-
return false, err
178-
}
179-
180-
var keys []string
181-
for k := range settings {
182-
keys = append(keys, k)
183-
}
184-
sort.Strings(keys)
185-
186-
var changed bool
187-
for _, k := range keys {
188-
updated, err := updateGitConfigOption(cfg, k, settings[k])
189-
if err != nil {
190-
return false, err
191-
}
192-
changed = changed || updated
193-
}
194-
195-
if !changed {
196-
return false, nil
197-
}
198-
199-
if err := repo.Storer.SetConfig(cfg); err != nil {
200-
return false, err
201-
}
202-
203-
return true, nil
204-
}
205-
20630
// CloneRepo clones one repository, adding the given config
20731
// settings. It returns the bare repo directory. The `name` argument
20832
// determines where the repo is stored relative to `destDir`. Returns
@@ -228,23 +52,10 @@ func CloneRepo(destDir, name, cloneURL string, settings map[string]string) (stri
22852
return "", nil
22953
}
23054

231-
var keys []string
232-
for k := range settings {
233-
keys = append(keys, k)
234-
}
235-
sort.Strings(keys)
236-
237-
var config []string
238-
for _, k := range keys {
239-
if settings[k] != "" {
240-
config = append(config, "--config", k+"="+settings[k])
241-
}
242-
}
243-
24455
cmd := exec.Command(
24556
"git", "clone", "--bare", "--verbose", "--progress",
24657
)
247-
cmd.Args = append(cmd.Args, config...)
58+
cmd.Args = append(cmd.Args, cloneConfigArgs(settings)...)
24859
cmd.Args = append(cmd.Args, cloneURL, repoDest)
24960

25061
// Prevent prompting

‎gitindex/clone_config.go‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
// Copyright 2026 Google Inc. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package gitindex
16+
17+
import (
18+
"bytes"
19+
"errors"
20+
"fmt"
21+
"os/exec"
22+
"sort"
23+
"strings"
24+
)
25+
26+
func sortedKeys(settings map[string]string) []string {
27+
keys := make([]string, 0, len(settings))
28+
for k := range settings {
29+
keys = append(keys, k)
30+
}
31+
sort.Strings(keys)
32+
return keys
33+
}
34+
35+
func cloneConfigArgs(settings map[string]string) []string {
36+
args := make([]string, 0, len(settings)*2)
37+
for _, key := range sortedKeys(settings) {
38+
if value := settings[key]; value != "" {
39+
args = append(args, "--config", key+"="+value)
40+
}
41+
}
42+
return args
43+
}
44+
45+
// updateZoektGitConfig applies zoekt.* settings and remote.origin.url updates
46+
// to an existing clone. It returns whether the repository config changed.
47+
func updateZoektGitConfig(repoDest string, settings map[string]string) (bool, error) {
48+
changed := false
49+
for _, key := range sortedKeys(settings) {
50+
updated, err := syncGitConfigOption(repoDest, key, settings[key])
51+
if err != nil {
52+
return false, err
53+
}
54+
changed = changed || updated
55+
}
56+
return changed, nil
57+
}
58+
59+
func syncGitConfigOption(repoDest, key, value string) (bool, error) {
60+
current, ok, err := repoConfigValue(repoDest, key)
61+
if err != nil {
62+
return false, err
63+
}
64+
65+
if value == "" {
66+
if !ok {
67+
return false, nil
68+
}
69+
if err := unsetRepoConfigValue(repoDest, key); err != nil {
70+
return false, err
71+
}
72+
return true, nil
73+
}
74+
75+
if ok && current == value {
76+
return false, nil
77+
}
78+
if err := setRepoConfigValue(repoDest, key, value); err != nil {
79+
return false, err
80+
}
81+
return true, nil
82+
}
83+
84+
func repoConfigValue(repoDest, key string) (string, bool, error) {
85+
cmd := exec.Command("git", "-C", repoDest, "config", "--get", key)
86+
var out bytes.Buffer
87+
cmd.Stdout = &out
88+
if err := cmd.Run(); err == nil {
89+
return strings.TrimSuffix(out.String(), "\n"), true, nil
90+
} else {
91+
var exitErr *exec.ExitError
92+
if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 {
93+
return "", false, nil
94+
}
95+
return "", false, fmt.Errorf("git config --get %q: %w", key, err)
96+
}
97+
}
98+
99+
func setRepoConfigValue(repoDest, key, value string) error {
100+
if err := exec.Command("git", "-C", repoDest, "config", "--replace-all", key, value).Run(); err != nil {
101+
return fmt.Errorf("git config --replace-all %q: %w", key, err)
102+
}
103+
return nil
104+
}
105+
106+
func unsetRepoConfigValue(repoDest, key string) error {
107+
if err := exec.Command("git", "-C", repoDest, "config", "--unset-all", key).Run(); err != nil {
108+
return fmt.Errorf("git config --unset-all %q: %w", key, err)
109+
}
110+
return nil
111+
}

0 commit comments

Comments
 (0)