From 6a8d2ca2bd965c35c5753f698daeb6805aa05db4 Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Thu, 7 Dec 2023 16:29:07 +0000 Subject: [PATCH] git: ensure that pin matches checked-out commit Previously, it was very possible for the CacheKey function to return a sha key that was *not* the checked out commit. There are two cases that I've encountered where this can happen: - An annotated tag will have the pin of the tag, and not the underlying commit, which will be HEAD after the checkout. - If multiple tags have the same path component (e.g. "mytag" and "abc/mytag") then the first alphabetical tag will be selected when (in this case "abc/mytag"). To avoid this kind of case, we can't just search for a single match in the results for ls-remote. There's no way to filter for just an exact match, so we need to scan through the output ourselves. Additionally, we need to dereference the annotated tags by also selecting refs ending in "^{}" - which have the commit that the tag points at. Finally, I've improved the test suite around this to check that: - The cache-key pin is equivalent to the checked out commit - We can check out non-master branches - That full ref syntax like "refs/heads/" and "refs/tags/" (or even "refs/") can be used. Signed-off-by: Justin Chadwell --- source/git/source.go | 37 ++++++++++++++++++++++++++------ source/git/source_test.go | 45 +++++++++++++++++++++++++++++++++++---- 2 files changed, 72 insertions(+), 10 deletions(-) diff --git a/source/git/source.go b/source/git/source.go index 727d275413e8..987ea0e80e0c 100644 --- a/source/git/source.go +++ b/source/git/source.go @@ -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 + } + 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 diff --git a/source/git/source_test.go b/source/git/source_test.go index 6712340a25d5..3735bb30e162 100644 --- a/source/git/source_test.go +++ b/source/git/source_test.go @@ -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) } @@ -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") @@ -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)) @@ -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", @@ -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",