From 6addac1b5dca187b3c0be19ec6051235befd925b Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Fri, 1 Mar 2019 02:01:32 -0800 Subject: [PATCH 1/3] [JENKINS-43194] Use merge_commit_sha for master Work-in-progress. --- .../GitHubSCMFileSystem.java | 13 +-- .../github_branch_source/GitHubSCMSource.java | 87 ++++++++++++++----- .../PullRequestSCMRevision.java | 21 ++++- 3 files changed, 90 insertions(+), 31 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystem.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystem.java index f4a1b80c6..2b855d931 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystem.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystem.java @@ -84,8 +84,11 @@ protected GitHubSCMFileSystem(GitHub gitHub, GHRepository repo, String refName, if (rev != null) { if (rev.getHead() instanceof PullRequestSCMHead) { PullRequestSCMHead pr = (PullRequestSCMHead) rev.getHead(); - assert !pr.isMerge(); // TODO see below - this.ref = ((PullRequestSCMRevision) rev).getPullHash(); + if (pr.isMerge()) { + this.ref = ((PullRequestSCMRevision) rev).getMergeHash(); + } else { + this.ref = ((PullRequestSCMRevision) rev).getPullHash(); + } } else if (rev instanceof AbstractGitSCMSource.SCMRevisionImpl) { this.ref = ((AbstractGitSCMSource.SCMRevisionImpl) rev).getHash(); } else { @@ -271,7 +274,7 @@ public SCMFileSystem build(@NonNull SCMSource source, @NonNull SCMHead head, @Ch refName = "tags/" + head.getName(); } else if (head instanceof PullRequestSCMHead) { PullRequestSCMHead pr = (PullRequestSCMHead) head; - if (!pr.isMerge() && pr.getSourceRepo() != null) { + if (pr.getSourceRepo() != null) { GHUser user = github.getUser(pr.getSourceOwner()); if (user == null) { // we need to release here as we are not throwing an exception or transferring @@ -288,12 +291,12 @@ public SCMFileSystem build(@NonNull SCMSource source, @NonNull SCMHead head, @Ch } return new GitHubSCMFileSystem( github, repo, - pr.getSourceBranch(), + null, rev); } // we need to release here as we are not throwing an exception or transferring responsibility to FS Connector.release(github); - return null; // TODO support merge revisions somehow + return null; } else { // we need to release here as we are not throwing an exception or transferring responsibility to FS Connector.release(github); diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java index 636f472cf..efb26745e 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java @@ -957,6 +957,7 @@ public SCMSourceCriteria.Probe create(@NonNull BranchSCMHead head, branchName = "PR-" + number + "-" + strategy.name().toLowerCase(Locale.ENGLISH); } count++; + ensureDetailedGHPullRequest(pr, listener, github, ghRepository); if (request.process(new PullRequestSCMHead( pr, branchName, strategy == ChangeRequestCheckoutStrategy.MERGE ), @@ -981,19 +982,22 @@ public SCMSourceCriteria.Probe create(@NonNull PullRequestSCMHead head, public SCMRevision create(@NonNull PullRequestSCMHead head, @Nullable Void ignored) throws IOException, InterruptedException { + String baseHash = pr.getBase().getSha(); + String mergeHash = null; + switch (strategy) { case MERGE: request.checkApiRateLimit(); - GHRef mergeRef = ghRepository.getRef( - "heads/" + pr.getBase().getRef() - ); - return new PullRequestSCMRevision(head, - mergeRef.getObject().getSha(), - pr.getHead().getSha()); + baseHash = ghRepository.getRef("heads/" + head.getTarget().getName()).getObject().getSha(); + mergeHash = pr.getMergeCommitSha(); + break; default: - return new PullRequestSCMRevision(head, pr.getBase().getSha(), - pr.getHead().getSha()); + break; } + return new PullRequestSCMRevision(head, + baseHash, + pr.getHead().getSha(), + mergeHash); } }, new MergabilityWitness(pr, strategy, listener), @@ -1200,7 +1204,7 @@ protected SCMRevision retrieve(@NonNull String headName, @NonNull TaskListener l int number = Integer.parseInt(prMatcher.group(1)); listener.getLogger().format("Attempting to resolve %s as pull request %d%n", headName, number); try { - GHPullRequest pr = ghRepository.getPullRequest(number); + GHPullRequest pr = getDetailedGHPullRequest(number, listener, github, ghRepository); if (pr != null) { boolean fork = !ghRepository.getOwner().equals(pr.getHead().getUser()); Set strategies; @@ -1253,22 +1257,31 @@ protected SCMRevision retrieve(@NonNull String headName, @NonNull TaskListener l PullRequestSCMHead head = new PullRequestSCMHead( pr, headName, strategy == ChangeRequestCheckoutStrategy.MERGE ); + String baseHash = pr.getBase().getSha(); + String mergeHash = null; + switch (strategy) { case MERGE: Connector.checkApiRateLimit(listener, github); - GHRef mergeRef = ghRepository.getRef( - "heads/" + pr.getBase().getRef() - ); + baseHash = ghRepository.getRef("heads/" + head.getTarget().getName()).getObject().getSha(); + mergeHash = pr.getMergeCommitSha(); + + if (Boolean.FALSE.equals(pr.getMergeable())) { + listener.getLogger().format("Resolved %s as pull request %d: Not mergeable.%n%n", + headName, + number); + return null; + } + listener.getLogger().format( - "Resolved %s as pull request %d at revision %s merged onto %s%n", + "Resolved %s as pull request %d at revision %s merged onto %s as %s%n", headName, number, pr.getHead().getSha(), - pr.getBase().getSha() + pr.getBase().getSha(), + pr.getMergeCommitSha() ); - return new PullRequestSCMRevision(head, - mergeRef.getObject().getSha(), - pr.getHead().getSha()); + break; default: listener.getLogger().format( "Resolved %s as pull request %d at revision %s%n", @@ -1276,9 +1289,12 @@ protected SCMRevision retrieve(@NonNull String headName, @NonNull TaskListener l number, pr.getHead().getSha() ); - return new PullRequestSCMRevision(head, pr.getBase().getSha(), - pr.getHead().getSha()); + break; } + return new PullRequestSCMRevision(head, + baseHash, + pr.getHead().getSha(), + mergeHash); } else { listener.getLogger().format( "Could not resolve %s as pull request %d%n", @@ -1438,21 +1454,26 @@ protected SCMRevision retrieve(SCMHead head, TaskListener listener) throws IOExc Connector.checkApiRateLimit(listener, github); String fullName = repoOwner + "/" + repository; ghRepository = github.getRepository(fullName); + final GHRepository ghRepository = this.ghRepository; repositoryUrl = ghRepository.getHtmlUrl(); if (head instanceof PullRequestSCMHead) { PullRequestSCMHead prhead = (PullRequestSCMHead) head; - int number = prhead.getNumber(); - GHPullRequest pr = ghRepository.getPullRequest(number); - String baseHash; + GHPullRequest pr = getDetailedGHPullRequest(prhead.getNumber(), listener, github, ghRepository); + String baseHash = pr.getBase().getSha(); + String mergeHash = null; switch (prhead.getCheckoutStrategy()) { case MERGE: + assert(pr.getMergeable() != null); + if (Boolean.FALSE.equals(pr.getMergeable())) { + throw new AbortException("Pull request " + prhead.getNumber() + " is not mergeable."); + } baseHash = ghRepository.getRef("heads/" + prhead.getTarget().getName()).getObject().getSha(); + mergeHash = pr.getMergeCommitSha(); break; default: - baseHash = pr.getBase().getSha(); break; } - return new PullRequestSCMRevision(prhead, baseHash, pr.getHead().getSha()); + return new PullRequestSCMRevision(prhead, baseHash, pr.getHead().getSha(), mergeHash); } else if (head instanceof GitHubTagSCMHead) { GitHubTagSCMHead tagHead = (GitHubTagSCMHead) head; GHRef tag = ghRepository.getRef("tags/" + tagHead.getName()); @@ -1475,6 +1496,24 @@ protected SCMRevision retrieve(SCMHead head, TaskListener listener) throws IOExc } } + private GHPullRequest getDetailedGHPullRequest(int number, TaskListener listener, GitHub github, GHRepository ghRepository) throws IOException, InterruptedException { + Connector.checkApiRateLimit(listener, github); + GHPullRequest pr = ghRepository.getPullRequest(number); + ensureDetailedGHPullRequest(pr, listener, github, ghRepository); + return pr; + } + + private void ensureDetailedGHPullRequest(GHPullRequest pr, TaskListener listener, GitHub github, GHRepository ghRepository) throws IOException, InterruptedException { + final long sleep = 1000; + while (pr.getMergeableState() == null) { + listener.getLogger().format( + "Could not determine the mergability of pull request %d. Retrying...%n", + pr.getNumber()); + Thread.sleep(sleep); + Connector.checkApiRateLimit(listener, github); + } + } + @Override public SCM build(SCMHead head, SCMRevision revision) { return new GitHubSCMBuilder(this, head, revision).withTraits(traits).build(); diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevision.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevision.java index f758c82e7..b28aeca03 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevision.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevision.java @@ -24,6 +24,7 @@ package org.jenkinsci.plugins.github_branch_source; +import edu.umd.cs.findbugs.annotations.CheckForNull; import edu.umd.cs.findbugs.annotations.NonNull; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import jenkins.plugins.git.AbstractGitSCMSource; @@ -36,16 +37,22 @@ * Revision of a pull request. */ public class PullRequestSCMRevision extends ChangeRequestSCMRevision { - + private static final long serialVersionUID = 1L; private final @NonNull String baseHash; private final @NonNull String pullHash; + private final @CheckForNull String mergeHash; public PullRequestSCMRevision(@NonNull PullRequestSCMHead head, @NonNull String baseHash, @NonNull String pullHash) { + this(head, baseHash, pullHash, null); + } + + public PullRequestSCMRevision(@NonNull PullRequestSCMHead head, @NonNull String baseHash, @NonNull String pullHash, @CheckForNull String mergeHash) { super(head, new AbstractGitSCMSource.SCMRevisionImpl(head.getTarget(), baseHash)); this.baseHash = baseHash; this.pullHash = pullHash; + this.mergeHash = mergeHash; } @SuppressFBWarnings({"SE_PRIVATE_READ_RESOLVE_NOT_INHERITED", "RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE"}) @@ -81,6 +88,16 @@ public String getPullHash() { return pullHash; } + /** + * The commit hash of the head of the pull request branch. + * + * @return The commit hash of the head of the pull request branch + */ + @CheckForNull + public String getMergeHash() { + return mergeHash; + } + @Override public boolean equivalent(ChangeRequestSCMRevision o) { if (!(o instanceof PullRequestSCMRevision)) { @@ -97,7 +114,7 @@ public int _hashCode() { @Override public String toString() { - return getHead() instanceof PullRequestSCMHead && ((PullRequestSCMHead) getHead()).isMerge() ? pullHash + "+" + baseHash : pullHash; + return getHead() instanceof PullRequestSCMHead && ((PullRequestSCMHead) getHead()).isMerge() ? pullHash + "+" + baseHash + " - " + mergeHash : pullHash; } } From dbd2786b99c6dbd280a08265d8f4b268567ec7fa Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Wed, 6 Mar 2019 16:08:19 -0800 Subject: [PATCH 2/3] Revert minor code cleanup to highlight only merge_commit_sha change --- .../github_branch_source/GitHubSCMSource.java | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java index efb26745e..eecb88f7e 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java @@ -982,22 +982,21 @@ public SCMSourceCriteria.Probe create(@NonNull PullRequestSCMHead head, public SCMRevision create(@NonNull PullRequestSCMHead head, @Nullable Void ignored) throws IOException, InterruptedException { - String baseHash = pr.getBase().getSha(); - String mergeHash = null; - switch (strategy) { case MERGE: request.checkApiRateLimit(); - baseHash = ghRepository.getRef("heads/" + head.getTarget().getName()).getObject().getSha(); - mergeHash = pr.getMergeCommitSha(); - break; + GHRef mergeRef = ghRepository.getRef( + "heads/" + pr.getBase().getRef() + ); + return new PullRequestSCMRevision(head, + mergeRef.getObject().getSha(), + pr.getHead().getSha(), + pr.getMergeCommitSha()); default: - break; + return new PullRequestSCMRevision(head, pr.getBase().getSha(), + pr.getHead().getSha(), + null); } - return new PullRequestSCMRevision(head, - baseHash, - pr.getHead().getSha(), - mergeHash); } }, new MergabilityWitness(pr, strategy, listener), From fe37f64b0caed3ca7d5173c8064cf21a246f2e87 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Mon, 25 Mar 2019 16:47:19 -0700 Subject: [PATCH 3/3] Check for inconsistent merge commit It is possible for the base and source branches to change between when a PR is seen and we look on the the commits and generate the merge commit. This change adds checks that the three commits in the PullRequestSCMRevision are consistent with each other. --- .../GitHubSCMFileSystem.java | 13 +++-- .../github_branch_source/GitHubSCMSource.java | 48 ++++++++++++------- .../PullRequestSCMRevision.java | 6 +-- 3 files changed, 44 insertions(+), 23 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystem.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystem.java index 2b855d931..3d6bd56a3 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystem.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystem.java @@ -28,6 +28,7 @@ import com.cloudbees.plugins.credentials.common.StandardCredentials; import edu.umd.cs.findbugs.annotations.CheckForNull; import edu.umd.cs.findbugs.annotations.NonNull; +import hudson.AbortException; import hudson.Extension; import hudson.model.Item; import hudson.plugins.git.GitSCM; @@ -35,6 +36,7 @@ import java.io.IOException; import java.io.OutputStream; import java.nio.charset.StandardCharsets; +import java.util.List; import java.util.Locale; import java.util.Objects; @@ -83,11 +85,16 @@ protected GitHubSCMFileSystem(GitHub gitHub, GHRepository repo, String refName, this.repo = repo; if (rev != null) { if (rev.getHead() instanceof PullRequestSCMHead) { - PullRequestSCMHead pr = (PullRequestSCMHead) rev.getHead(); + PullRequestSCMRevision prRev = (PullRequestSCMRevision) rev; + PullRequestSCMHead pr = (PullRequestSCMHead) prRev.getHead(); if (pr.isMerge()) { - this.ref = ((PullRequestSCMRevision) rev).getMergeHash(); + this.ref = prRev.getMergeHash(); + List parents = repo.getCommit(this.ref).getParentSHA1s(); + if (parents.size() != 2 || !parents.contains(prRev.getBaseHash()) || !parents.contains(prRev.getPullHash())) { + throw new AbortException("Merge commit does not match base and head commits for pull request " + pr.getNumber() + "."); + } } else { - this.ref = ((PullRequestSCMRevision) rev).getPullHash(); + this.ref = prRev.getPullHash(); } } else if (rev instanceof AbstractGitSCMSource.SCMRevisionImpl) { this.ref = ((AbstractGitSCMSource.SCMRevisionImpl) rev).getHash(); diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java index eecb88f7e..bb78c2b5c 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java @@ -982,21 +982,20 @@ public SCMSourceCriteria.Probe create(@NonNull PullRequestSCMHead head, public SCMRevision create(@NonNull PullRequestSCMHead head, @Nullable Void ignored) throws IOException, InterruptedException { + String prHeadHash = pr.getHead().getSha(); + String baseHash = pr.getBase().getSha(); + String mergeHash = null; + switch (strategy) { case MERGE: request.checkApiRateLimit(); - GHRef mergeRef = ghRepository.getRef( - "heads/" + pr.getBase().getRef() - ); - return new PullRequestSCMRevision(head, - mergeRef.getObject().getSha(), - pr.getHead().getSha(), - pr.getMergeCommitSha()); + baseHash = ghRepository.getRef("heads/" + pr.getBase().getRef()).getObject().getSha(); + mergeHash = pr.getMergeCommitSha(); + break; default: - return new PullRequestSCMRevision(head, pr.getBase().getSha(), - pr.getHead().getSha(), - null); + break; } + return new PullRequestSCMRevision(head, baseHash, prHeadHash, mergeHash); } }, new MergabilityWitness(pr, strategy, listener), @@ -1256,6 +1255,7 @@ protected SCMRevision retrieve(@NonNull String headName, @NonNull TaskListener l PullRequestSCMHead head = new PullRequestSCMHead( pr, headName, strategy == ChangeRequestCheckoutStrategy.MERGE ); + String prHeadHash = pr.getHead().getSha(); String baseHash = pr.getBase().getSha(); String mergeHash = null; @@ -1271,14 +1271,22 @@ protected SCMRevision retrieve(@NonNull String headName, @NonNull TaskListener l number); return null; } + List parents = ghRepository.getCommit(mergeHash).getParentSHA1s(); + if (parents.size() != 2 || !parents.contains(baseHash) || !parents.contains(prHeadHash)) { + listener.getLogger().format("Resolved %s as pull request %d: Merge commit does not match base and head.%n%n", + headName, + number); + return null; + + } listener.getLogger().format( "Resolved %s as pull request %d at revision %s merged onto %s as %s%n", headName, number, - pr.getHead().getSha(), - pr.getBase().getSha(), - pr.getMergeCommitSha() + prHeadHash, + baseHash, + mergeHash ); break; default: @@ -1286,13 +1294,13 @@ protected SCMRevision retrieve(@NonNull String headName, @NonNull TaskListener l "Resolved %s as pull request %d at revision %s%n", headName, number, - pr.getHead().getSha() + prHeadHash ); break; } return new PullRequestSCMRevision(head, baseHash, - pr.getHead().getSha(), + prHeadHash, mergeHash); } else { listener.getLogger().format( @@ -1457,8 +1465,10 @@ protected SCMRevision retrieve(SCMHead head, TaskListener listener) throws IOExc repositoryUrl = ghRepository.getHtmlUrl(); if (head instanceof PullRequestSCMHead) { PullRequestSCMHead prhead = (PullRequestSCMHead) head; + GHPullRequest pr = getDetailedGHPullRequest(prhead.getNumber(), listener, github, ghRepository); String baseHash = pr.getBase().getSha(); + String prHeadHash = pr.getHead().getSha(); String mergeHash = null; switch (prhead.getCheckoutStrategy()) { case MERGE: @@ -1466,13 +1476,17 @@ protected SCMRevision retrieve(SCMHead head, TaskListener listener) throws IOExc if (Boolean.FALSE.equals(pr.getMergeable())) { throw new AbortException("Pull request " + prhead.getNumber() + " is not mergeable."); } - baseHash = ghRepository.getRef("heads/" + prhead.getTarget().getName()).getObject().getSha(); + baseHash = ghRepository.getRef("heads/" + pr.getBase().getRef()).getObject().getSha(); mergeHash = pr.getMergeCommitSha(); + List parents = ghRepository.getCommit(mergeHash).getParentSHA1s(); + if (parents.size() != 2 || !parents.contains(baseHash) || !parents.contains(prHeadHash)) { + throw new AbortException("Merge commit does not match base and head commits for pull request " + prhead.getNumber() + "."); + } break; default: break; } - return new PullRequestSCMRevision(prhead, baseHash, pr.getHead().getSha(), mergeHash); + return new PullRequestSCMRevision(prhead, baseHash, prHeadHash, mergeHash); } else if (head instanceof GitHubTagSCMHead) { GitHubTagSCMHead tagHead = (GitHubTagSCMHead) head; GHRef tag = ghRepository.getRef("tags/" + tagHead.getName()); diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevision.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevision.java index b28aeca03..c9a81f08a 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevision.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevision.java @@ -42,13 +42,13 @@ public class PullRequestSCMRevision extends ChangeRequestSCMRevision