Skip to content

Commit 31c7091

Browse files
tonistiigitonistiigi
authored
Merge pull request #6259 from tonistiigi/git-tag-mutation-fix
git: handle tag changes in upstream
2 parents 1742649 + 9f5cf39 commit 31c7091

File tree

2 files changed

+239
-16
lines changed

2 files changed

+239
-16
lines changed

‎source/git/source.go‎

Lines changed: 72 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -105,24 +105,30 @@ func (gs *gitSource) Identifier(scheme, ref string, attrs map[string]string, pla
105105
}
106106

107107
// needs to be called with repo lock
108-
func (gs *gitSource) mountRemote(ctx context.Context, remote string, authArgs []string, sha256 bool, g session.Group) (target string, release func() error, retErr error) {
108+
func (gs *gitSource) mountRemote(ctx context.Context, remote string, authArgs []string, sha256 bool, reset bool, g session.Group) (target string, release func() error, retErr error) {
109109
sis, err := searchGitRemote(ctx, gs.cache, remote)
110110
if err != nil {
111111
return "", nil, errors.Wrapf(err, "failed to search metadata for %s", urlutil.RedactCredentials(remote))
112112
}
113113

114114
var remoteRef cache.MutableRef
115115
for _, si := range sis {
116-
remoteRef, err = gs.cache.GetMutable(ctx, si.ID())
117-
if err != nil {
118-
if errors.Is(err, cache.ErrLocked) {
119-
// should never really happen as no other function should access this metadata, but lets be graceful
120-
bklog.G(ctx).Warnf("mutable ref for %s %s was locked: %v", urlutil.RedactCredentials(remote), si.ID(), err)
121-
continue
116+
if reset {
117+
if err := si.clearGitRemote(); err != nil {
118+
bklog.G(ctx).Warnf("failed to clear git remote metadata for %s %s: %v", urlutil.RedactCredentials(remote), si.ID(), err)
119+
}
120+
} else {
121+
remoteRef, err = gs.cache.GetMutable(ctx, si.ID())
122+
if err != nil {
123+
if errors.Is(err, cache.ErrLocked) {
124+
// should never really happen as no other function should access this metadata, but lets be graceful
125+
bklog.G(ctx).Warnf("mutable ref for %s %s was locked: %v", urlutil.RedactCredentials(remote), si.ID(), err)
126+
continue
127+
}
128+
return "", nil, errors.Wrapf(err, "failed to get mutable ref for %s", urlutil.RedactCredentials(remote))
122129
}
123-
return "", nil, errors.Wrapf(err, "failed to get mutable ref for %s", urlutil.RedactCredentials(remote))
130+
break
124131
}
125-
break
126132
}
127133

128134
initializeRepo := false
@@ -464,7 +470,7 @@ func (gs *gitSourceHandler) CacheKey(ctx context.Context, g session.Group, index
464470
return cacheKey, sha, nil, true, nil
465471
}
466472

467-
func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out cache.ImmutableRef, retErr error) {
473+
func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (cache.ImmutableRef, error) {
468474
cacheKey := gs.cacheKey
469475
if cacheKey == "" {
470476
var err error
@@ -491,13 +497,35 @@ func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out
491497
gs.locker.Lock(gs.src.Remote)
492498
defer gs.locker.Unlock(gs.src.Remote)
493499

500+
ref, err := gs.trySnapshot(ctx, g, false)
501+
if err != nil {
502+
var wce *wouldClobberExistingTagError
503+
var ulre *unableToUpdateLocalRefError
504+
if errors.As(err, &wce) || errors.As(err, &ulre) {
505+
ref, err = gs.trySnapshot(ctx, g, true)
506+
if err != nil {
507+
return nil, err
508+
}
509+
} else {
510+
return nil, err
511+
}
512+
}
513+
514+
md := cacheRefMetadata{ref}
515+
if err := md.setGitSnapshot(snapshotKey); err != nil {
516+
return nil, err
517+
}
518+
return ref, nil
519+
}
520+
521+
func (gs *gitSourceHandler) trySnapshot(ctx context.Context, g session.Group, reset bool) (out cache.ImmutableRef, retErr error) {
494522
git, cleanup, err := gs.emptyGitCli(ctx, g)
495523
if err != nil {
496524
return nil, err
497525
}
498526
defer cleanup()
499527

500-
gitDir, unmountGitDir, err := gs.mountRemote(ctx, gs.src.Remote, gs.authArgs, gs.sha256, g)
528+
gitDir, unmountGitDir, err := gs.mountRemote(ctx, gs.src.Remote, gs.authArgs, gs.sha256, reset, g)
501529
if err != nil {
502530
return nil, err
503531
}
@@ -547,7 +575,19 @@ func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out
547575
args = append(args, "--force", ref+":"+targetRef)
548576
}
549577
if _, err := git.Run(ctx, args...); err != nil {
550-
return nil, errors.Wrapf(err, "failed to fetch remote %s", urlutil.RedactCredentials(gs.src.Remote))
578+
err := errors.Wrapf(err, "failed to fetch remote %s", urlutil.RedactCredentials(gs.src.Remote))
579+
if strings.Contains(err.Error(), "rejected") && strings.Contains(err.Error(), "(would clobber existing tag)") {
580+
// this can happen if a tag was mutated to another commit in remote.
581+
// only hope is to abandon the existing shared repo and start a fresh one
582+
return nil, &wouldClobberExistingTagError{err}
583+
}
584+
if strings.Contains(err.Error(), "(unable to update local ref)") && strings.Contains(err.Error(), "some local refs could not be updated;") {
585+
// this can happen if a branch updated in remote so that old branch
586+
// is now a parent dir of a new branch
587+
return nil, &unableToUpdateLocalRefError{err}
588+
}
589+
590+
return nil, err
551591
}
552592
_, err = git.Run(ctx, "reflog", "expire", "--all", "--expire=now")
553593
if err != nil {
@@ -748,13 +788,25 @@ func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out
748788
}
749789
}()
750790

751-
md := cacheRefMetadata{snap}
752-
if err := md.setGitSnapshot(snapshotKey); err != nil {
753-
return nil, err
754-
}
755791
return snap, nil
756792
}
757793

794+
type wouldClobberExistingTagError struct {
795+
error
796+
}
797+
798+
func (e *wouldClobberExistingTagError) Unwrap() error {
799+
return e.error
800+
}
801+
802+
type unableToUpdateLocalRefError struct {
803+
error
804+
}
805+
806+
func (e *unableToUpdateLocalRefError) Unwrap() error {
807+
return e.error
808+
}
809+
758810
func (gs *gitSourceHandler) emptyGitCli(ctx context.Context, g session.Group, opts ...gitutil.Option) (*gitutil.GitCLI, func() error, error) {
759811
var cleanups []func() error
760812
cleanup := func() error {
@@ -863,6 +915,10 @@ func (md cacheRefMetadata) setGitRemote(key string) error {
863915
return md.SetString(keyGitRemote, key, gitRemoteIndex+key)
864916
}
865917

918+
func (md cacheRefMetadata) clearGitRemote() error {
919+
return md.ClearValueAndIndex(keyGitRemote, gitRemoteIndex)
920+
}
921+
866922
func gitCLI(opts ...gitutil.Option) *gitutil.GitCLI {
867923
opts = append([]gitutil.Option{
868924
gitutil.WithExec(runWithStandardUmask),

‎source/git/source_test.go‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -831,6 +831,173 @@ func testFetchAnnotatedTagChecksums(t *testing.T, format string, keepGitDir bool
831831
}
832832
}
833833

834+
func TestFetchMutatedTagSHA1(t *testing.T) {
835+
testFetchMutatedTag(t, "sha1", false)
836+
}
837+
838+
func TestFetchMutatedTagKeepGitDirSHA1(t *testing.T) {
839+
testFetchMutatedTag(t, "sha1", true)
840+
}
841+
842+
func TestFetchMutatedTagSHA256(t *testing.T) {
843+
testFetchMutatedTag(t, "sha256", false)
844+
}
845+
846+
func TestFetchMutatedTagKeepGitDirSHA256(t *testing.T) {
847+
testFetchMutatedTag(t, "sha256", true)
848+
}
849+
850+
func testFetchMutatedTag(t *testing.T, format string, keepGitDir bool) {
851+
ctx := t.Context()
852+
if runtime.GOOS == "windows" {
853+
t.Skip("Depends on unimplemented containerd bind-mount support on Windows")
854+
}
855+
repo := setupGitRepo(t, format)
856+
cmd := exec.Command("git", "rev-parse", "v1.2.3")
857+
cmd.Dir = repo.mainPath
858+
859+
out, err := cmd.Output()
860+
require.NoError(t, err)
861+
862+
expLen := 40
863+
if format == "sha256" {
864+
expLen = 64
865+
}
866+
shaTag := strings.TrimSpace(string(out))
867+
require.Equal(t, expLen, len(shaTag))
868+
869+
id := &GitIdentifier{Remote: repo.mainURL, Ref: shaTag, KeepGitDir: keepGitDir}
870+
gs := setupGitSource(t, t.TempDir())
871+
872+
g, err := gs.Resolve(ctx, id, nil, nil)
873+
require.NoError(t, err)
874+
875+
key1, pin1, _, done, err := g.CacheKey(ctx, nil, 0)
876+
require.NoError(t, err)
877+
require.True(t, done)
878+
879+
ref, err := g.Snapshot(ctx, nil)
880+
require.NoError(t, err)
881+
ref.Release(context.TODO())
882+
883+
// mutate the tag to point to another commit
884+
cmd = exec.Command("git", "tag", "-f", "v1.2.3", "feature")
885+
cmd.Dir = repo.mainPath
886+
out, err = cmd.CombinedOutput()
887+
require.NoError(t, err, string(out))
888+
889+
// verify that the tag now points to a different commit
890+
cmd = exec.Command("git", "rev-parse", "v1.2.3")
891+
cmd.Dir = repo.mainPath
892+
893+
out, err = cmd.Output()
894+
require.NoError(t, err)
895+
896+
shaTagMutated := strings.TrimSpace(string(out))
897+
require.Equal(t, expLen, len(shaTagMutated))
898+
require.NotEqual(t, shaTag, shaTagMutated)
899+
900+
id = &GitIdentifier{Remote: repo.mainURL, Ref: shaTagMutated, KeepGitDir: keepGitDir}
901+
902+
// fetching the original tag should still return the original commit because of caching
903+
g, err = gs.Resolve(ctx, id, nil, nil)
904+
require.NoError(t, err)
905+
906+
key2, pin2, _, done, err := g.CacheKey(ctx, nil, 0)
907+
require.NoError(t, err)
908+
require.True(t, done)
909+
910+
require.NotEqual(t, key1, key2)
911+
require.NotEqual(t, pin1, pin2)
912+
913+
ref, err = g.Snapshot(ctx, nil)
914+
require.NoError(t, err)
915+
ref.Release(context.TODO())
916+
}
917+
918+
func TestFetchMutatedBranchSHA1(t *testing.T) {
919+
testFetchMutatedBranch(t, "sha1", false)
920+
}
921+
922+
func TestFetchMutatedBranchKeepGitDirSHA1(t *testing.T) {
923+
testFetchMutatedBranch(t, "sha1", true)
924+
}
925+
926+
func TestFetchMutatedBranchSHA256(t *testing.T) {
927+
testFetchMutatedBranch(t, "sha256", false)
928+
}
929+
930+
func TestFetchMutatedBranchKeepGitDirSHA256(t *testing.T) {
931+
testFetchMutatedBranch(t, "sha256", true)
932+
}
933+
934+
// testFetchMutatedBranch tests that if a branch is mutated in a way that previous
935+
// ref becomes parent directory of new ref, causing collision to existing checkouts
936+
func testFetchMutatedBranch(t *testing.T, format string, keepGitDir bool) {
937+
ctx := t.Context()
938+
if runtime.GOOS == "windows" {
939+
t.Skip("Depends on unimplemented containerd bind-mount support on Windows")
940+
}
941+
repo := setupGitRepo(t, format)
942+
cmd := exec.Command("git", "rev-parse", "feature")
943+
cmd.Dir = repo.mainPath
944+
945+
out, err := cmd.Output()
946+
require.NoError(t, err)
947+
948+
expLen := 40
949+
if format == "sha256" {
950+
expLen = 64
951+
}
952+
shaBranch := strings.TrimSpace(string(out))
953+
require.Equal(t, expLen, len(shaBranch))
954+
955+
id := &GitIdentifier{Remote: repo.mainURL, Ref: "feature", KeepGitDir: keepGitDir}
956+
gs := setupGitSource(t, t.TempDir())
957+
958+
g, err := gs.Resolve(ctx, id, nil, nil)
959+
require.NoError(t, err)
960+
961+
key1, pin1, _, done, err := g.CacheKey(ctx, nil, 0)
962+
require.NoError(t, err)
963+
require.True(t, done)
964+
965+
ref, err := g.Snapshot(ctx, nil)
966+
require.NoError(t, err)
967+
ref.Release(context.TODO())
968+
969+
// mutate the branch to point to become parent dir
970+
cmd = exec.Command("git", "branch", "-D", "feature")
971+
cmd.Dir = repo.mainPath
972+
out, err = cmd.CombinedOutput()
973+
require.NoError(t, err, string(out))
974+
975+
cmd = exec.Command("git", "branch", "feature/new", shaBranch)
976+
cmd.Dir = repo.mainPath
977+
out, err = cmd.CombinedOutput()
978+
require.NoError(t, err, string(out))
979+
980+
id = &GitIdentifier{Remote: repo.mainURL, Ref: "feature/new", KeepGitDir: keepGitDir}
981+
982+
g, err = gs.Resolve(ctx, id, nil, nil)
983+
require.NoError(t, err)
984+
985+
key2, pin2, _, done, err := g.CacheKey(ctx, nil, 0)
986+
require.NoError(t, err)
987+
require.True(t, done)
988+
989+
if keepGitDir {
990+
require.NotEqual(t, key1, key2) // key contains new ref
991+
} else {
992+
require.Equal(t, key1, key2)
993+
}
994+
require.Equal(t, pin1, pin2)
995+
996+
ref, err = g.Snapshot(ctx, nil)
997+
require.NoError(t, err)
998+
ref.Release(context.TODO())
999+
}
1000+
8341001
func TestMultipleTagAccessKeepGitDirSHA1(t *testing.T) {
8351002
testMultipleTagAccess(t, true, "sha1")
8361003
}

0 commit comments

Comments
 (0)