Skip to content

[JENKINS-43194] Use merge_commit_sha for master #209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bitwiseman merged 3 commits into from
Mar 27, 2019
Merged

[JENKINS-43194] Use merge_commit_sha for master #209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6addac1
[JENKINS-43194] Use merge_commit_sha for master
bitwiseman Mar 1, 2019
dbd2786
Revert minor code cleanup to highlight only merge_commit_sha change
bitwiseman Mar 7, 2019
fe37f64
Check for inconsistent merge commit
bitwiseman Mar 25, 2019
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
22 changes: 16 additions & 6 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,15 @@
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;
import hudson.scm.SCM;
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;

Expand Down Expand Up @@ -83,9 +85,17 @@ protected GitHubSCMFileSystem(GitHub gitHub, GHRepository repo, String refName,
this.repo = repo;
if (rev != null) {
if (rev.getHead() instanceof PullRequestSCMHead) {
PullRequestSCMHead pr = (PullRequestSCMHead) rev.getHead();
assert !pr.isMerge(); // TODO see below
this.ref = ((PullRequestSCMRevision) rev).getPullHash();
PullRequestSCMRevision prRev = (PullRequestSCMRevision) rev;
PullRequestSCMHead pr = (PullRequestSCMHead) prRev.getHead();
if (pr.isMerge()) {
this.ref = prRev.getMergeHash();
List<String> 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 = prRev.getPullHash();
}
} else if (rev instanceof AbstractGitSCMSource.SCMRevisionImpl) {
this.ref = ((AbstractGitSCMSource.SCMRevisionImpl) rev).getHash();
} else {
Expand Down Expand Up @@ -271,7 +281,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
Expand All @@ -288,12 +298,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);
Expand Down
106 changes: 79 additions & 27 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
),
Expand All @@ -981,19 +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();
Copy link
Member

@jglick jglick Mar 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessarily a meaningful value, as noted here. It is in particular not necessarily the current head commit of the base branch (what is being calculated in the deleted code).

Copy link
Contributor Author

@bitwiseman bitwiseman Mar 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a change from the existing behavior - if you read the code, all I did was move the assignments/return statements around. If you like, I can put them back so they don't show up in the diff.

Copy link
Member

@jglick jglick Mar 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all I did was move the assignments/return statements around

Unclear to me. The original code called GhRepository.getRef on the base branch, which would retrieve its current head. The new code is extracting .base.sha from the PR API response object, which can be a completely different value. (IIRC it is typically, but not always, the last common ancestor between the two branches.)

I can put them back so they don't show up in the diff.

If some part of a patch can be reverted so that the real changes can be seen to have more localized effects than of course this is preferable.

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/" + pr.getBase().getRef()).getObject().getSha();
mergeHash = pr.getMergeCommitSha();
Copy link
Member

@jglick jglick Mar 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is going to produce a synthetic commit which may or may not have as parents the baseHash and the .head.sha. Depends on exactly when you call it.

Copy link
Contributor Author

@bitwiseman bitwiseman Mar 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I'm not sure what you're pointing out here or why.

Copy link
Member

@jglick jglick Mar 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That the new behavior will be nondeterministic, and the PullRequestSCMRevision may wind up with three fields that are not in fact consistent.

break;
default:
return new PullRequestSCMRevision(head, pr.getBase().getSha(),
pr.getHead().getSha());
break;
}
return new PullRequestSCMRevision(head, baseHash, prHeadHash, mergeHash);
}
},
new MergabilityWitness(pr, strategy, listener),
Expand Down Expand Up @@ -1200,7 +1202,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<ChangeRequestCheckoutStrategy> strategies;
Expand Down Expand Up @@ -1253,32 +1255,53 @@ 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;

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;
}
List<String> 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%n",
"Resolved %s as pull request %d at revision %s merged onto %s as %s%n",
headName,
number,
pr.getHead().getSha(),
pr.getBase().getSha()
prHeadHash,
baseHash,
mergeHash
);
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",
headName,
number,
pr.getHead().getSha()
prHeadHash
);
return new PullRequestSCMRevision(head, pr.getBase().getSha(),
pr.getHead().getSha());
break;
}
return new PullRequestSCMRevision(head,
baseHash,
prHeadHash,
mergeHash);
} else {
listener.getLogger().format(
"Could not resolve %s as pull request %d%n",
Expand Down Expand Up @@ -1438,21 +1461,32 @@ 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 prHeadHash = pr.getHead().getSha();
String mergeHash = null;
switch (prhead.getCheckoutStrategy()) {
case MERGE:
baseHash = ghRepository.getRef("heads/" + prhead.getTarget().getName()).getObject().getSha();
assert(pr.getMergeable() != null);
if (Boolean.FALSE.equals(pr.getMergeable())) {
throw new AbortException("Pull request " + prhead.getNumber() + " is not mergeable.");
}
baseHash = ghRepository.getRef("heads/" + pr.getBase().getRef()).getObject().getSha();
mergeHash = pr.getMergeCommitSha();
List<String> 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:
baseHash = pr.getBase().getSha();
break;
}
return new PullRequestSCMRevision(prhead, baseHash, pr.getHead().getSha());
return new PullRequestSCMRevision(prhead, baseHash, prHeadHash, mergeHash);
} else if (head instanceof GitHubTagSCMHead) {
GitHubTagSCMHead tagHead = (GitHubTagSCMHead) head;
GHRef tag = ghRepository.getRef("tags/" + tagHead.getName());
Expand All @@ -1475,6 +1509,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();
Expand Down
21 changes: 19 additions & 2 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevision.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -36,16 +37,22 @@
* Revision of a pull request.
*/
public class PullRequestSCMRevision extends ChangeRequestSCMRevision<PullRequestSCMHead> {

private static final long serialVersionUID = 1L;

private final @NonNull String baseHash;
private final @NonNull String pullHash;
private final 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, 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"})
Expand Down Expand Up @@ -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
Copy link
Member

@jglick jglick Mar 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be null if you load an old serialized object, and yet the one caller is not checking for null.

Copy link
Contributor Author

@bitwiseman bitwiseman Mar 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is once again, POC. The scenario for the caller is guaranteed to never have null.
In final version, this will need to be safely migrated.

Copy link
Member

@jglick jglick Mar 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I was just mentioning a potential problem while I was here, in case you had not noticed it yet.

public String getMergeHash() {
return mergeHash;
}

@Override
public boolean equivalent(ChangeRequestSCMRevision<?> o) {
if (!(o instanceof PullRequestSCMRevision)) {
Expand All @@ -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;
}

}