From fa09f6d2ba19afe2eedbd1fc1e70400fcf6ec7b8 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Tue, 31 Oct 2023 16:28:28 +1100 Subject: [PATCH 1/4] Fetch all heads and tags when a fetch fails with "fatal: couldn't find remote ref" --- internal/job/checkout.go | 2 ++ internal/job/git.go | 14 +++++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/internal/job/checkout.go b/internal/job/checkout.go index e41b6f1b79..40fdadd6e1 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -532,6 +532,8 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) error { switch gerr.Type { case gitErrorFetchRetryClean, gitErrorFetchBadObject: return fmt.Errorf("fetching commit %q: %w", e.Commit, err) + case gitErrorFetchBadReference: + // fallback to fetching all heads and tags } } diff --git a/internal/job/git.go b/internal/job/git.go index 85169502b2..3f2e1b9676 100644 --- a/internal/job/git.go +++ b/internal/job/git.go @@ -25,6 +25,7 @@ const ( gitErrorFetch gitErrorFetchRetryClean gitErrorFetchBadObject + gitErrorFetchBadReference gitErrorClean gitErrorCleanSubmodules ) @@ -158,7 +159,13 @@ func gitFetch( } const badObject = "fatal: bad object" - if o, err := sh.RunWithOlfactor(ctx, []string{badObject}, "git", commandArgs...); err != nil { + const badReference = "fatal: couldn't find remote ref" + if o, err := sh.RunWithOlfactor( + ctx, + []string{badObject, badReference}, + "git", + commandArgs..., + ); err != nil { // "fatal: bad object" can happen when the local repo in the checkout // directory is corrupted, not just the remote or the mirror. // When using git mirrors, the existing checkout directory might have a @@ -169,6 +176,11 @@ func gitFetch( return &gitError{error: err, Type: gitErrorFetchBadObject} } + // "fatal: couldn't find remote ref" can happen when the just the short commit hash is given. + if o.Smelt(badReference) { + return &gitError{error: err, Type: gitErrorFetchBadReference} + } + // 128 is extremely broad, but it seems permissions errors, network unreachable errors etc, // don't result in it if exitErr := new(exec.ExitError); errors.As(err, &exitErr) && exitErr.ExitCode() == 128 { From 487299cf2d63d3dc2fc9276048462776949de104 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Wed, 1 Nov 2023 00:07:35 +1100 Subject: [PATCH 2/4] Add test for checkout of short commit hash --- .../integration/checkout_integration_test.go | 49 +++++++++++++++++++ internal/job/integration/git.go | 8 +-- 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/internal/job/integration/checkout_integration_test.go b/internal/job/integration/checkout_integration_test.go index cd92469afe..677bc8afef 100644 --- a/internal/job/integration/checkout_integration_test.go +++ b/internal/job/integration/checkout_integration_test.go @@ -14,6 +14,7 @@ import ( "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/bintest/v3" + "gotest.tools/v3/assert" ) // Example commit info: @@ -274,6 +275,54 @@ func TestCheckingOutShallowCloneOfLocalGitProject(t *testing.T) { tester.RunAndCheck(t, env...) } +func TestCheckingOutLocalGitProjectWithShortCommitHash(t *testing.T) { + t.Parallel() + + tester, err := NewBootstrapTester(mainCtx) + assert.NilError(t, err) + defer tester.Close() + + // Do one checkout + tester.RunAndCheck(t) + + // Create another commit on the same branch in the remote repo + err = tester.Repo.ExecuteAll([][]string{ + {"commit", "--allow-empty", "-m", "Another commit"}, + }) + assert.NilError(t, err) + + commitHash, err := tester.Repo.RevParse("HEAD") + assert.NilError(t, err) + shortCommitHash := commitHash[:7] + + git := tester. + MustMock(t, "git"). + PassthroughToLocalCommand() + + // Git should attempt to fetch the shortHash, but fail. Then fallback to fetching + // all the heads and tags and checking out the short hash. + git.ExpectAll([][]any{ + {"remote", "get-url", "origin"}, + {"clean", "-ffxdq"}, + {"fetch", "--", "origin", shortCommitHash}, + {"config", "remote.origin.fetch"}, + {"fetch", "--", "origin", "+refs/heads/*:refs/remotes/origin/*", "+refs/tags/*:refs/tags/*"}, + {"checkout", "-f", shortCommitHash}, + {"clean", "-ffxdq"}, + {"--no-pager", "log", "-1", "HEAD", "-s", "--no-color", gitShowFormatArg}, + }) + + // Mock out the meta-data calls to the agent after checkout + agent := tester.MockAgent(t) + agent.Expect("meta-data", "exists", "buildkite:git:commit").AndExitWith(1) + agent.Expect("meta-data", "set", "buildkite:git:commit").WithStdin(commitPattern) + + env := []string{ + fmt.Sprintf("BUILDKITE_COMMIT=%s", shortCommitHash), + } + tester.RunAndCheck(t, env...) +} + func TestCheckoutErrorIsRetried(t *testing.T) { t.Parallel() diff --git a/internal/job/integration/git.go b/internal/job/integration/git.go index f687439faf..f68d5f4113 100644 --- a/internal/job/integration/git.go +++ b/internal/job/integration/git.go @@ -9,6 +9,10 @@ import ( "strings" ) +type gitRepository struct { + Path string +} + func createTestGitRespository() (*gitRepository, error) { repo, err := newGitRepository() if err != nil { @@ -66,10 +70,6 @@ func createTestGitRespository() (*gitRepository, error) { return repo, nil } -type gitRepository struct { - Path string -} - func newGitRepository() (*gitRepository, error) { tempDirRaw, err := os.MkdirTemp("", "git-repo") if err != nil { From 9c305cbe495795c5b44b55b1927c53058f389188 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Wed, 1 Nov 2023 00:23:36 +1100 Subject: [PATCH 3/4] Assert the checkout directory is at the right commit --- internal/job/integration/checkout_integration_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/job/integration/checkout_integration_test.go b/internal/job/integration/checkout_integration_test.go index 677bc8afef..e7bdb09b1f 100644 --- a/internal/job/integration/checkout_integration_test.go +++ b/internal/job/integration/checkout_integration_test.go @@ -321,6 +321,13 @@ func TestCheckingOutLocalGitProjectWithShortCommitHash(t *testing.T) { fmt.Sprintf("BUILDKITE_COMMIT=%s", shortCommitHash), } tester.RunAndCheck(t, env...) + + // Check state of the build directory + checkoutRepo := &gitRepository{Path: tester.CheckoutDir()} + checkoutRepoCommit, err := checkoutRepo.RevParse("HEAD") + assert.NilError(t, err) + + assert.Equal(t, checkoutRepoCommit, commitHash) } func TestCheckoutErrorIsRetried(t *testing.T) { From fa3257ede69c70230b2fc389c548e59af51383b2 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Wed, 1 Nov 2023 21:27:13 +1100 Subject: [PATCH 4/4] Clarify that the checkout directory is what's being checked in a test --- internal/job/integration/checkout_integration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/job/integration/checkout_integration_test.go b/internal/job/integration/checkout_integration_test.go index e7bdb09b1f..811869cef8 100644 --- a/internal/job/integration/checkout_integration_test.go +++ b/internal/job/integration/checkout_integration_test.go @@ -322,7 +322,7 @@ func TestCheckingOutLocalGitProjectWithShortCommitHash(t *testing.T) { } tester.RunAndCheck(t, env...) - // Check state of the build directory + // Check state of the checkout directory checkoutRepo := &gitRepository{Path: tester.CheckoutDir()} checkoutRepoCommit, err := checkoutRepo.RevParse("HEAD") assert.NilError(t, err)