tools/internal/github: correctly handle github's mergeability updates (…

…#2214)

Sometimes, the API returns PR info with a null "mergeable" bool, which means
"I need a few seconds to update the PR and check for mergeability, please
try again in a few seconds". Previously this was interpreted by psltool as
"this PR has a merge conflict and cannot be checked".

Instead, do as the Github API docs say, and just keep polling until the PR
information is up to date.

tools/internal/github/pr.go

@@ -4,6 +4,7 @@ package github
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"time"
@@ -52,44 +53,109 @@ func (c *Client) apiClient() *github.Client {
 // given pull request. Returns the PSL file for the target branch, and
 // the same but with the PR's changes applied.
 func (c *Client) PSLForPullRequest(ctx context.Context, prNum int) (withoutPR, withPR []byte, err error) {
+	// Github sometimes needs a little time to think to update the PR
+	// state, so we might need to sleep and retry a few times. Usually
+	// the status updates in <5s, but just for safety, give it a more
+	// generous timeout.
+	ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+	defer cancel()
+
+	var withoutHash, withHash string
+	for withoutHash == "" {
+		withoutHash, withHash, err = c.getPRCommitInfo(ctx, prNum)
+		if errors.Is(err, errMergeInfoNotReady) {
+			// PR exists but merge info is stale, need to wait and
+			// retry.
+			select {
+			case <-time.After(2 * time.Second):
+				continue
+			case <-ctx.Done():
+				return nil, nil, ctx.Err()
+			}
+		} else if err != nil {
+			return nil, nil, err
+		}
+	}
+
+	withoutPR, err = c.PSLForHash(ctx, withoutHash)
+	if err != nil {
+		return nil, nil, err
+	}
+	withPR, err = c.PSLForHash(ctx, withHash)
+	if err != nil {
+		return nil, nil, err
+	}
+	return withoutPR, withPR, nil
+}
+
+var errMergeInfoNotReady = errors.New("PR mergeability information not available yet, please retry later")
+
+// getPRCommitInfo returns the "before" and "after" commit hashes for
+// prNum.
+//
+// The exact meaning of "before" and "after" varies, but in general
+// before is the state of the master branch right before the PR is
+// merged, and "after" is the same state plus the PR's changes, with
+// no unrelated changes.
+//
+// For an unmerged PR, "after" is a "trial merge commit" created
+// automatically by Github to run CI and check that the PR is
+// mergeable, and "before" is the master branch state from that trial
+// merge - usually the latest current state.
+//
+// For a merged PR, "after" is the commit where the PR's changes first
+// appeared in master, and "before" is the state of master immediately
+// before that.
+//
+// getPRCommitInfo returns the sentinel error errMergeInfoNotReady if
+// an open PR exists, but github needs a bit more time to update the
+// trial merge commit. The caller is expected to retry with
+// appropriate backoff.
+func (c *Client) getPRCommitInfo(ctx context.Context, prNum int) (withoutPRCommit, withPRCommit string, err error) {
 	ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
 	defer cancel()
 
 	pr, _, err := c.apiClient().PullRequests.Get(ctx, c.owner(), c.repo(), prNum)
 	if err != nil {
-		return nil, nil, err
+		return "", "", err
 	}
 
 	mergeCommit := pr.GetMergeCommitSHA()
 	if mergeCommit == "" {
-		return nil, nil, fmt.Errorf("no merge commit available for PR %d", prNum)
+		return "", "", fmt.Errorf("no merge commit available for PR %d", prNum)
 	}
 	commitInfo, _, err := c.apiClient().Git.GetCommit(ctx, c.owner(), c.repo(), mergeCommit)
 	if err != nil {
-		return nil, nil, fmt.Errorf("getting info for trial merge SHA %q: %w", mergeCommit, err)
+		return "", "", fmt.Errorf("getting info for trial merge SHA %q: %w", mergeCommit, err)
 	}
 
 	var beforeMergeCommit string
 	if pr.GetMerged() && len(commitInfo.Parents) == 1 {
 		// PR was merged, PSL policy is to use squash-and-merge, so
 		// the pre-PR commit is simply the parent of the merge commit.
 		beforeMergeCommit = commitInfo.Parents[0].GetSHA()
+	} else if pr.Mergeable == nil {
+		// PR isn't merged, but github needs time to rebase the PR and
+		// create a trial merge. Unfortunately the only way to know
+		// when it's done is to just poll and wait for the mergeable
+		// bool to be valid.
+		return "", "", errMergeInfoNotReady
 	} else if !pr.GetMergeable() {
 		// PR isn't merged, and there's a merge conflict that prevents
 		// us from knowing what the pre- and post-merge states are.
-		return nil, nil, fmt.Errorf("cannot get PSL for PR %d, needs rebase", prNum)
+		return "", "", fmt.Errorf("cannot get PSL for PR %d, needs rebase to resolve conflicts", prNum)
 	} else {
 		// PR is either open, or it was merged without squashing. In
 		// both cases, mergeCommit has 2 parents: one is the PR head
 		// commit, and the other is the master branch without the PR's
 		// changes.
 		if numParents := len(commitInfo.Parents); numParents != 2 {
-			return nil, nil, fmt.Errorf("unexpected parent count %d for trial merge commit on PR %d, expected 2 parents", numParents, prNum)
+			return "", "", fmt.Errorf("unexpected parent count %d for trial merge commit on PR %d, expected 2 parents", numParents, prNum)
 		}
 
 		prHeadCommit := pr.GetHead().GetSHA()
 		if prHeadCommit == "" {
-			return nil, nil, fmt.Errorf("no commit SHA available for head of PR %d", prNum)
+			return "", "", fmt.Errorf("no commit SHA available for head of PR %d", prNum)
 		}
 		if commitInfo.Parents[0].GetSHA() == prHeadCommit {
 			beforeMergeCommit = commitInfo.Parents[1].GetSHA()
@@ -98,15 +164,7 @@ func (c *Client) PSLForPullRequest(ctx context.Context, prNum int) (withoutPR, w
 		}
 	}
 
-	withoutPR, err = c.PSLForHash(ctx, beforeMergeCommit)
-	if err != nil {
-		return nil, nil, err
-	}
-	withPR, err = c.PSLForHash(ctx, mergeCommit)
-	if err != nil {
-		return nil, nil, err
-	}
-	return withoutPR, withPR, nil
+	return beforeMergeCommit, mergeCommit, nil
 }
 
 // PSLForHash returns the PSL file at the given git commit hash.