Skip to content

Commit e6d4cda

Browse files
bitwisemanbitwiseman
committed
Make merge commit handling more robust
1 parent 5b2434f commit e6d4cda

File tree

6 files changed

+217
-104
lines changed

6 files changed

+217
-104
lines changed

‎src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystem.java‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@
3333
import hudson.model.Item;
3434
import hudson.plugins.git.GitSCM;
3535
import hudson.scm.SCM;
36+
37+
import java.io.FileNotFoundException;
3638
import java.io.IOException;
3739
import java.io.OutputStream;
3840
import java.nio.charset.StandardCharsets;
@@ -88,7 +90,6 @@ protected GitHubSCMFileSystem(GitHub gitHub, GHRepository repo, String refName,
8890
PullRequestSCMRevision prRev = (PullRequestSCMRevision) rev;
8991
PullRequestSCMHead pr = (PullRequestSCMHead) prRev.getHead();
9092
if (pr.isMerge()) {
91-
prRev.validateMergeHash(repo);
9293
this.ref = prRev.getMergeHash();
9394
} else {
9495
this.ref = prRev.getPullHash();
@@ -279,7 +280,17 @@ public SCMFileSystem build(@NonNull SCMSource source, @NonNull SCMHead head, @Ch
279280
refName = "tags/" + head.getName();
280281
} else if (head instanceof PullRequestSCMHead) {
281282
refName = null;
282-
if (rev == null) {
283+
if (rev != null && rev instanceof PullRequestSCMRevision) {
284+
PullRequestSCMRevision prRev = (PullRequestSCMRevision) rev;
285+
if (((PullRequestSCMHead)head).isMerge()) {
286+
if (prRev.getMergeHash() == null) {
287+
// we need to release here as we are not throwing an exception or transferring responsibility to FS
288+
Connector.release(github);
289+
return null;
290+
}
291+
prRev.validateMergeHash();
292+
}
293+
} else {
283294
// we need to release here as we are not throwing an exception or transferring responsibility to FS
284295
Connector.release(github);
285296
return null;

‎src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java‎

Lines changed: 65 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1081,14 +1081,19 @@ private static void retrievePullRequest(
10811081
}
10821082
return;
10831083
}
1084-
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
10851084
for (final ChangeRequestCheckoutStrategy strategy : strategies.get(fork)) {
10861085
final String branchName;
10871086
if (strategies.get(fork).size() == 1) {
10881087
branchName = "PR-" + number;
10891088
} else {
10901089
branchName = "PR-" + number + "-" + strategy.name().toLowerCase(Locale.ENGLISH);
10911090
}
1091+
1092+
// PR details only needed for merge PRs
1093+
if (strategy == ChangeRequestCheckoutStrategy.MERGE) {
1094+
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
1095+
}
1096+
10921097
if (request.process(new PullRequestSCMHead(
10931098
pr, branchName, strategy == ChangeRequestCheckoutStrategy.MERGE
10941099
),
@@ -1251,7 +1256,8 @@ protected SCMRevision retrieve(@NonNull String headName, @NonNull TaskListener l
12511256
int number = Integer.parseInt(prMatcher.group(1));
12521257
listener.getLogger().format("Attempting to resolve %s as pull request %d%n", headName, number);
12531258
try {
1254-
GHPullRequest pr = getDetailedGHPullRequest(number, listener, github, ghRepository);
1259+
Connector.checkApiRateLimit(listener, github);
1260+
GHPullRequest pr = ghRepository.getPullRequest(number);
12551261
if (pr != null) {
12561262
boolean fork = !ghRepository.getOwner().equals(pr.getHead().getUser());
12571263
Set<ChangeRequestCheckoutStrategy> strategies;
@@ -1304,12 +1310,15 @@ protected SCMRevision retrieve(@NonNull String headName, @NonNull TaskListener l
13041310
PullRequestSCMHead head = new PullRequestSCMHead(
13051311
pr, headName, strategy == ChangeRequestCheckoutStrategy.MERGE
13061312
);
1313+
if (head.isMerge()) {
1314+
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
1315+
}
13071316
PullRequestSCMRevision prRev = createPullRequestSCMRevision(pr, head, listener, github, ghRepository);
13081317

13091318
switch (strategy) {
13101319
case MERGE:
13111320
try {
1312-
prRev.validateMergeHash(ghRepository);
1321+
prRev.validateMergeHash();
13131322
} catch (AbortException e) {
13141323
listener.getLogger().format("Resolved %s as pull request %d: %s.%n%n",
13151324
headName,
@@ -1499,12 +1508,13 @@ protected SCMRevision retrieve(SCMHead head, TaskListener listener) throws IOExc
14991508
repositoryUrl = ghRepository.getHtmlUrl();
15001509
if (head instanceof PullRequestSCMHead) {
15011510
PullRequestSCMHead prhead = (PullRequestSCMHead) head;
1502-
1503-
GHPullRequest pr = getDetailedGHPullRequest(prhead.getNumber(), listener, github, ghRepository);
1504-
PullRequestSCMRevision prRev = createPullRequestSCMRevision(pr, prhead, listener, github, ghRepository);
1511+
Connector.checkApiRateLimit(listener, github);
1512+
GHPullRequest pr = ghRepository.getPullRequest(prhead.getNumber());
15051513
if (prhead.isMerge()) {
1506-
prRev.validateMergeHash(ghRepository);
1514+
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
15071515
}
1516+
PullRequestSCMRevision prRev = createPullRequestSCMRevision(pr, prhead, listener, github, ghRepository);
1517+
prRev.validateMergeHash();
15081518
return prRev;
15091519
} else if (head instanceof GitHubTagSCMHead) {
15101520
GitHubTagSCMHead tagHead = (GitHubTagSCMHead) head;
@@ -1532,26 +1542,57 @@ private static PullRequestSCMRevision createPullRequestSCMRevision(GHPullRequest
15321542
String baseHash = pr.getBase().getSha();
15331543
String prHeadHash = pr.getHead().getSha();
15341544
String mergeHash = null;
1535-
switch (prhead.getCheckoutStrategy()) {
1536-
case MERGE:
1537-
Connector.checkApiRateLimit(listener, github);
1538-
baseHash = ghRepository.getRef("heads/" + pr.getBase().getRef()).getObject().getSha();
1539-
if (pr.getMergeable() != null) {
1540-
mergeHash = Boolean.TRUE.equals(pr.getMergeable()) ? pr.getMergeCommitSha() : PullRequestSCMRevision.NOT_MERGEABLE_HASH;
1545+
1546+
if (prhead.isMerge()) {
1547+
if (Boolean.FALSE.equals(pr.getMergeable())) {
1548+
mergeHash = PullRequestSCMRevision.NOT_MERGEABLE_HASH;
1549+
} else if (Boolean.TRUE.equals(pr.getMergeable())) {
1550+
String proposedMergeHash = pr.getMergeCommitSha();
1551+
GHCommit commit = null;
1552+
try {
1553+
commit = ghRepository.getCommit(proposedMergeHash);
1554+
} catch (FileNotFoundException e) {
1555+
listener.getLogger().format("Pull request %s : github merge_commit_sha not found (%s). Close and reopen the PR to reset its merge hash.%n",
1556+
pr.getNumber(),
1557+
proposedMergeHash);
1558+
} catch (IOException e) {
1559+
throw new AbortException("Error while retrieving pull request " + pr.getNumber() + " merge hash : " + e.toString());
15411560
}
1542-
break;
1543-
default:
1544-
break;
1545-
}
1546-
return new PullRequestSCMRevision(prhead, baseHash, prHeadHash, mergeHash);
1547-
}
15481561

1562+
if (commit != null) {
1563+
List<String> parents = commit.getParentSHA1s();
1564+
// Merge commits always merge against the most recent base commit they can detect.
1565+
if (parents.size() != 2) {
1566+
listener.getLogger().format("WARNING: Invalid github merge_commit_sha for pull request %s : merge commit %s with parents - %s.%n",
1567+
pr.getNumber(),
1568+
proposedMergeHash,
1569+
StringUtils.join(parents, "+"));
1570+
} else if (!parents.contains(prHeadHash)) {
1571+
// This is maintains the existing behavior from pre-2.5.x when the merge_commit_sha is out of sync from the requested prHead
1572+
listener.getLogger().format("WARNING: Invalid github merge_commit_sha for pull request %s : Head commit %s does match merge commit %s with parents - %s.%n",
1573+
pr.getNumber(),
1574+
prHeadHash,
1575+
proposedMergeHash,
1576+
StringUtils.join(parents, "+"));
1577+
} else {
1578+
// We found a merge_commit_sha with 2 parents and one matches the prHeadHash
1579+
// Use the other parent hash as the base. This keeps the merge hash in sync with head and base.
1580+
// It is possible that head or base hash will not exist in their branch by the time we build
1581+
// This is be true (and cause a failure) regardless of how we determine the commits.
1582+
mergeHash = proposedMergeHash;
1583+
baseHash = prHeadHash.equals(parents.get(0)) ? parents.get(1) : parents.get(0);
1584+
}
1585+
}
1586+
}
1587+
1588+
// Merge PR jobs always merge against the most recent base branch commit they can detect.
1589+
// For an invalid merge_commit_sha, we need to query for most recent base commit separately
1590+
if (mergeHash == null) {
1591+
baseHash = ghRepository.getRef("heads/" + pr.getBase().getRef()).getObject().getSha();
1592+
}
1593+
}
15491594

1550-
private static GHPullRequest getDetailedGHPullRequest(int number, TaskListener listener, GitHub github, GHRepository ghRepository) throws IOException, InterruptedException {
1551-
Connector.checkApiRateLimit(listener, github);
1552-
GHPullRequest pr = ghRepository.getPullRequest(number);
1553-
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
1554-
return pr;
1595+
return new PullRequestSCMRevision(prhead, baseHash, prHeadHash, mergeHash);
15551596
}
15561597

15571598
private static void ensureDetailedGHPullRequest(GHPullRequest pr, TaskListener listener, GitHub github, GHRepository ghRepository) throws IOException, InterruptedException {

‎src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevision.java‎

Lines changed: 3 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -106,27 +106,9 @@ public String getMergeHash() {
106106
return mergeHash;
107107
}
108108

109-
void validateMergeHash(@NonNull GHRepository repo) throws IOException {
110-
if (!isMerge()) {
111-
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Not a merge head");
112-
} else if (this.mergeHash == null) {
113-
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Unknown merge state " + this.toString());
114-
} else if (this.mergeHash == NOT_MERGEABLE_HASH) {
115-
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Not mergeable " + this.toString());
116-
} else {
117-
GHCommit commit = null;
118-
try {
119-
commit = repo.getCommit(this.mergeHash);
120-
} catch (FileNotFoundException e) {
121-
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : commit not found (" + this.mergeHash + "). Close and reopen the PR to reset its merge hash.");
122-
} catch (IOException e) {
123-
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : " + e.toString());
124-
}
125-
assert(commit != null);
126-
List<String> parents = commit.getParentSHA1s();
127-
if (parents.size() != 2 || !parents.contains(this.getBaseHash()) || !parents.contains(this.getPullHash())) {
128-
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Head and base commits do match merge commit " + this.toString() );
129-
}
109+
void validateMergeHash() throws AbortException {
110+
if (this.mergeHash == NOT_MERGEABLE_HASH) {
111+
throw new AbortException("Pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Not mergeable at " + this.toString());
130112
}
131113
}
132114

‎src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystemTest.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -284,14 +284,14 @@ public void resolveDirPRMerge() throws Exception {
284284
assertThat(fs.getRoot().child("fu").getType(), is(SCMFile.Type.DIRECTORY));
285285
}
286286

287-
@Test(expected = AbortException.class)
288-
public void resolveDirPRInvalidMerge() throws Exception {
289-
assumeThat(revision, nullValue());
287+
// @Test(expected = AbortException.class)
288+
// public void resolveDirPRInvalidMerge() throws Exception {
289+
// assumeThat(revision, nullValue());
290290

291-
assertThat(prMergeInvalidRevision.isMerge(), is(true));
291+
// assertThat(prMergeInvalidRevision.isMerge(), is(true));
292292

293-
SCMFileSystem fs = SCMFileSystem.of(source, prMerge, prMergeInvalidRevision);
294-
assertThat(fs, instanceOf(GitHubSCMFileSystem.class));
295-
}
293+
// SCMFileSystem fs = SCMFileSystem.of(source, prMerge, prMergeInvalidRevision);
294+
// assertThat(fs, instanceOf(GitSCMFileSystem.class));
295+
// }
296296

297297
}

0 commit comments

Comments
 (0)