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 731745246..69ec96c83 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 @@ -178,11 +178,12 @@ private List botSpecificChecks() throws IOException { 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 last merge commit - the very last commit is not eligible (as the merge needs a parent) + // 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) { @@ -198,10 +199,12 @@ private List botSpecificChecks() throws IOException { try { var sourceHash = prInstance.localRepo().fetch(mergeSourceRepo.url(), source.get().branchName); var mergeCommit = commits.get(mergeCommitIndex); - for (int i = 1; i < mergeCommit.parents().size(); ++i) { + for (int i = 0; i < mergeCommit.parents().size(); ++i) { if (!prInstance.localRepo().isAncestor(mergeCommit.parents().get(i), sourceHash)) { - ret.add("The merge contains commits that are not ancestors of the source."); - break; + 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) { 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 845a895ca..61f31f9a8 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 @@ -146,7 +146,7 @@ private Hash commitMerge(List activeReviews, Namespace namespace, String for (int i = 0; i < commits.size() - 1; ++i) { if (commits.get(i).isMerge()) { boolean isSourceMerge = false; - for (int j = 1; j < commits.get(i).parents().size(); ++j) { + for (int j = 0; j < commits.get(i).parents().size(); ++j) { if (!localRepo.isAncestor(baseHash, commits.get(i).parents().get(j))) { isSourceMerge = true; } @@ -154,12 +154,15 @@ private Hash commitMerge(List activeReviews, Namespace namespace, String if (isSourceMerge) { mergeCommitIndex = i; break; + } else { + // TODO: We can solve this with retroactive rerere + throw new CommitFailure("A merge PR is only allowed to contain a single merge commit. You will need to amend your commits."); } } } if (mergeCommitIndex == commits.size()) { - throw new CommitFailure("No merge commit containing commits from another branch than the target was found"); + throw new CommitFailure("No merge commit containing incoming commits from another branch than the target was found"); } var contributor = namespace.get(pr.author().id()); @@ -183,9 +186,13 @@ private Hash commitMerge(List activeReviews, Namespace namespace, String return localRepo.amend(commitMessage, author.name(), author.email(), committer.name(), committer.email()); } + private boolean isMergeCommit() { + return pr.title().startsWith("Merge"); + } + Hash commit(Namespace namespace, String censusDomain, String sponsorId) throws IOException, CommitFailure { var activeReviews = filterActiveReviews(pr.reviews()); - if (!pr.title().startsWith("Merge")) { + if (!isMergeCommit()) { return commitSquashed(activeReviews, namespace, censusDomain, sponsorId); } else { return commitMerge(activeReviews, namespace, censusDomain, sponsorId); @@ -215,6 +222,12 @@ Optional rebase(Hash commitHash, PrintWriter reply) { try { var commit = localRepo.lookup(commitHash).orElseThrow(); + if (isMergeCommit()) { + // TODO: We can solve this with retroactive rerere + reply.println("Merge PRs cannot currently be automatically rebased. You will need to rebase it manually."); + return Optional.empty(); + } + localRepo.rebase(targetHash, commit.committer().name(), commit.committer().email()); reply.println(); reply.println("Your commit was automatically rebased without conflicts."); 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 cace9ea2b..cc4187085 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 @@ -209,6 +209,7 @@ void branchMergeShortName(TestInfo testInfo) throws IOException { } @Test + @Disabled void branchMergeRebase(TestInfo testInfo) throws IOException { try (var credentials = new HostCredentials(testInfo); var tempFolder = new TemporaryDirectory()) { @@ -303,6 +304,7 @@ void branchMergeRebase(TestInfo testInfo) throws IOException { } @Test + @Disabled void branchMergeAdditionalCommits(TestInfo testInfo) throws IOException { try (var credentials = new HostCredentials(testInfo); var tempFolder = new TemporaryDirectory()) { @@ -466,7 +468,7 @@ void invalidMergeCommit(TestInfo testInfo) throws IOException { 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()); + assertEquals("- It was not possible to create a commit for the changes in this PR: A merge PR is only allowed to contain a single merge commit. You will need to amend your commits.", check.summary().orElseThrow()); } } @@ -683,6 +685,7 @@ void wrongSourceBranch(TestInfo testInfo) throws IOException { localRepo.push(other2Hash, author.url(), "other2", true); // Make a change with a corresponding PR + localRepo.checkout(masterHash, true); var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8); localRepo.add(unrelated); var updatedMaster = localRepo.commit("Unrelated", "some", "some@one"); @@ -711,7 +714,7 @@ void wrongSourceBranch(TestInfo testInfo) throws IOException { assertEquals(1, error, () -> pr.comments().stream().map(Comment::body).collect(Collectors.joining("\n\n"))); var check = pr.checks(mergeHash).get("jcheck"); - assertEquals("- The merge contains commits that are not ancestors of the source.", check.summary().orElseThrow()); + assertEquals("- The merge contains commits that are neither ancestors of the source nor the target.", check.summary().orElseThrow()); } } @@ -836,11 +839,14 @@ void unrelatedHistory(TestInfo testInfo) throws IOException { pr.addComment("/integrate"); TestBotRunner.runPeriodicItems(mergeBot); - // The bot should reply with an ok message as we currently allow this - var pushed = pr.comments().stream() - .filter(comment -> comment.body().contains("Pushed as commit")) - .count(); - assertEquals(1, pushed); + // 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("- The merge contains commits that are neither ancestors of the source nor the target.", check.summary().orElseThrow()); } } diff --git a/vcs/src/main/java/org/openjdk/skara/vcs/git/GitRepository.java b/vcs/src/main/java/org/openjdk/skara/vcs/git/GitRepository.java index 210b26a56..d80fb2404 100644 --- a/vcs/src/main/java/org/openjdk/skara/vcs/git/GitRepository.java +++ b/vcs/src/main/java/org/openjdk/skara/vcs/git/GitRepository.java @@ -727,7 +727,7 @@ public boolean isAncestor(Hash ancestor, Hash descendant) throws IOException { @Override public void rebase(Hash hash, String committerName, String committerEmail) throws IOException { - try (var p = Process.capture("git", "rebase", "--onto", hash.hex(), "--root", "--rebase-merges") + try (var p = Process.capture("git", "rebase", "--onto", hash.hex(), "--root") .environ("GIT_COMMITTER_NAME", committerName) .environ("GIT_COMMITTER_EMAIL", committerEmail) .workdir(dir)