From 44b1aca26a97107190c6b89e8277183e12ad17a0 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Tue, 22 Oct 2024 17:45:06 -0700 Subject: [PATCH] git: fix caching git commit through multiple refs This fixes current issue when a Git commit is accessed multiple times through different refs or ref is added after commit has already been pulled once. When keep-git-dir option is true, then program can try to resolve the current reference via .git directory and because old cache key was only the git commit, previous .git directory can be reused without any refs inside. There is no change to the behavior if keep-git-dir is false as then requests through multiple refs yield to identical content. Only the reference in the user provided identifier is added to the cache key, and that is the only one that can be expected in .git because of the shallow fetches. We do not do extra request to find named refs for a commit SHA if that is provided in the identifier. Signed-off-by: Tonis Tiigi --- source/git/source.go | 13 +++- source/git/source_test.go | 141 ++++++++++++++++++++++++++++++++++---- 2 files changed, 137 insertions(+), 17 deletions(-) diff --git a/source/git/source.go b/source/git/source.go index cb56eeed5986..12b381196b06 100644 --- a/source/git/source.go +++ b/source/git/source.go @@ -196,10 +196,13 @@ type gitSourceHandler struct { authArgs []string } -func (gs *gitSourceHandler) shaToCacheKey(sha string) string { +func (gs *gitSourceHandler) shaToCacheKey(sha, ref string) string { key := sha if gs.src.KeepGitDir { key += ".git" + if ref != "" { + key += "#" + ref + } } if gs.src.Subdir != "" { key += ":" + gs.src.Subdir @@ -341,7 +344,7 @@ func (gs *gitSourceHandler) CacheKey(ctx context.Context, g session.Group, index defer gs.locker.Unlock(remote) if ref := gs.src.Ref; ref != "" && gitutil.IsCommitSHA(ref) { - cacheKey := gs.shaToCacheKey(ref) + cacheKey := gs.shaToCacheKey(ref, "") gs.cacheKey = cacheKey return cacheKey, ref, nil, true, nil } @@ -377,6 +380,7 @@ func (gs *gitSourceHandler) CacheKey(ctx context.Context, g session.Group, index annotatedTagRef = tagRef + "^{}" ) var sha, headSha, tagSha string + var usedRef string for _, line := range lines { lineSha, lineRef, _ := strings.Cut(line, "\t") switch lineRef { @@ -386,15 +390,18 @@ func (gs *gitSourceHandler) CacheKey(ctx context.Context, g session.Group, index tagSha = lineSha case partialRef: sha = lineSha + usedRef = lineRef } } // git-checkout prefers branches in case of ambiguity if sha == "" { sha = headSha + usedRef = headRef } if sha == "" { sha = tagSha + usedRef = tagRef } if sha == "" { return "", "", nil, false, errors.Errorf("repository does not contain ref %s, output: %q", ref, string(buf)) @@ -403,7 +410,7 @@ func (gs *gitSourceHandler) CacheKey(ctx context.Context, g session.Group, index return "", "", nil, false, errors.Errorf("invalid commit sha %q", sha) } - cacheKey := gs.shaToCacheKey(sha) + cacheKey := gs.shaToCacheKey(sha, usedRef) gs.cacheKey = cacheKey return cacheKey, sha, nil, true, nil } diff --git a/source/git/source_test.go b/source/git/source_test.go index 5955e697848c..863cf3090954 100644 --- a/source/git/source_test.go +++ b/source/git/source_test.go @@ -3,6 +3,7 @@ package git import ( "bytes" "context" + "fmt" "io" "net/http" "net/http/cgi" @@ -69,9 +70,10 @@ func testRepeatedFetch(t *testing.T, keepGitDir bool) { expLen := 40 if keepGitDir { expLen += 4 + require.GreaterOrEqual(t, len(key1), expLen) + } else { + require.Equal(t, expLen, len(key1)) } - - require.Equal(t, expLen, len(key1)) require.Equal(t, 40, len(pin1)) ref1, err := g.Snapshot(ctx, nil) @@ -189,9 +191,10 @@ func testFetchBySHA(t *testing.T, keepGitDir bool) { expLen := 40 if keepGitDir { expLen += 4 + require.GreaterOrEqual(t, len(key1), expLen) + } else { + require.Equal(t, expLen, len(key1)) } - - require.Equal(t, expLen, len(key1)) require.Equal(t, 40, len(pin1)) ref1, err := g.Snapshot(ctx, nil) @@ -276,9 +279,10 @@ func testFetchUnreferencedRefSha(t *testing.T, ref string, keepGitDir bool) { expLen := 40 if keepGitDir { expLen += 4 + require.GreaterOrEqual(t, len(key1), expLen) + } else { + require.Equal(t, expLen, len(key1)) } - - require.Equal(t, expLen, len(key1)) require.Equal(t, 40, len(pin1)) ref1, err := g.Snapshot(ctx, nil) @@ -372,9 +376,10 @@ func testFetchByTag(t *testing.T, tag, expectedCommitSubject string, isAnnotated expLen := 40 if keepGitDir { expLen += 4 + require.GreaterOrEqual(t, len(key1), expLen) + } else { + require.Equal(t, expLen, len(key1)) } - - require.Equal(t, expLen, len(key1)) require.Equal(t, 40, len(pin1)) ref1, err := g.Snapshot(ctx, nil) @@ -447,6 +452,105 @@ func testFetchByTag(t *testing.T, tag, expectedCommitSubject string, isAnnotated } } +func TestMultipleTagAccessKeepGitDir(t *testing.T) { + testMultipleTagAccess(t, true) +} + +func TestMultipleTagAccess(t *testing.T) { + testMultipleTagAccess(t, false) +} + +func testMultipleTagAccess(t *testing.T, keepGitDir bool) { + if runtime.GOOS == "windows" { + t.Skip("Depends on unimplemented containerd bind-mount support on Windows") + } + + t.Parallel() + ctx := namespaces.WithNamespace(context.Background(), "buildkit-test") + ctx = logProgressStreams(ctx, t) + + gs := setupGitSource(t, t.TempDir()) + + repo := setupGitRepo(t) + + id := &GitIdentifier{Remote: repo.mainURL, KeepGitDir: keepGitDir, Ref: "a/v1.2.3"} + + g, err := gs.Resolve(ctx, id, nil, nil) + require.NoError(t, err) + + expLen := 40 + if keepGitDir { + expLen += 4 + } + + key1, pin1, _, _, err := g.CacheKey(ctx, nil, 0) + require.NoError(t, err) + if keepGitDir { + require.GreaterOrEqual(t, len(key1), expLen) + } else { + require.Equal(t, expLen, len(key1)) + } + require.Equal(t, 40, len(pin1)) + + ref1, err := g.Snapshot(ctx, nil) + require.NoError(t, err) + defer ref1.Release(context.TODO()) + + id2 := &GitIdentifier{Remote: repo.mainURL, KeepGitDir: keepGitDir, Ref: "a/v1.2.3-same"} + g2, err := gs.Resolve(ctx, id2, nil, nil) + require.NoError(t, err) + + key2, pin2, _, _, err := g2.CacheKey(ctx, nil, 0) + require.NoError(t, err) + if keepGitDir { + require.GreaterOrEqual(t, len(key1), expLen) + } else { + require.Equal(t, expLen, len(key1)) + } + require.Equal(t, 40, len(pin2)) + + require.Equal(t, pin1, pin2) + if !keepGitDir { + require.Equal(t, key1, key2) + return + } + // key should be different because of the ref + require.NotEqual(t, key1, key2) + + ref2, err := g2.Snapshot(ctx, nil) + require.NoError(t, err) + defer ref1.Release(context.TODO()) + + mount1, err := ref2.Mount(ctx, true, nil) + require.NoError(t, err) + + lm1 := snapshot.LocalMounter(mount1) + dir1, err := lm1.Mount() + require.NoError(t, err) + defer lm1.Unmount() + + workDir := t.TempDir() + + runShell(t, dir1, fmt.Sprintf(`git rev-parse a/v1.2.3 > %s/ref1`, workDir)) + + dt1, err := os.ReadFile(filepath.Join(workDir, "ref1")) + require.NoError(t, err) + + mount2, err := ref2.Mount(ctx, true, nil) + require.NoError(t, err) + + lm2 := snapshot.LocalMounter(mount2) + dir2, err := lm2.Mount() + require.NoError(t, err) + defer lm2.Unmount() + + runShell(t, dir2, fmt.Sprintf(`git rev-parse a/v1.2.3-same > %s/ref2`, workDir)) + + dt2, err := os.ReadFile(filepath.Join(workDir, "ref2")) + require.NoError(t, err) + require.Equal(t, string(dt1), string(dt2)) +} + func TestMultipleRepos(t *testing.T) { testMultipleRepos(t, false) } @@ -496,12 +600,20 @@ func testMultipleRepos(t *testing.T, keepGitDir bool) { key1, pin1, _, _, err := g.CacheKey(ctx, nil, 0) require.NoError(t, err) - require.Equal(t, expLen, len(key1)) + if keepGitDir { + require.GreaterOrEqual(t, len(key1), expLen) + } else { + require.Equal(t, expLen, len(key1)) + } require.Equal(t, 40, len(pin1)) key2, pin2, _, _, err := g2.CacheKey(ctx, nil, 0) require.NoError(t, err) - require.Equal(t, expLen, len(key2)) + if keepGitDir { + require.GreaterOrEqual(t, len(key2), expLen) + } else { + require.Equal(t, expLen, len(key2)) + } require.Equal(t, 40, len(pin2)) require.NotEqual(t, key1, key2) @@ -608,9 +720,10 @@ func testSubdir(t *testing.T, keepGitDir bool) { expLen := 44 if keepGitDir { expLen += 4 + require.GreaterOrEqual(t, len(key1), expLen) + } else { + require.Equal(t, expLen, len(key1)) } - - require.Equal(t, expLen, len(key1)) require.Equal(t, 40, len(pin1)) ref1, err := g.Snapshot(ctx, nil) @@ -712,7 +825,7 @@ func setupGitRepo(t *testing.T) gitRepoFixture { // | * (tag: refs/tags/v1.2.3-special) tagonly-leaf // |/ // * (tag: refs/tags/v1.2.3) second - // * (tag: refs/tags/a/v1.2.3) initial + // * (tag: refs/tags/a/v1.2.3, refs/tags/a/v1.2.3-same) initial runShell(t, fixture.mainPath, "git -c init.defaultBranch=master init", "git config --local user.email test", @@ -722,7 +835,7 @@ func setupGitRepo(t *testing.T) gitRepoFixture { "git add abc", "git commit -m initial", "git tag --no-sign a/v1.2.3", - + "git tag --no-sign a/v1.2.3-same", "echo bar > def", "mkdir subdir", "echo subcontents > subdir/subfile",