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 <[email protected]>

Author: tonistiigi

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
 }

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",