Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 10 additions & 2 deletions internal/job/checkout_fetch.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ func (e *Executor) fetchSource(ctx context.Context, addBloblessFilter bool, atte
Retry: kind == refspecGithubPRHead,
RefSpecs: refspecs,
}); err != nil {
return fmt.Errorf("fetching PR refspec %q: %w", refspecs, err)
return fmt.Errorf("fetching PR refspec %q: %w%s", refspecs, err, prMergeRefspecHint(kind == refspecGithubPRMerge))
}
if kind == refspecGithubPRMerge && e.PullRequestHeadCommit != "" {
if err := e.validateGithubPRMergeHead(ctx); err != nil {
Expand All @@ -172,7 +172,7 @@ func (e *Executor) fetchSource(ctx context.Context, addBloblessFilter bool, atte
refspecs = append(refspecs, e.Commit)
// We aim to eliminate network round-trip as much as possible so we use a single git fetch here.
if err := gitFetchWithFallback(ctx, e.shell, gitFetchFlags, refspecs...); err != nil {
return fmt.Errorf("fetching PR refspec %q: %w", refspecs, err)
return fmt.Errorf("fetching PR refspec %q: %w%s", refspecs, err, prMergeRefspecHint(kind == refspecGithubPRMerge))
}
}

Expand Down Expand Up @@ -257,6 +257,14 @@ func commitSecondParent(commit string) (string, bool) {
return "", false
}

// prMergeRefspecHint returns a clear suffix for refs/pull/N/merge fetch failures.
func prMergeRefspecHint(isMergeRefspec bool) string {
if !isMergeRefspec {
return ""
}
return "\nThis is possibly due to a merge conflict, or GitHub being unable to create the merge ref automatically"
Comment thread
ss1909 marked this conversation as resolved.
}

func isExistingCheckoutRemoteMirrorAttempt(attempt *remoteMirrorAttempt) bool {
return attempt != nil &&
attempt.site == remoteMirrorSiteExistingCheckout &&
Expand Down
12 changes: 8 additions & 4 deletions internal/job/checkout_mirror.go
Original file line number Diff line number Diff line change
Expand Up @@ -323,23 +323,27 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string, attem

if isMainRepository && !commitAlreadyPresent && !remoteMirrorHit {
var refspecs []string
var retry bool
var retry, isMergeRefspec bool

switch {
case e.RefSpec != "":
// If a custom refspec is provided, use it instead of the branch
e.shell.Commentf("Fetching and mirroring custom refspec %s", e.RefSpec)
refspecs = []string{e.RefSpec}
case e.PullRequest != "false" && strings.Contains(e.PipelineProvider, "github"):
e.shell.Commentf("Fetching and mirroring pull request head from GitHub. This will be retried if it fails, as the pull request head might not be available yet — GitHub creates them asynchronously")
var refspec string
if e.PullRequestUsingMergeRefspec {
// As in fetchSource: a missing merge ref usually means a real
// merge conflict, so fail fast rather than retrying for ~2m.
e.shell.Commentf("Fetching and mirroring pull request merge commit from GitHub")
refspec = fmt.Sprintf("refs/pull/%s/merge", e.PullRequest)
isMergeRefspec = true
} else {
e.shell.Commentf("Fetching and mirroring pull request head from GitHub. This will be retried if it fails, as the pull request head might not be available yet — GitHub creates them asynchronously")
refspec = fmt.Sprintf("refs/pull/%s/head", e.PullRequest)
retry = true
}
refspecs = []string{refspec}
retry = true
default:
// Fetch the build branch from the upstream repository into the mirror.
refspecs = []string{e.Branch}
Expand All @@ -355,7 +359,7 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string, attem
Retry: retry,
})
}); err != nil {
return "", err
return "", fmt.Errorf("%w%s", err, prMergeRefspecHint(isMergeRefspec))
}
}
if !isMainRepository {
Expand Down
30 changes: 30 additions & 0 deletions internal/job/checkout_mirror_remote_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,36 @@ func TestUpdateGitMirrorPullRequestHeadMissFallsBackToCanonical(t *testing.T) {
}
}

// TestUpdateGitMirrorMergeRefspecMissingHintsMergeConflict checks that a
// missing refs/pull/N/merge ref hits the same clarifying hint on the
// --git-mirrors-path update fetch as it does on the canonical checkout fetch.
func TestUpdateGitMirrorMergeRefspecMissingHintsMergeConflict(t *testing.T) {
canonical := newOnHostMirrorHTTPRepo(t, "canonical")
if _, _, err := canonical.PushBranch("canonical", "feature-branch"); err != nil {
t.Fatal(err)
}

// A commit that doesn't exist anywhere, standing in for the speculative
// merge commit GitHub never created because of a conflict.
const missingMergeCommit = "0000000000000000000000000000000000000f"
e := newOnHostMirrorExecutor(t, canonical.RepoURL("canonical"), missingMergeCommit)
cloneOnHostMirrorToPath(t, e.Repository, expectedOnHostMirrorDir(e))
e.PullRequest = "999"
e.PipelineProvider = "github"
e.PullRequestUsingMergeRefspec = true

// refs/pull/999/merge is never created, so the mirror update fetch fails.
_, err := e.updateGitMirror(t.Context(), e.Repository, nil)
if err == nil {
t.Fatal("updateGitMirror() error = nil, want non-nil (missing merge ref)")
}

const wantHint = "This is possibly due to a merge conflict, or GitHub being unable to create the merge ref automatically"
if !strings.Contains(err.Error(), wantHint) {
t.Fatalf("updateGitMirror() error = %q, want it to contain %q", err.Error(), wantHint)
}
}

func TestGetOrUpdateMirrorDirCloneLockTimeoutFallsBackWithoutMirror(t *testing.T) {
canonical := newOnHostMirrorHTTPRepo(t, "canonical")
commit, _, err := canonical.PushBranch("canonical", "feature-branch")
Expand Down
5 changes: 5 additions & 0 deletions internal/job/checkout_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,11 @@ func TestDefaultCheckoutPhase_MergeRefspecFailsFast(t *testing.T) {
if elapsed >= maxDuration {
t.Fatalf("executor.defaultCheckoutPhase(ctx, 1) took %s, want < %s — merge refspec should not be retried", elapsed, maxDuration)
}

const wantHint = "This is possibly due to a merge conflict, or GitHub being unable to create the merge ref automatically"
if !strings.Contains(err.Error(), wantHint) {
t.Fatalf("executor.defaultCheckoutPhase(ctx, 1) error = %q, want it to contain %q", err.Error(), wantHint)
}
}

func TestSkipCheckout(t *testing.T) {
Expand Down