gitindex: diff config updates for existing clones#1019
Draft
keegancsmith wants to merge 6 commits intomainfrom
Draft
gitindex: diff config updates for existing clones#1019keegancsmith wants to merge 6 commits intomainfrom
keegancsmith wants to merge 6 commits intomainfrom
Conversation
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
keegancsmith
requested review from
a team,
burmudar and
stefanhengl
and removed request for
a team
Member
Author
|
Sorry vibe coded this change and the agent decided to go ahead and open a PR before I had reviewed / improved anything |
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>
The helper that sorted config keys manually built an intermediate slice before sorting, even though Go now provides an iterator-first path for this pattern. Using maps.Keys with slices.Sorted keeps the helper shorter and makes the intent explicit while preserving deterministic ordering for clone and config-update command generation. Test Plan: go test ./... Amp-Thread-ID: https://ampcode.com/threads/T-019d0be8-9ecb-7004-acaa-a53513692639 Co-authored-by: Amp <amp@ampcode.com>
The #635 port accidentally introduced remote.origin.url rewrites for existing clones. That is a behavior change from the original flow and not required for the metadata-diff optimization. This keeps the update path focused on zoekt settings only, while preserving the return-value signal that triggers reindex when metadata actually changes. The integration test now validates metadata mutation and no-op behavior without expecting remote URL churn. Test Plan: go test ./... Amp-Thread-ID: https://ampcode.com/threads/T-019d0c05-51b0-77ee-a1dc-cb60ce14712d Co-authored-by: Amp <amp@ampcode.com>
After removing remote.origin.url sync from existing-clone updates, keep an explicit regression check that captures the behavior boundary. The integration test now changes the clone URL during a metadata update and verifies remote.origin.url remains at the originally cloned value while zoekt config still updates and triggers a destination return. Test Plan: go test ./... Amp-Thread-ID: https://ampcode.com/threads/T-019d0c05-51b0-77ee-a1dc-cb60ce14712d Co-authored-by: Amp <amp@ampcode.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This supersedes #635 by porting its selective config-sync idea onto current
mainwith a smaller, easier-to-read shape. Clone orchestration stays ingitindex/clone.go, while config argument generation and existing-clone sync logic now live ingitindex/clone_config.go.For existing clones, we now diff each
zoekt.*setting before writing. Unchanged values are skipped, changed values are updated, and settings that disappear are removed.CloneReporeturns the repo destination only when a setting change actually happened so the caller can trigger reindexing only when needed.This version intentionally does not rewrite
remote.origin.urlduring the existing-clone settings update path. The sync path is scoped to zoekt metadata, and the integration test covers that a clone URL change does not mutateremote.origin.urlwhile metadata changes still apply.cmd/zoekt-mirror-githubwas also cleaned up so optional integer metadata keys are only added to the config map when present, which avoids pushing empty config values downstream.Test Plan
go test ./...Changelog