perf(git): optimize fetch for repos with many tags and branches#453
Open
zendesk-slowery23 wants to merge 2 commits into
Open
perf(git): optimize fetch for repos with many tags and branches#453zendesk-slowery23 wants to merge 2 commits into
zendesk-slowery23 wants to merge 2 commits into
Conversation
When syncing a git source with a fixed ref (tag or branch name), vendir previously fetched all remote refs and all tags, which is very slow for repos with thousands of branches or hundreds of tags. Changes: - Named refs (tags, branches) now use a targeted refspec fetch (`git fetch origin <ref> --no-tags`) and check out FETCH_HEAD, avoiding negotiation of every remote ref. - `--no-tags` tagOpt is set for named refs; `--tags` is kept only for RefSelection (semver matching) and bare SHAs. - In `--locked` mode, `DirectoryContentsGit.Lock()` now preserves the original ref in a new `OriginalRef` field (not serialized) before overwriting `Ref` with the locked SHA. The fetcher uses `OriginalRef` for the targeted fetch, then verifies the resulting commit matches the locked SHA — catching force-pushed tags. - Plain SHA refs without an OriginalRef (edge case) fall back to the previous full fetch with `--tags`. Observed improvement on a repo with thousands of branches: vendir sync: 43s → 11s (74% faster) vendir sync --locked: 86s → 9s (89% faster) Adds unit tests covering each fetch path and e2e tests verifying the targeted fetch behaviour and tampered-lock detection. Signed-off-by: Steve Lowery <steve.lowery@zendesk.com>
| if hasUser { | ||
| password := string(secret.Data[ | ||
| ctlconf.SecretK8sCorev1BasicAuthPasswordKey]) | ||
| password := string(secret.Data[ctlconf.SecretK8sCorev1BasicAuthPasswordKey]) |
Author
There was a problem hiding this comment.
result of go fmt from hack/build.sh
There was a problem hiding this comment.
Pull request overview
This PR speeds up vendir sync for git sources by avoiding full ref negotiation and unconditional tag downloads when only a single named ref is needed, and adds test coverage around the new fetch behaviors (including --locked).
Changes:
- Add an internal
OriginalReffield to preserve the pre-lock ref name for optimized fetching in--lockedmode. - Update the git fetcher to use targeted single-refspec fetches and conditionally disable tag fetching.
- Add unit + e2e tests validating fetch command shapes and locked-SHA mismatch detection.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/vendir/fetch/git/git.go |
Implements targeted fetch logic, conditional --tags/--no-tags, FETCH_HEAD checkout handling, and locked SHA verification. |
pkg/vendir/config/directory.go |
Adds OriginalRef and sets it during Lock() before overwriting Ref with the locked SHA. |
pkg/vendir/fetch/git/git_test.go |
Adds unit tests covering the new fetch paths and helper utilities for command assertions. |
test/e2e/git_test.go |
Adds an end-to-end test asserting optimized fetch behavior and tampered lock detection. |
pkg/vendir/fetch/http/sync.go |
Minor formatting change in basic auth password extraction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Strip origin/ prefix from OriginalRef before using it as a fetch
refspec in locked mode (origin/main is not a valid remote refspec)
- In locked mode, verify FETCH_HEAD^{commit} matches the locked SHA
before checkout, then check out the SHA directly rather than
FETCH_HEAD; this ensures mutable refs like origin/main stay pinned
to the locked commit rather than tracking the branch tip
- Accept uppercase hex in isHexSHA (git SHAs are case-insensitive)
- Fix e2e test bare-fetch assertion (needle had a spurious \n that
prevented it from ever triggering)
- Add unit test covering origin/ prefix stripping in locked mode
- Update locked-mode unit test to match revised checkout behavior
Signed-off-by: Steve Lowery <steve.lowery@zendesk.com>
Author
|
Are any of the maintainers available to approve the workflows for my PR and/or to discuss it? We use vendir pretty heavily and this performance improvement will have a big boost in our engineer's productivity. |
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
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.
Problem
vendir syncis very slow for git repositories with large numbers of branches and tags. The root cause is that the git fetcher unconditionally:remote.origin.tagOpt = --tags, causing all tag objects to be downloaded on every fetchgit fetch, causing all remote refs to be negotiatedOn a repo with thousands of branches and tags, this turns every sync into a multi-megabyte download regardless of what ref you actually need.
Solution
When a fixed named ref is specified (tag or branch, not a SHA), use a targeted single-refspec fetch:
Then check out
FETCH_HEAD(which git populates from a single-refspec fetch) instead of the ref name.--tagsis kept only where it's actually needed:refSelection(semver matching must enumerate all tags)OriginalRef(SHAs can't be fetched by name)--lockedpath--lockedsubstitutes the locked SHA for the original ref before reaching the fetcher, which previously forced a full--tagsfetch every time. This PR preserves the original ref in a new unexportedOriginalReffield onDirectoryContentsGit(set byLock(), not serialized to YAML). The fetcher uses it for the same targeted fetch path, then verifies the resulting commit SHA matches the lock — catching force-pushed tags.Performance
Measured against an internal repo with thousands of branches and hundreds of tags:
vendir syncvendir sync --lockedChanges
pkg/vendir/config/directory.go— addOriginalRef string(unexported from YAML) toDirectoryContentsGit; set it inLock()before overwritingRefwith the SHApkg/vendir/fetch/git/git.go— targeted refspec fetch for named refs and locked-SHA-with-OriginalRef; conditional--tags/--no-tags; post-checkout SHA verification in locked mode;isHexSHA()helperpkg/vendir/fetch/git/git_test.go— unit tests for each fetch path (named tag,origin/branch, locked SHA+OriginalRef, plain SHA, depth ordering)test/e2e/git_test.go—TestGitFetchOptimizations: verifies targeted fetch line in--jsonoutput, locked sync uses OriginalRef, tampered lock SHA is detectedTesting
🤖 Generated with Claude Code