Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

git: ensure that pin matches checked-out commit #4473

Merged
merged 1 commit into from
Dec 13, 2023
Merged

git: ensure that pin matches checked-out commit #4473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 31 additions & 6 deletions source/git/source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,20 +365,45 @@ func (gs *gitSourceHandler) CacheKey(ctx context.Context, g session.Group, index

// TODO: should we assume that remote tag is immutable? add a timer?

buf, err := git.Run(ctx, "ls-remote", "origin", ref)
buf, err := git.Run(ctx, "ls-remote", "origin", ref, ref+"^{}")
if err != nil {
return "", "", nil, false, errors.Wrapf(err, "failed to fetch remote %s", urlutil.RedactCredentials(remote))
}
out := string(buf)
idx := strings.Index(out, "\t")
if idx == -1 {
return "", "", nil, false, errors.Errorf("repository does not contain ref %s, output: %q", ref, string(out))
lines := strings.Split(string(buf), "\n")

var (
partialRef = "refs/" + strings.TrimPrefix(ref, "refs/")
headRef = "refs/heads/" + strings.TrimPrefix(ref, "refs/heads/")
tagRef = "refs/tags/" + strings.TrimPrefix(ref, "refs/tags/")
annotatedTagRef = tagRef + "^{}"
)
var sha, headSha, tagSha string
for _, line := range lines {
lineSha, lineRef, _ := strings.Cut(line, "\t")
switch lineRef {
case headRef:
headSha = lineSha
case tagRef, annotatedTagRef:
tagSha = lineSha
case partialRef:
sha = lineSha
}
}

sha := string(out[:idx])
// git-checkout prefers branches in case of ambiguity
if sha == "" {
sha = headSha
}
if sha == "" {
sha = tagSha
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tagSha can be empty in here as the full ref is also allowed, not only branch/tag name.

Also, if no matches, then error should be better, like in the old code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, my bad.

  • I've restored the previous error message for when the ref can't be found.
  • I've added tests for full refs (so this doesn't ever accidentally break)
    • And then made sure that this patch pass them correctly
  • I've also added tests for checking out branches (similar to how we checkout tags), which we somehow hadn't included before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is still missing the case where lineRef == ref

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, added this case in, and made sure we test for it (yay more tests 🎉)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git ls-remote https://github.com/moby/buildkit.git pull/4473/head should be a valid match as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this case works in the current state? Or are you suggesting there should be a test for that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why it should work. Note that the input/output do not match for this case:

 # git ls-remote https://github.com/moby/buildkit.git  pull/4473/head
ad56d1540d61e9b98911cc9f38696fa31a596b56	refs/pull/4473/head

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed in the latest update I pushed.

}
if sha == "" {
return "", "", nil, false, errors.Errorf("repository does not contain ref %s, output: %q", ref, string(buf))
}
if !isCommitSHA(sha) {
return "", "", nil, false, errors.Errorf("invalid commit sha %q", sha)
}

cacheKey := gs.shaToCacheKey(sha)
gs.cacheKey = cacheKey
return cacheKey, sha, nil, true, nil
Expand Down
45 changes: 41 additions & 4 deletions source/git/source_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,10 @@ func TestFetchByTagKeepGitDir(t *testing.T) {
testFetchByTag(t, "lightweight-tag", "third", false, true, true)
}

func TestFetchByTagFull(t *testing.T) {
testFetchByTag(t, "refs/tags/lightweight-tag", "third", false, true, true)
}

func TestFetchByAnnotatedTag(t *testing.T) {
testFetchByTag(t, "v1.2.3", "second", true, false, false)
}
Expand All @@ -233,6 +237,34 @@ func TestFetchByAnnotatedTagKeepGitDir(t *testing.T) {
testFetchByTag(t, "v1.2.3", "second", true, false, true)
}

func TestFetchByAnnotatedTagFull(t *testing.T) {
testFetchByTag(t, "refs/tags/v1.2.3", "second", true, false, true)
}

func TestFetchByBranch(t *testing.T) {
testFetchByTag(t, "feature", "withsub", false, true, false)
}

func TestFetchByBranchKeepGitDir(t *testing.T) {
testFetchByTag(t, "feature", "withsub", false, true, true)
}

func TestFetchByBranchFull(t *testing.T) {
testFetchByTag(t, "refs/heads/feature", "withsub", false, true, true)
}

func TestFetchByRef(t *testing.T) {
testFetchByTag(t, "test", "feature", false, true, false)
}

func TestFetchByRefKeepGitDir(t *testing.T) {
testFetchByTag(t, "test", "feature", false, true, true)
}

func TestFetchByRefFull(t *testing.T) {
testFetchByTag(t, "refs/test", "feature", false, true, true)
}

func testFetchByTag(t *testing.T, tag, expectedCommitSubject string, isAnnotatedTag, hasFoo13File, keepGitDir bool) {
if runtime.GOOS == "windows" {
t.Skip("Depends on unimplemented containerd bind-mount support on Windows")
Expand Down Expand Up @@ -296,15 +328,18 @@ func testFetchByTag(t *testing.T, tag, expectedCommitSubject string, isAnnotated
gitutil.WithWorkTree(dir),
)

// get current commit sha
headCommit, err := git.Run(ctx, "rev-parse", "HEAD")
require.NoError(t, err)

// ensure that we checked out the same commit as was in the cache key
require.Equal(t, strings.TrimSpace(string(headCommit)), pin1)

if isAnnotatedTag {
// get commit sha that the annotated tag points to
annotatedTagCommit, err := git.Run(ctx, "rev-list", "-n", "1", tag)
require.NoError(t, err)

// get current commit sha
headCommit, err := git.Run(ctx, "rev-parse", "HEAD")
require.NoError(t, err)

// HEAD should match the actual commit sha (and not the sha of the annotated tag,
// since it's not possible to checkout a non-commit object)
require.Equal(t, string(annotatedTagCommit), string(headCommit))
Expand Down Expand Up @@ -582,6 +617,7 @@ func setupGitRepo(t *testing.T) gitRepoFixture {
"echo foo > abc",
"git add abc",
"git commit -m initial",
"git tag --no-sign a/v1.2.3",
"echo bar > def",
"git add def",
"git commit -m second",
Expand All @@ -594,6 +630,7 @@ func setupGitRepo(t *testing.T) gitRepoFixture {
"echo baz > ghi",
"git add ghi",
"git commit -m feature",
"git update-ref refs/test $(git rev-parse HEAD)",
"git submodule add "+fixture.subURL+" sub",
"git add -A",
"git commit -m withsub",
Expand Down