Skip to content

Commit

Permalink
git: ensure that pin matches checked-out commit
Browse files Browse the repository at this point in the history
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/<branch-name>" and
  "refs/tags/<tag-name>" can be used.

Signed-off-by: Justin Chadwell <me@jedevc.com>
  • Loading branch information
jedevc committed Dec 11, 2023
1 parent 6997850 commit ca23770
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 10 deletions.
30 changes: 24 additions & 6 deletions source/git/source.go
Original file line number Diff line number Diff line change
Expand Up @@ -365,20 +365,38 @@ 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))
var (
headRef = "refs/heads/" + strings.TrimPrefix(ref, "refs/heads/")
tagRef = "refs/tags/" + strings.TrimPrefix(ref, "refs/tags/")
annotatedTagRef = tagRef + "^{}"
)
var headSha, tagSha string
for _, line := range strings.Split(string(buf), "\n") {
lineSha, lineRef, _ := strings.Cut(line, "\t")
switch lineRef {
case headRef:
headSha = lineSha
case tagRef, annotatedTagRef:
tagSha = lineSha
}
}

sha := string(out[:idx])
// git-checkout prefers branches in case of ambiguity
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
Expand Down
32 changes: 28 additions & 4 deletions source/git/source_test.go
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,22 @@ 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 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 +316,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 +605,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 Down

0 comments on commit ca23770

Please sign in to comment.