From 80d7d6cec06fbc03b077f0c63db01fcf64f9db94 Mon Sep 17 00:00:00 2001
From: Fabian Holler <fabian.holler@simplesurance.de>
Date: Tue, 7 Sep 2021 17:35:20 +0200
Subject: [PATCH] github: determine if a PR uptodate via compare API endpoint

The implementation to determine if a PR was uptodate with it's base branch was
not reliable.
In the scenario documented in the commit message of commit 2d9ff1a
PRIsUptodate() returned true despite the PR branch was not uptodate.

Use the compare github API rest endpoint to determine if a PR is uptodate.
When comparing the base branch with the PR branch it returns the number of
commit the pr branch is behind the base branch.
If it is more =>1, the PR is not uptodate.
---
 internal/githubclt/client.go | 48 +++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/internal/githubclt/client.go b/internal/githubclt/client.go
index b28bfedd..572d9809 100644
--- a/internal/githubclt/client.go
+++ b/internal/githubclt/client.go
@@ -57,21 +57,24 @@ type Client struct {
 	logger *zap.Logger
 }
 
-func (clt *Client) BranchHeadCommitSHA(ctx context.Context, owner, repo string, branch string) (string, error) {
-	resp, _, err := clt.clt.Repositories.ListCommits(ctx, owner, repo, &github.CommitsListOptions{
-		SHA:         branch,
-		ListOptions: github.ListOptions{Page: 1, PerPage: 1},
-	})
-
+// BranchIsBehindBase returns true if branch is based on the most recent commit of baseBranch.
+// If it is based on older commit, false is returned.
+func (clt *Client) BranchIsBehindBase(ctx context.Context, owner, repo, baseBranch, branch string) (behind bool, branchHEADSHA string, err error) {
+	cmp, _, err := clt.clt.Repositories.CompareCommits(ctx, owner, repo, baseBranch, branch, &github.ListOptions{PerPage: 1})
 	if err != nil {
-		return "", clt.wrapRetryableErrors(err)
+		return false, "", clt.wrapRetryableErrors(err)
+	}
+
+	if len(cmp.Commits) == 0 {
+		return false, "", errors.New("compare response contains 0 commits")
 	}
 
-	if len(resp) != 1 {
-		return "", fmt.Errorf("expected 1 response, got %d", len(resp))
+	branchHEADSHA = cmp.Commits[len(cmp.Commits)-1].GetSHA()
+	if branchHEADSHA == "" {
+		return false, "", errors.New("sha field of last commit is empty")
 	}
 
-	return resp[0].GetSHA(), nil
+	return cmp.GetBehindBy() > 0, branchHEADSHA, nil
 }
 
 const (
@@ -110,7 +113,6 @@ func (clt *Client) CombinedStatus(ctx context.Context, owner, repo, ref string)
 // Additionally it returns the SHA of the head commit for which the status was
 // checked.
 // If the PR is closed PullRequestIsClosedError is returned.
-// Known Bugs: https://github.com/simplesurance/goordinator/issues/28
 func (clt *Client) PRIsUptodate(ctx context.Context, owner, repo string, pullRequestNumber int) (isUptodate bool, headSHA string, err error) {
 	pr, _, err := clt.clt.PullRequests.Get(ctx, owner, repo, pullRequestNumber)
 	if err != nil {
@@ -126,24 +128,36 @@ func (clt *Client) PRIsUptodate(ctx context.Context, owner, repo string, pullReq
 		return false, "", errors.New("got pull request object with empty head")
 	}
 
+	prHeadSHA := prHead.GetSHA()
+	if prHeadSHA == "" {
+		return false, "", errors.New("got pull request object with empty head sha")
+	}
+
 	if pr.GetMergeableState() == "behind" {
-		return false, prHead.GetSHA(), nil
+		return false, prHeadSHA, nil
+	}
+
+	prBranch := prHead.GetRef()
+	if prBranch == "" {
+		return false, "", errors.New("got pull request object with empty ref field")
 	}
 
 	base := pr.GetBase()
 	if base == nil {
-		return false, "", errors.New("got pull request object with empty base")
+		return false, "", errors.New("got pull request object with empty base field")
 	}
 
 	baseBranch := base.GetRef()
-	PRbaseSHA := base.GetSHA()
+	if baseBranch == "" {
+		return false, "", errors.New("got pull request object with empty base ref field")
+	}
 
-	baseBranchHEADSHA, err := clt.BranchHeadCommitSHA(ctx, owner, repo, baseBranch)
+	isBehind, branchHEADSHA, err := clt.BranchIsBehindBase(ctx, owner, repo, baseBranch, prBranch)
 	if err != nil {
-		return false, "", fmt.Errorf("could not retrieve head SHA of base branch %q: %w", baseBranch, err)
+		return false, branchHEADSHA, fmt.Errorf("evaluating if branch is behind base failed: %w", err)
 	}
 
-	return PRbaseSHA == baseBranchHEADSHA, prHead.GetSHA(), nil
+	return !isBehind, branchHEADSHA, nil
 }
 
 // CreateIssueComment creates a comment in a issue or pull request