From 1292613165dd5770364e0476cd8262ef286946ab Mon Sep 17 00:00:00 2001 From: Steve Lowery Date: Wed, 24 Jun 2026 16:23:45 -0500 Subject: [PATCH 1/2] Optimize git fetch for repos with many tags and branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --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 --- pkg/vendir/config/directory.go | 4 + pkg/vendir/fetch/git/git.go | 59 +++++++++++- pkg/vendir/fetch/git/git_test.go | 155 +++++++++++++++++++++++++++++++ pkg/vendir/fetch/http/sync.go | 3 +- test/e2e/git_test.go | 125 +++++++++++++++++++++++++ 5 files changed, 341 insertions(+), 5 deletions(-) diff --git a/pkg/vendir/config/directory.go b/pkg/vendir/config/directory.go index 72c78696..2b707521 100644 --- a/pkg/vendir/config/directory.go +++ b/pkg/vendir/config/directory.go @@ -73,6 +73,9 @@ type DirectoryContentsGit struct { SkipInitSubmodules bool `json:"skipInitSubmodules,omitempty"` Depth int `json:"depth,omitempty"` ForceHTTPBasicAuth bool `json:"forceHTTPBasicAuth,omitempty"` + // OriginalRef holds the ref before it was replaced by a locked SHA. + // Not serialized; set by Lock() to enable targeted fetching in locked mode. + OriginalRef string `json:"-" yaml:"-"` } type DirectoryContentsGitVerification struct { @@ -348,6 +351,7 @@ func (c *DirectoryContentsGit) Lock(lockConfig *LockDirectoryContentsGit) error if len(lockConfig.SHA) == 0 { return fmt.Errorf("Expected git SHA to be non-empty") } + c.OriginalRef = c.Ref c.Ref = lockConfig.SHA return nil } diff --git a/pkg/vendir/fetch/git/git.go b/pkg/vendir/fetch/git/git.go index 07883a7f..e240e98e 100644 --- a/pkg/vendir/fetch/git/git.go +++ b/pkg/vendir/fetch/git/git.go @@ -166,17 +166,40 @@ func (t *Git) fetch(dstPath string, tempArea ctlfetch.TempArea, bundle string) e } } - argss = append(argss, []string{"config", "remote.origin.tagOpt", "--tags"}) + // Suppress tag fetching when a targeted refspec fetch is possible: + // - named ref (tag/branch): targeted fetch by name + // - locked SHA with a known named original ref: fetch by original ref name + // RefSelection needs all tags for semver matching. A bare SHA with no + // OriginalRef has no named ref to target and must fall back to a full fetch. + canTargetFetch := (len(t.opts.Ref) > 0 && !isHexSHA(t.opts.Ref) && t.opts.RefSelection == nil) || + (isHexSHA(t.opts.Ref) && len(t.opts.OriginalRef) > 0 && !isHexSHA(t.opts.OriginalRef)) + if canTargetFetch { + argss = append(argss, []string{"config", "remote.origin.tagOpt", "--no-tags"}) + } else { + argss = append(argss, []string{"config", "remote.origin.tagOpt", "--tags"}) + } if bundle != "" { argss = append(argss, []string{"bundle", "unbundle", bundle}) } + // When fetching a single named ref, git places it in FETCH_HEAD only — + // no local ref is created. Track this so we can use FETCH_HEAD for checkout. + var useFetchHead bool { fetchArgs := []string{"fetch", "origin"} - if strings.HasPrefix(t.opts.Ref, "origin/") { - // only fetch the exact ref we're seeking + switch { + case strings.HasPrefix(t.opts.Ref, "origin/"): + // only fetch the exact remote branch we're seeking fetchArgs = append(fetchArgs, t.opts.Ref[7:]) + case len(t.opts.Ref) > 0 && !isHexSHA(t.opts.Ref): + // fetch only the named tag or branch; SHAs must use a full fetch + fetchArgs = append(fetchArgs, t.opts.Ref, "--no-tags") + useFetchHead = true + case isHexSHA(t.opts.Ref) && len(t.opts.OriginalRef) > 0 && !isHexSHA(t.opts.OriginalRef): + // locked mode: fetch by the original named ref so we avoid a full fetch + fetchArgs = append(fetchArgs, t.opts.OriginalRef, "--no-tags") + useFetchHead = true } if t.opts.Depth > 0 { fetchArgs = append(fetchArgs, "--depth", strconv.Itoa(t.opts.Depth)) @@ -193,6 +216,9 @@ func (t *Git) fetch(dstPath string, tempArea ctlfetch.TempArea, bundle string) e if err != nil { return err } + if useFetchHead { + ref = "FETCH_HEAD" + } if t.opts.Verification != nil { err := Verification{dstPath, *t.opts.Verification, t.refFetcher}.Verify(ref) @@ -206,6 +232,18 @@ func (t *Git) fetch(dstPath string, tempArea ctlfetch.TempArea, bundle string) e return err } + // In locked mode we fetched by the original named ref; verify the resulting + // commit matches the SHA recorded in the lock file. + if useFetchHead && isHexSHA(t.opts.Ref) { + out, _, err := t.cmdRunner.Run([]string{"rev-parse", "HEAD"}, nil, dstPath) + if err != nil { + return err + } + if strings.TrimSpace(out) != t.opts.Ref { + return fmt.Errorf("Locked SHA %s does not match fetched commit %s — tag may have been moved", t.opts.Ref, strings.TrimSpace(out)) + } + } + if !t.opts.SkipInitSubmodules { _, _, err = t.cmdRunner.Run([]string{"submodule", "update", "--init", "--recursive"}, env, dstPath) if err != nil { @@ -245,6 +283,21 @@ func (t *Git) tags(dstPath string) ([]string, error) { return strings.Split(out, "\n"), nil } +// isHexSHA reports whether s looks like a full or abbreviated git commit SHA +// (7–40 lowercase hex characters). SHAs cannot be fetched by refspec name and +// require a full fetch to be resolved. +func isHexSHA(s string) bool { + if len(s) < 7 || len(s) > 40 { + return false + } + for _, c := range s { + if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')) { + return false + } + } + return true +} + type CommandRunner interface { RunMultiple(argss [][]string, env []string, dstPath string) error Run(args []string, env []string, dstPath string) (string, string, error) diff --git a/pkg/vendir/fetch/git/git_test.go b/pkg/vendir/fetch/git/git_test.go index 3ccdd286..daf7a046 100644 --- a/pkg/vendir/fetch/git/git_test.go +++ b/pkg/vendir/fetch/git/git_test.go @@ -11,6 +11,7 @@ import ( "carvel.dev/vendir/pkg/vendir/config" "carvel.dev/vendir/pkg/vendir/fetch" "carvel.dev/vendir/pkg/vendir/fetch/git" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -65,10 +66,159 @@ func TestGit_Retrieve(t *testing.T) { _, err := gitRetriever.Retrieve("", &tmpFolder{t}, "") require.ErrorContains(t, err, "Username/password authentication is only supported for https remotes") }) + + t.Run("Named tag ref uses targeted fetch with --no-tags", func(t *testing.T) { + runner := &cmdRunnerLocal{commandsToRun: [][]string{}} + gitRetriever := git.NewGitWithRunner(config.DirectoryContentsGit{ + URL: "https://some.git/repo", + Ref: "v1.2.3", + }, os.Stdout, &fetch.SingleSecretRefFetcher{}, runner) + _, err := gitRetriever.Retrieve("", &tmpFolder{t}, "") + require.NoError(t, err) + + fetchArgs := findCommandArgs(runner.commandsToRun, "fetch") + require.NotNil(t, fetchArgs, "expected a fetch command") + assert.Contains(t, fetchArgs, "v1.2.3", "expected specific ref in fetch") + assert.Contains(t, fetchArgs, "--no-tags", "expected --no-tags for named ref fetch") + + tagOptArgs := findConfigArgs(runner.commandsToRun, "remote.origin.tagOpt") + require.NotNil(t, tagOptArgs, "expected tagOpt config command") + assert.Equal(t, "--no-tags", tagOptArgs[len(tagOptArgs)-1], "expected --no-tags tagOpt for named ref") + + checkoutArgs := findCheckoutArgs(runner.commandsToRun) + require.NotNil(t, checkoutArgs, "expected a checkout command") + assert.Contains(t, checkoutArgs, "FETCH_HEAD", "expected FETCH_HEAD checkout for targeted fetch") + }) + + t.Run("origin/ branch ref uses branch-scoped fetch without --no-tags on refspec", func(t *testing.T) { + runner := &cmdRunnerLocal{commandsToRun: [][]string{}} + gitRetriever := git.NewGitWithRunner(config.DirectoryContentsGit{ + URL: "https://some.git/repo", + Ref: "origin/main", + }, os.Stdout, &fetch.SingleSecretRefFetcher{}, runner) + _, err := gitRetriever.Retrieve("", &tmpFolder{t}, "") + require.NoError(t, err) + + fetchArgs := findCommandArgs(runner.commandsToRun, "fetch") + require.NotNil(t, fetchArgs, "expected a fetch command") + assert.Contains(t, fetchArgs, "main", "expected branch name in fetch for origin/ ref") + assert.NotContains(t, fetchArgs, "--no-tags", "expected no --no-tags on the refspec for origin/ branch") + + checkoutArgs := findCheckoutArgs(runner.commandsToRun) + require.NotNil(t, checkoutArgs, "expected a checkout command") + assert.Contains(t, checkoutArgs, "main", "expected branch name checkout for origin/ ref") + assert.NotContains(t, checkoutArgs, "FETCH_HEAD", "expected direct checkout for origin/ ref, not FETCH_HEAD") + }) + + t.Run("Locked SHA with OriginalRef uses OriginalRef for targeted fetch", func(t *testing.T) { + lockedSHA := "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2" + runner := &cmdRunnerLocal{ + commandsToRun: [][]string{}, + // rev-parse HEAD must return the locked SHA for the post-checkout verification to pass + responses: map[string]string{"rev-parse HEAD": lockedSHA}, + } + gitRetriever := git.NewGitWithRunner(config.DirectoryContentsGit{ + URL: "https://some.git/repo", + Ref: lockedSHA, + OriginalRef: "v1.2.3", + }, os.Stdout, &fetch.SingleSecretRefFetcher{}, runner) + _, err := gitRetriever.Retrieve("", &tmpFolder{t}, "") + require.NoError(t, err) + + fetchArgs := findCommandArgs(runner.commandsToRun, "fetch") + require.NotNil(t, fetchArgs, "expected a fetch command") + assert.Contains(t, fetchArgs, "v1.2.3", "expected OriginalRef in fetch for locked SHA mode") + assert.Contains(t, fetchArgs, "--no-tags", "expected --no-tags for locked SHA with OriginalRef") + + tagOptArgs := findConfigArgs(runner.commandsToRun, "remote.origin.tagOpt") + require.NotNil(t, tagOptArgs) + assert.Equal(t, "--no-tags", tagOptArgs[len(tagOptArgs)-1], "expected --no-tags tagOpt for locked SHA with OriginalRef") + + checkoutArgs := findCheckoutArgs(runner.commandsToRun) + require.NotNil(t, checkoutArgs) + assert.Contains(t, checkoutArgs, "FETCH_HEAD", "expected FETCH_HEAD checkout in locked SHA mode") + + revParseArgs := findCommandArgs(runner.commandsToRun, "rev-parse") + require.NotNil(t, revParseArgs, "expected rev-parse for SHA verification") + assert.Contains(t, revParseArgs, "HEAD") + }) + + t.Run("Plain SHA ref without OriginalRef uses full fetch with --tags", func(t *testing.T) { + runner := &cmdRunnerLocal{commandsToRun: [][]string{}} + gitRetriever := git.NewGitWithRunner(config.DirectoryContentsGit{ + URL: "https://some.git/repo", + Ref: "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2", // 40-char hex SHA, no OriginalRef + }, os.Stdout, &fetch.SingleSecretRefFetcher{}, runner) + _, err := gitRetriever.Retrieve("", &tmpFolder{t}, "") + require.NoError(t, err) + + fetchArgs := findCommandArgs(runner.commandsToRun, "fetch") + require.NotNil(t, fetchArgs, "expected a fetch command") + // full fetch: only "fetch" and "origin", no extra refspec + assert.Equal(t, []string{"fetch", "origin"}, fetchArgs, "expected bare full fetch for plain SHA") + + tagOptArgs := findConfigArgs(runner.commandsToRun, "remote.origin.tagOpt") + require.NotNil(t, tagOptArgs) + assert.Equal(t, "--tags", tagOptArgs[len(tagOptArgs)-1], "expected --tags tagOpt for plain SHA ref") + }) + + t.Run("Depth flag is appended after refspec for named ref targeted fetch", func(t *testing.T) { + runner := &cmdRunnerLocal{commandsToRun: [][]string{}} + gitRetriever := git.NewGitWithRunner(config.DirectoryContentsGit{ + URL: "https://some.git/repo", + Ref: "v1.2.3", + Depth: 1, + }, os.Stdout, &fetch.SingleSecretRefFetcher{}, runner) + _, err := gitRetriever.Retrieve("", &tmpFolder{t}, "") + require.NoError(t, err) + + fetchArgs := findCommandArgs(runner.commandsToRun, "fetch") + require.NotNil(t, fetchArgs) + fetchStr := strings.Join(fetchArgs, " ") + assert.Contains(t, fetchStr, "v1.2.3 --no-tags --depth 1", "expected refspec, --no-tags, then --depth in order") + }) +} + +// findCommandArgs finds the first command in commandsToRun whose first arg matches firstArg. +func findCommandArgs(commands [][]string, firstArg string) []string { + for _, args := range commands { + if len(args) > 0 && args[0] == firstArg { + return args + } + } + return nil +} + +// findCheckoutArgs finds the git checkout command (issued as "-c advice.detachedHead=false checkout ..."). +func findCheckoutArgs(commands [][]string) []string { + for _, args := range commands { + for i, a := range args { + if a == "checkout" { + return args[i:] + } + } + } + return nil +} + +// findConfigArgs finds the git config command that sets the given key. +func findConfigArgs(commands [][]string, key string) []string { + for _, args := range commands { + if len(args) >= 2 && args[0] == "config" { + for _, a := range args[1:] { + if a == key { + return args + } + } + } + } + return nil } type cmdRunnerLocal struct { commandsToRun [][]string + // responses maps "arg0 arg1 ..." to the stdout value Run should return. + responses map[string]string } func (c *cmdRunnerLocal) RunMultiple(argss [][]string, _ []string, _ string) error { @@ -78,6 +228,11 @@ func (c *cmdRunnerLocal) RunMultiple(argss [][]string, _ []string, _ string) err func (c *cmdRunnerLocal) Run(args []string, _ []string, _ string) (string, string, error) { c.commandsToRun = append(c.commandsToRun, args) + if c.responses != nil { + if out, ok := c.responses[strings.Join(args, " ")]; ok { + return out, "", nil + } + } return "", "", nil } diff --git a/pkg/vendir/fetch/http/sync.go b/pkg/vendir/fetch/http/sync.go index f95ae138..43d5c229 100644 --- a/pkg/vendir/fetch/http/sync.go +++ b/pkg/vendir/fetch/http/sync.go @@ -195,8 +195,7 @@ func (t *Sync) addAuth(req *http.Request) error { // Basic auth — password is optional, defaults to empty string if hasUser { - password := string(secret.Data[ - ctlconf.SecretK8sCorev1BasicAuthPasswordKey]) + password := string(secret.Data[ctlconf.SecretK8sCorev1BasicAuthPasswordKey]) req.SetBasicAuth( string(secret.Data[ctlconf.SecretK8sCorev1BasicAuthUsernameKey]), password, diff --git a/test/e2e/git_test.go b/test/e2e/git_test.go index a5468739..97720913 100644 --- a/test/e2e/git_test.go +++ b/test/e2e/git_test.go @@ -235,6 +235,131 @@ directories: require.True(t, unbundled, "git did not use the cached bundle") } +func TestGitFetchOptimizations(t *testing.T) { + env := BuildEnv(t) + logger := Logger{} + vendir := Vendir{t, env.BinaryPath, logger} + + gitSrcPath, err := os.MkdirTemp("", "vendir-e2e-git-fetch-opt") + require.NoError(t, err) + defer os.RemoveAll(gitSrcPath) + + out, err := exec.Command("tar", "xzvf", "assets/git-repo-signed/asset.tgz", "-C", gitSrcPath).CombinedOutput() + require.NoErrorf(t, err, "Unpacking git-repo-signed asset (output: '%s')", out) + + repoPath := filepath.Join(gitSrcPath, "git-repo") + + // SHAs are stable — baked into the tarball. + const ( + signedTrustedTagSHA = "35a63c8ba40a44f6bdf16c6ea8ae589d8539cc5b" + masterHeadSHA = "f0076929d229f0819eabd2d1937bdb6aa148f19f" + ) + + yamlConfig := func(ref string) string { + return fmt.Sprintf(` +apiVersion: vendir.k14s.io/v1alpha1 +kind: Config +directories: +- path: vendor + contents: + - path: test + git: + url: "%s" + ref: "%s" +`, repoPath, ref) + } + + logger.Section("named tag fetch uses targeted FETCH_HEAD path", func() { + dstPath, err := os.MkdirTemp("", "vendir-e2e-git-fetch-opt-dst") + require.NoError(t, err) + defer os.RemoveAll(dstPath) + + var stdout bytes.Buffer + vendir.RunWithOpts( + []string{"sync", "-f", "-", "--json"}, + RunOpts{Dir: dstPath, StdinReader: strings.NewReader(yamlConfig("signed-trusted-tag")), StdoutWriter: &stdout}, + ) + + var vendirOutput VendirOutput + require.NoError(t, json.NewDecoder(&stdout).Decode(&vendirOutput)) + + var foundTargetedFetch bool + for _, l := range vendirOutput.Lines { + if strings.Contains(l, "fetch origin signed-trusted-tag --no-tags") { + foundTargetedFetch = true + } + assert.NotContains(t, l, "--> git fetch origin\n", "expected no bare full fetch for named tag ref") + } + assert.True(t, foundTargetedFetch, "expected targeted fetch line 'fetch origin signed-trusted-tag --no-tags'") + + _, err = os.Stat(filepath.Join(dstPath, "vendor", "test")) + assert.NoError(t, err, "expected vendored directory to exist after sync") + }) + + logger.Section("locked sync uses OriginalRef for targeted fetch (not bare SHA fetch)", func() { + dstPath, err := os.MkdirTemp("", "vendir-e2e-git-fetch-opt-locked-dst") + require.NoError(t, err) + defer os.RemoveAll(dstPath) + + // First sync to produce a lock file + vendir.RunWithOpts( + []string{"sync", "-f", "-"}, + RunOpts{Dir: dstPath, StdinReader: strings.NewReader(yamlConfig("signed-trusted-tag"))}, + ) + + // Locked sync: should use OriginalRef ("signed-trusted-tag") not the SHA + var stdout bytes.Buffer + vendir.RunWithOpts( + []string{"sync", "--locked", "-f", "-", "--json"}, + RunOpts{Dir: dstPath, StdinReader: strings.NewReader(yamlConfig("signed-trusted-tag")), StdoutWriter: &stdout}, + ) + + var vendirOutput VendirOutput + require.NoError(t, json.NewDecoder(&stdout).Decode(&vendirOutput)) + + var foundTargetedFetch bool + for _, l := range vendirOutput.Lines { + if strings.Contains(l, "fetch origin signed-trusted-tag --no-tags") { + foundTargetedFetch = true + } + } + assert.True(t, foundTargetedFetch, "expected locked sync to use OriginalRef for targeted fetch") + }) + + logger.Section("locked sync detects tampered lock SHA", func() { + dstPath, err := os.MkdirTemp("", "vendir-e2e-git-fetch-opt-tampered-dst") + require.NoError(t, err) + defer os.RemoveAll(dstPath) + + // Write a vendir.yml pointing at the tag + err = os.WriteFile(filepath.Join(dstPath, "vendir.yml"), []byte(yamlConfig("signed-trusted-tag")), 0600) + require.NoError(t, err) + + // Write a lock file with a different (valid) SHA — masterHeadSHA ≠ signedTrustedTagSHA + lockContent := fmt.Sprintf(`apiVersion: vendir.k14s.io/v1alpha1 +kind: LockConfig +directories: +- path: vendor + contents: + - path: test + git: + sha: "%s" + commitTitle: tampered +`, masterHeadSHA) + err = os.WriteFile(filepath.Join(dstPath, "vendir.lock.yml"), []byte(lockContent), 0600) + require.NoError(t, err) + + _, err = vendir.RunWithOpts( + []string{"sync", "--locked"}, + RunOpts{Dir: dstPath, AllowError: true}, + ) + require.Error(t, err, "expected error when lock SHA does not match fetched commit") + assert.Contains(t, err.Error(), "Locked SHA", "expected error to mention locked SHA") + assert.Contains(t, err.Error(), "does not match fetched commit", "expected error to describe the mismatch") + _ = signedTrustedTagSHA // used in comment above for clarity + }) +} + func readFile(t *testing.T, path string) string { contents, err := os.ReadFile(path) if err != nil { From 04d5899b4bf229e85972c03c0d99999358444ec1 Mon Sep 17 00:00:00 2001 From: Steve Lowery Date: Thu, 25 Jun 2026 07:43:12 -0500 Subject: [PATCH 2/2] Address PR review feedback - 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 --- pkg/vendir/fetch/git/git.go | 42 ++++++++++++++++++-------------- pkg/vendir/fetch/git/git_test.go | 32 +++++++++++++++++++++--- test/e2e/git_test.go | 8 ++++-- 3 files changed, 58 insertions(+), 24 deletions(-) diff --git a/pkg/vendir/fetch/git/git.go b/pkg/vendir/fetch/git/git.go index e240e98e..04cdbcbb 100644 --- a/pkg/vendir/fetch/git/git.go +++ b/pkg/vendir/fetch/git/git.go @@ -197,8 +197,11 @@ func (t *Git) fetch(dstPath string, tempArea ctlfetch.TempArea, bundle string) e fetchArgs = append(fetchArgs, t.opts.Ref, "--no-tags") useFetchHead = true case isHexSHA(t.opts.Ref) && len(t.opts.OriginalRef) > 0 && !isHexSHA(t.opts.OriginalRef): - // locked mode: fetch by the original named ref so we avoid a full fetch - fetchArgs = append(fetchArgs, t.opts.OriginalRef, "--no-tags") + // locked mode: fetch by the original named ref so we avoid a full fetch. + // Strip any "origin/" prefix — that prefix is a local tracking convention + // and is not a valid remote refspec. + originalRef := strings.TrimPrefix(t.opts.OriginalRef, "origin/") + fetchArgs = append(fetchArgs, originalRef, "--no-tags") useFetchHead = true } if t.opts.Depth > 0 { @@ -216,7 +219,22 @@ func (t *Git) fetch(dstPath string, tempArea ctlfetch.TempArea, bundle string) e if err != nil { return err } - if useFetchHead { + + // In locked mode we fetch by the original named ref for performance, but must + // check out the exact locked SHA. Verify FETCH_HEAD resolves to that SHA before + // checkout so a force-pushed tag is caught early. + if useFetchHead && isHexSHA(t.opts.Ref) { + out, _, err := t.cmdRunner.Run([]string{"rev-parse", "FETCH_HEAD^{commit}"}, nil, dstPath) + if err != nil { + return err + } + if strings.TrimSpace(out) != t.opts.Ref { + return fmt.Errorf("Locked SHA %s does not match fetched commit %s — tag may have been moved", t.opts.Ref, strings.TrimSpace(out)) + } + // Use the locked SHA for checkout so mutable refs (e.g. origin/main) don't + // cause locked syncs to track the branch tip instead of the pinned commit. + ref = t.opts.Ref + } else if useFetchHead { ref = "FETCH_HEAD" } @@ -232,18 +250,6 @@ func (t *Git) fetch(dstPath string, tempArea ctlfetch.TempArea, bundle string) e return err } - // In locked mode we fetched by the original named ref; verify the resulting - // commit matches the SHA recorded in the lock file. - if useFetchHead && isHexSHA(t.opts.Ref) { - out, _, err := t.cmdRunner.Run([]string{"rev-parse", "HEAD"}, nil, dstPath) - if err != nil { - return err - } - if strings.TrimSpace(out) != t.opts.Ref { - return fmt.Errorf("Locked SHA %s does not match fetched commit %s — tag may have been moved", t.opts.Ref, strings.TrimSpace(out)) - } - } - if !t.opts.SkipInitSubmodules { _, _, err = t.cmdRunner.Run([]string{"submodule", "update", "--init", "--recursive"}, env, dstPath) if err != nil { @@ -284,14 +290,14 @@ func (t *Git) tags(dstPath string) ([]string, error) { } // isHexSHA reports whether s looks like a full or abbreviated git commit SHA -// (7–40 lowercase hex characters). SHAs cannot be fetched by refspec name and -// require a full fetch to be resolved. +// (7–40 hex characters, case-insensitive). SHAs cannot be fetched by refspec +// name and require a full fetch to be resolved. func isHexSHA(s string) bool { if len(s) < 7 || len(s) > 40 { return false } for _, c := range s { - if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')) { + if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F')) { return false } } diff --git a/pkg/vendir/fetch/git/git_test.go b/pkg/vendir/fetch/git/git_test.go index daf7a046..3bbb0231 100644 --- a/pkg/vendir/fetch/git/git_test.go +++ b/pkg/vendir/fetch/git/git_test.go @@ -114,8 +114,8 @@ func TestGit_Retrieve(t *testing.T) { lockedSHA := "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2" runner := &cmdRunnerLocal{ commandsToRun: [][]string{}, - // rev-parse HEAD must return the locked SHA for the post-checkout verification to pass - responses: map[string]string{"rev-parse HEAD": lockedSHA}, + // rev-parse FETCH_HEAD^{commit} must return the locked SHA for verification to pass + responses: map[string]string{"rev-parse FETCH_HEAD^{commit}": lockedSHA}, } gitRetriever := git.NewGitWithRunner(config.DirectoryContentsGit{ URL: "https://some.git/repo", @@ -134,13 +134,37 @@ func TestGit_Retrieve(t *testing.T) { require.NotNil(t, tagOptArgs) assert.Equal(t, "--no-tags", tagOptArgs[len(tagOptArgs)-1], "expected --no-tags tagOpt for locked SHA with OriginalRef") + // Checkout must use the locked SHA directly, not FETCH_HEAD, so mutable + // refs (e.g. origin/main) don't cause locked sync to track the branch tip. checkoutArgs := findCheckoutArgs(runner.commandsToRun) require.NotNil(t, checkoutArgs) - assert.Contains(t, checkoutArgs, "FETCH_HEAD", "expected FETCH_HEAD checkout in locked SHA mode") + assert.Contains(t, checkoutArgs, lockedSHA, "expected locked SHA checkout in locked SHA mode") + assert.NotContains(t, checkoutArgs, "FETCH_HEAD", "expected locked SHA, not FETCH_HEAD, for checkout") + // Verification: rev-parse FETCH_HEAD^{commit} must be called before checkout revParseArgs := findCommandArgs(runner.commandsToRun, "rev-parse") require.NotNil(t, revParseArgs, "expected rev-parse for SHA verification") - assert.Contains(t, revParseArgs, "HEAD") + assert.Contains(t, revParseArgs, "FETCH_HEAD^{commit}") + }) + + t.Run("Locked SHA with origin/ OriginalRef strips prefix before fetch", func(t *testing.T) { + lockedSHA := "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2" + runner := &cmdRunnerLocal{ + commandsToRun: [][]string{}, + responses: map[string]string{"rev-parse FETCH_HEAD^{commit}": lockedSHA}, + } + gitRetriever := git.NewGitWithRunner(config.DirectoryContentsGit{ + URL: "https://some.git/repo", + Ref: lockedSHA, + OriginalRef: "origin/main", // common in vendir configs + }, os.Stdout, &fetch.SingleSecretRefFetcher{}, runner) + _, err := gitRetriever.Retrieve("", &tmpFolder{t}, "") + require.NoError(t, err) + + fetchArgs := findCommandArgs(runner.commandsToRun, "fetch") + require.NotNil(t, fetchArgs) + assert.Contains(t, fetchArgs, "main", "expected origin/ prefix stripped from OriginalRef") + assert.NotContains(t, fetchArgs, "origin/main", "expected origin/ prefix stripped before fetch refspec") }) t.Run("Plain SHA ref without OriginalRef uses full fetch with --tags", func(t *testing.T) { diff --git a/test/e2e/git_test.go b/test/e2e/git_test.go index 97720913..4aa2bbbe 100644 --- a/test/e2e/git_test.go +++ b/test/e2e/git_test.go @@ -283,14 +283,18 @@ directories: var vendirOutput VendirOutput require.NoError(t, json.NewDecoder(&stdout).Decode(&vendirOutput)) - var foundTargetedFetch bool + var foundTargetedFetch, foundBareFetch bool for _, l := range vendirOutput.Lines { if strings.Contains(l, "fetch origin signed-trusted-tag --no-tags") { foundTargetedFetch = true } - assert.NotContains(t, l, "--> git fetch origin\n", "expected no bare full fetch for named tag ref") + // "--> git fetch origin" followed by a depth/no refspec is the old slow path + if strings.Contains(l, "--> git fetch origin") && !strings.Contains(l, "signed-trusted-tag") { + foundBareFetch = true + } } assert.True(t, foundTargetedFetch, "expected targeted fetch line 'fetch origin signed-trusted-tag --no-tags'") + assert.False(t, foundBareFetch, "expected no bare full fetch for named tag ref") _, err = os.Stat(filepath.Join(dstPath, "vendor", "test")) assert.NoError(t, err, "expected vendored directory to exist after sync")