diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java index a3d228591..a53f884f8 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java @@ -577,10 +577,16 @@ private void checkStatus() { // Post check in-progress log.info("Starting to run jcheck on PR head"); pr.createCheck(checkBuilder.build()); - var localHash = prInstance.commit(censusInstance.namespace(), censusDomain, null); + List additionalErrors = List.of(); + Hash localHash; + try { + localHash = prInstance.commit(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()); + localHash = prInstance.baseHash(); + } boolean rebasePossible = true; PullRequestCheckIssueVisitor visitor = prInstance.createVisitor(localHash, censusInstance); - List additionalErrors; if (!localHash.equals(prInstance.baseHash())) { // Try to rebase var ignored = new PrintWriter(new StringWriter()); @@ -595,9 +601,10 @@ private void checkStatus() { var additionalConfiguration = AdditionalConfiguration.get(prInstance.localRepo(), localHash, pr.repository().forge().currentUser(), comments); prInstance.executeChecks(localHash, censusInstance, visitor, additionalConfiguration); additionalErrors = botSpecificChecks(); - } - else { - additionalErrors = List.of("This PR contains no changes"); + } else { + if (additionalErrors.isEmpty()) { + additionalErrors = List.of("This PR contains no changes"); + } } updateCheckBuilder(checkBuilder, visitor, additionalErrors); updateReadyForReview(visitor, additionalErrors); diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestInstance.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestInstance.java index e669de819..0559c1df2 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestInstance.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestInstance.java @@ -35,6 +35,12 @@ import java.util.*; import java.util.stream.Collectors; +class CommitFailure extends Exception { + CommitFailure(String reason) { + super(reason); + } +} + class PullRequestInstance { private final PullRequest pr; private final Repository localRepo; @@ -132,19 +138,33 @@ private Hash commitSquashed(List activeReviews, Namespace namespace, Str return localRepo.commit(commitMessage, author.name(), author.email(), committer.name(), committer.email()); } - private Hash commitMerge(List activeReviews, Namespace namespace, String censusDomain) throws IOException { - // Find the last merge commit - the very last commit is not eligible (as the merge needs a parent) - var commits = localRepo.commits(baseHash + ".." + headHash).asList(); + private Hash commitMerge(List activeReviews, Namespace namespace, String censusDomain) throws IOException, CommitFailure { + // Find the first merge commit with an incoming parent outside of the merge target + // The very last commit is not eligible (as the merge needs a parent) + var commits = localRepo.commitMetadata(baseHash, headHash); int mergeCommitIndex = commits.size(); for (int i = 0; i < commits.size() - 1; ++i) { if (commits.get(i).isMerge()) { - mergeCommitIndex = i; + boolean isSourceMerge = false; + for (int j = 1; j < commits.get(i).parents().size(); ++j) { + if (!localRepo.isAncestor(baseHash, commits.get(i).parents().get(j))) { + isSourceMerge = true; + } + } + if (isSourceMerge) { + mergeCommitIndex = i; + break; + } } } + if (mergeCommitIndex == commits.size()) { + throw new CommitFailure("No merge commit containing commits from another branch than the target was found"); + } + var contributor = namespace.get(pr.author().id()); if (contributor == null) { - throw new RuntimeException("Merges can only be performed by Committers"); + throw new CommitFailure("Merges can only be performed by Committers"); } var author = new Author(contributor.fullName().orElseThrow(), contributor.username() + "@" + censusDomain); @@ -156,7 +176,7 @@ private Hash commitMerge(List activeReviews, Namespace namespace, String return localRepo.amend(commitMessage, author.name(), author.email(), author.name(), author.email()); } - Hash commit(Namespace namespace, String censusDomain, String sponsorId) throws IOException { + Hash commit(Namespace namespace, String censusDomain, String sponsorId) throws IOException, CommitFailure { var activeReviews = filterActiveReviews(pr.reviews()); if (!pr.title().startsWith("Merge")) { return commitSquashed(activeReviews, namespace, censusDomain, sponsorId); @@ -165,16 +185,21 @@ Hash commit(Namespace namespace, String censusDomain, String sponsorId) throws I } } - List divergingCommits() { + List divergingCommits() { + return divergingCommits(headHash); + } + + private List divergingCommits(Hash commitHash) { try { - return localRepo.commits(baseHash + ".." + targetHash).asList(); + var updatedBase = localRepo.mergeBase(targetHash, commitHash); + return localRepo.commitMetadata(updatedBase, targetHash); } catch (IOException e) { throw new RuntimeException(e); } } Optional rebase(Hash commitHash, PrintWriter reply) { - var divergingCommits = divergingCommits(); + var divergingCommits = divergingCommits(commitHash); if (divergingCommits.size() > 0) { reply.print("The following commits have been pushed to "); reply.print(pr.targetRef()); diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/MergeTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/MergeTests.java index e353bb3ff..2c8f601ed 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/MergeTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/MergeTests.java @@ -73,8 +73,10 @@ void branchMerge(TestInfo testInfo) throws IOException { // Make a change with a corresponding PR var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8); localRepo.add(unrelated); - localRepo.commit("Unrelated", "some", "some@one"); + var updatedMaster = localRepo.commit("Unrelated", "some", "some@one"); localRepo.merge(otherHash2); + localRepo.push(updatedMaster, author.url(), "master"); + var mergeHash = localRepo.commit("Merge commit", "some", "some@one"); localRepo.push(mergeHash, author.url(), "edit", true); var pr = credentials.createPullRequest(author, "master", "edit", "Merge " + author.name() + ":other"); @@ -155,7 +157,9 @@ void branchMergeShortName(TestInfo testInfo) throws IOException { // Make a change with a corresponding PR var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8); localRepo.add(unrelated); - localRepo.commit("Unrelated", "some", "some@one"); + var updatedMaster = localRepo.commit("Unrelated", "some", "some@one"); + localRepo.push(updatedMaster, author.url(), "master"); + localRepo.merge(otherHash2); var mergeHash = localRepo.commit("Merge commit", "some", "some@one"); localRepo.push(mergeHash, author.url(), "edit", true); @@ -238,6 +242,8 @@ void branchMergeRebase(TestInfo testInfo) throws IOException { var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8); localRepo.add(unrelated); var updatedMaster = localRepo.commit("Unrelated", "some", "some@one"); + localRepo.push(updatedMaster, author.url(), "master"); + localRepo.merge(otherHash2); var mergeHash = localRepo.commit("Merge commit", "some", "some@one"); localRepo.push(mergeHash, author.url(), "edit", true); @@ -330,6 +336,8 @@ void branchMergeAdditionalCommits(TestInfo testInfo) throws IOException { var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8); localRepo.add(unrelated); var updatedMaster = localRepo.commit("Unrelated", "some", "some@one"); + localRepo.push(updatedMaster, author.url(), "master"); + localRepo.merge(otherHash2); var mergeHash = localRepo.commit("Our own merge commit", "some", "some@one"); localRepo.push(mergeHash, author.url(), "edit", true); @@ -360,6 +368,14 @@ void branchMergeAdditionalCommits(TestInfo testInfo) throws IOException { // Let the bot notice again TestBotRunner.runPeriodicItems(mergeBot); + // Merge the latest from master + localRepo.merge(newMasterHash); + var latestMergeHash = localRepo.commit("Our to be squashed merge commit", "some", "some@one"); + localRepo.push(latestMergeHash, author.url(), "edit"); + + // Let the bot notice again + TestBotRunner.runPeriodicItems(mergeBot); + // Push it pr.addComment("/integrate"); TestBotRunner.runPeriodicItems(mergeBot); @@ -397,7 +413,7 @@ void branchMergeAdditionalCommits(TestInfo testInfo) throws IOException { } @Test - void invalidSourceRepo(TestInfo testInfo) throws IOException { + void invalidMergeCommit(TestInfo testInfo) throws IOException { try (var credentials = new HostCredentials(testInfo); var tempFolder = new TemporaryDirectory()) { @@ -427,6 +443,66 @@ void invalidSourceRepo(TestInfo testInfo) throws IOException { var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8); localRepo.add(unrelated); localRepo.commit("Unrelated", "some", "some@one"); + localRepo.merge(otherHash); + var mergeHash = localRepo.commit("Merge commit", "some", "some@one"); + localRepo.push(mergeHash, author.url(), "edit", true); + var pr = credentials.createPullRequest(author, "master", "edit", "Merge " + author.name() + ":other"); + + // Approve it as another user + var approvalPr = integrator.pullRequest(pr.id()); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); + + // Let the bot check the status + TestBotRunner.runPeriodicItems(mergeBot); + + // Push it + pr.addComment("/integrate"); + TestBotRunner.runPeriodicItems(mergeBot); + + // The bot should reply with a failure message + var error = pr.comments().stream() + .filter(comment -> comment.body().contains("did not complete successfully")) + .count(); + assertEquals(1, error, () -> pr.comments().stream().map(Comment::body).collect(Collectors.joining("\n\n"))); + + var check = pr.checks(mergeHash).get("jcheck"); + assertEquals("- It was not possible to create a commit for the changes in this PR: No merge commit containing commits from another branch than the target was found", check.summary().orElseThrow()); + } + } + + @Test + void invalidSourceRepo(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory()) { + + var author = credentials.getHostedRepository(); + var integrator = credentials.getHostedRepository(); + var censusBuilder = credentials.getCensusBuilder() + .addCommitter(author.forge().currentUser().id()) + .addReviewer(integrator.forge().currentUser().id()); + var mergeBot = PullRequestBot.newBuilder().repo(integrator).censusRepo(censusBuilder.build()).build(); + + // Populate the projects repository + var localRepoFolder = tempFolder.path().resolve("localrepo"); + var localRepo = CheckableRepository.init(localRepoFolder, author.repositoryType()); + var masterHash = localRepo.resolve("master").orElseThrow(); + assertFalse(CheckableRepository.hasBeenEdited(localRepo)); + localRepo.push(masterHash, author.url(), "master", true); + + // Make a change in another branch + var otherHash = CheckableRepository.appendAndCommit(localRepo, "Change in other", + "Other\n\nReviewed-by: integrationreviewer2"); + localRepo.push(otherHash, author.url(), "other", true); + + // Go back to the original master + localRepo.checkout(masterHash, true); + + // Make a change with a corresponding PR + var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8); + localRepo.add(unrelated); + var updatedMaster = localRepo.commit("Unrelated", "some", "some@one"); + localRepo.push(updatedMaster, author.url(), "master"); + localRepo.merge(otherHash); var mergeHash = localRepo.commit("Merge commit", "some", "some@one"); localRepo.push(mergeHash, author.url(), "edit", true); @@ -484,7 +560,9 @@ void invalidSourceBranch(TestInfo testInfo) throws IOException { // Make a change with a corresponding PR var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8); localRepo.add(unrelated); - localRepo.commit("Unrelated", "some", "some@one"); + var updatedMaster = localRepo.commit("Unrelated", "some", "some@one"); + localRepo.push(updatedMaster, author.url(), "master"); + localRepo.merge(otherHash); var mergeHash = localRepo.commit("Merge commit", "some", "some@one"); localRepo.push(mergeHash, author.url(), "edit", true); @@ -547,7 +625,9 @@ void wrongSourceBranch(TestInfo testInfo) throws IOException { // Make a change with a corresponding PR var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8); localRepo.add(unrelated); - localRepo.commit("Unrelated", "some", "some@one"); + var updatedMaster = localRepo.commit("Unrelated", "some", "some@one"); + localRepo.push(updatedMaster, author.url(), "master"); + localRepo.merge(other1Hash, "ours"); var mergeHash = localRepo.commit("Merge commit", "some", "some@one"); localRepo.push(mergeHash, author.url(), "edit", true); @@ -605,7 +685,9 @@ void invalidAuthor(TestInfo testInfo) throws IOException { // Make a change with a corresponding PR var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8); localRepo.add(unrelated); - localRepo.commit("Unrelated", "some", "some@one"); + var updatedMaster = localRepo.commit("Unrelated", "some", "some@one"); + localRepo.push(updatedMaster, author.url(), "master"); + localRepo.merge(otherHash); var mergeHash = localRepo.commit("Merge commit", "some", "some@one"); localRepo.push(mergeHash, author.url(), "edit", true);