Skip to content
Permalink
Browse files
Improve handling of squash and rebase of PRs
Reviewed-by: ehelin
  • Loading branch information
rwestberg committed Apr 20, 2020
1 parent 2969125 commit 15cf5f695367f7b30b5507a57f2c9a8c231bf91f
Show file tree
Hide file tree
Showing 12 changed files with 305 additions and 172 deletions.
@@ -52,8 +52,6 @@ class CheckRun {
private final String mergeReadyMarker = "<!-- PullRequestBot merge is ready comment -->";
private final String outdatedHelpMarker = "<!-- PullRequestBot outdated help comment -->";
private final String sourceBranchWarningMarker = "<!-- PullRequestBot source branch warning comment -->";
private final Pattern mergeSourceFullPattern = Pattern.compile("^Merge ([-/\\w]+):([-\\w]+)$");
private final Pattern mergeSourceBranchOnlyPattern = Pattern.compile("^Merge ([-\\w]+)$");
private final Set<String> newLabels;

private CheckRun(CheckWorkItem workItem, PullRequest pr, PullRequestInstance prInstance, List<Comment> comments,
@@ -112,42 +110,8 @@ private boolean checkCommitAuthor(List<CommitMetadata> commits) throws IOExcepti
return ((names.size() == 1) && emails.size() == 1);
}

private static class MergeSource {
private final String repositoryName;
private final String branchName;

private MergeSource(String repositoryName, String branchName) {
this.repositoryName = repositoryName;
this.branchName = branchName;
}
}

private Optional<MergeSource> mergeSource() {
var repoMatcher = mergeSourceFullPattern.matcher(pr.title());
if (!repoMatcher.matches()) {
var branchMatcher = mergeSourceBranchOnlyPattern.matcher(pr.title());
if (!branchMatcher.matches()) {
return Optional.empty();
}

// Verify that the branch exists
var isValidBranch = prInstance.remoteBranches().stream()
.map(Reference::name)
.anyMatch(branch -> branch.equals(branchMatcher.group(1)));
if (!isValidBranch) {
// Assume the name refers to a sibling repository
var repoName = Path.of(pr.repository().name()).resolveSibling(branchMatcher.group(1)).toString();
return Optional.of(new MergeSource(repoName, "master"));
}

return Optional.of(new MergeSource(pr.repository().name(), branchMatcher.group(1)));
}

return Optional.of(new MergeSource(repoMatcher.group(1), repoMatcher.group(2)));
}

// Additional bot-specific checks that are not handled by JCheck
private List<String> botSpecificChecks() throws IOException {
private List<String> botSpecificChecks(Hash finalHash) throws IOException {
var ret = new ArrayList<String>();

if (bodyWithoutStatus().isBlank()) {
@@ -165,63 +129,15 @@ private List<String> botSpecificChecks() throws IOException {

var baseHash = prInstance.baseHash();
var headHash = pr.headHash();
var commits = prInstance.localRepo().commitMetadata(baseHash, headHash);
var originalCommits = prInstance.localRepo().commitMetadata(baseHash, headHash);

if (!checkCommitAuthor(commits)) {
if (!checkCommitAuthor(originalCommits)) {
var error = "For contributors who are not existing OpenJDK Authors, commit attribution will be taken from " +
"the commits in the PR. However, the commits in this PR have inconsistent user names and/or " +
"email addresses. Please amend the commits.";
ret.add(error);
}

if (pr.title().startsWith("Merge")) {
if (commits.size() < 2) {
ret.add("A Merge PR must contain at least two commits that are not already present in the target.");
} else {
// Find the first merge commit - the very last commit is not eligible (as the merge needs a parent)
int mergeCommitIndex = commits.size();
for (int i = 0; i < commits.size() - 1; ++i) {
if (commits.get(i).isMerge()) {
mergeCommitIndex = i;
break;
}
}
if (mergeCommitIndex >= commits.size() - 1) {
ret.add("A Merge PR must contain a merge commit as well as at least one other commit from the merge source.");
}

var source = mergeSource();
if (source.isPresent()) {
try {
var mergeSourceRepo = pr.repository().forge().repository(source.get().repositoryName).orElseThrow(() ->
new RuntimeException("Could not find repository " + source.get().repositoryName)
);
try {
var sourceHash = prInstance.localRepo().fetch(mergeSourceRepo.url(), source.get().branchName, false);
var mergeCommit = commits.get(mergeCommitIndex);
for (int i = 0; i < mergeCommit.parents().size(); ++i) {
if (!prInstance.localRepo().isAncestor(mergeCommit.parents().get(i), sourceHash)) {
if (!mergeCommit.parents().get(i).equals(prInstance.targetHash())) {
ret.add("The merge contains commits that are neither ancestors of the source nor the target.");
break;
}
}
}
} catch (IOException e) {
ret.add("Could not fetch branch `" + source.get().branchName + "` from project `" +
source.get().repositoryName + "` - check that they are correct.");
}
} catch (RuntimeException e) {
ret.add("Could not find project `" +
source.get().repositoryName + "` - check that it is correct.");
}
} else {
ret.add("Could not determine the source for this merge. A Merge PR title must be specified on the format: " +
"Merge `project`:`branch` to allow verification of the merge contents.");
}
}
}

for (var blocker : workItem.bot.blockingCheckLabels().entrySet()) {
if (labels.contains(blocker.getKey())) {
ret.add(blocker.getValue());
@@ -667,30 +583,31 @@ private void checkStatus() {
// Post check in-progress
log.info("Starting to run jcheck on PR head");
pr.createCheck(checkBuilder.build());

var ignored = new PrintWriter(new StringWriter());
var rebasePossible = true;
var commitHash = pr.headHash();
var mergedHash = prInstance.mergeTarget(ignored);
if (mergedHash.isPresent()) {
commitHash = mergedHash.get();
} else {
rebasePossible = false;
}

List<String> additionalErrors = List.of();
Hash localHash;
try {
localHash = prInstance.commit(censusInstance.namespace(), censusDomain, null);
localHash = prInstance.commit(commitHash, censusInstance.namespace(), censusDomain, null);
} catch (CommitFailure e) {
additionalErrors = List.of("It was not possible to create a commit for the changes in this PR: " + e.getMessage());
additionalErrors = List.of(e.getMessage());
localHash = prInstance.baseHash();
}
boolean rebasePossible = true;
PullRequestCheckIssueVisitor visitor = prInstance.createVisitor(localHash, censusInstance);
if (!localHash.equals(prInstance.baseHash())) {
// Try to rebase
var ignored = new PrintWriter(new StringWriter());
var rebasedHash = prInstance.rebase(localHash, ignored);
if (rebasedHash.isEmpty()) {
rebasePossible = false;
} else {
localHash = rebasedHash.get();
}

// Determine current status
var additionalConfiguration = AdditionalConfiguration.get(prInstance.localRepo(), localHash, pr.repository().forge().currentUser(), comments);
prInstance.executeChecks(localHash, censusInstance, visitor, additionalConfiguration);
additionalErrors = botSpecificChecks();
additionalErrors = botSpecificChecks(localHash);
} else {
if (additionalErrors.isEmpty()) {
additionalErrors = List.of("This PR contains no changes");
@@ -96,8 +96,6 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
new HostedRepositoryPool(seedPath),
pr,
bot.ignoreStaleReviews());
var localHash = prInstance.commit(censusInstance.namespace(), censusInstance.configuration().census().domain(), null);

// Validate the target hash if requested
var rebaseMessage = new StringWriter();
if (!args.isBlank()) {
@@ -109,18 +107,16 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
}
};

// Now rebase onto the target hash
// Now merge the latest changes from the target
var rebaseWriter = new PrintWriter(rebaseMessage);
var rebasedHash = prInstance.rebase(localHash, rebaseWriter);
var rebasedHash = prInstance.mergeTarget(rebaseWriter);
if (rebasedHash.isEmpty()) {
reply.println(rebaseMessage.toString());
return;
} else {
if (!rebasedHash.get().equals(localHash)) {
localHash = rebasedHash.get();
}
}

var localHash = prInstance.commit(rebasedHash.get(), censusInstance.namespace(), censusInstance.configuration().census().domain(), null);

var issues = prInstance.createVisitor(localHash, censusInstance);
var additionalConfiguration = AdditionalConfiguration.get(prInstance.localRepo(), localHash, pr.repository().forge().currentUser(), allComments);
prInstance.executeChecks(localHash, censusInstance, issues, additionalConfiguration);
@@ -159,8 +155,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
}
} catch (Exception e) {
log.throwing("IntegrateCommand", "handle", e);
reply.println("An error occurred during final integration jcheck");
throw new RuntimeException(e);
reply.println("An error occurred during final integration jcheck. No push attempt will be made.");
}
}

0 comments on commit 15cf5f6

Please sign in to comment.