diff --git a/bots/merge/build.gradle b/bots/merge/build.gradle index 0871ad692..3cd36ffc9 100644 --- a/bots/merge/build.gradle +++ b/bots/merge/build.gradle @@ -39,6 +39,7 @@ dependencies { implementation project(':census') implementation project(':json') implementation project(':vcs') + implementation project(':jcheck') testImplementation project(':test') } diff --git a/bots/merge/src/main/java/module-info.java b/bots/merge/src/main/java/module-info.java index 1852a4c47..2b7638ec3 100644 --- a/bots/merge/src/main/java/module-info.java +++ b/bots/merge/src/main/java/module-info.java @@ -23,6 +23,7 @@ module org.openjdk.skara.bots.merge { requires org.openjdk.skara.bot; requires org.openjdk.skara.vcs; + requires org.openjdk.skara.jcheck; requires java.logging; provides org.openjdk.skara.bot.BotFactory with org.openjdk.skara.bots.merge.MergeBotFactory; diff --git a/bots/merge/src/main/java/org/openjdk/skara/bots/merge/MergeBot.java b/bots/merge/src/main/java/org/openjdk/skara/bots/merge/MergeBot.java index 6030334c0..fe78f61ff 100644 --- a/bots/merge/src/main/java/org/openjdk/skara/bots/merge/MergeBot.java +++ b/bots/merge/src/main/java/org/openjdk/skara/bots/merge/MergeBot.java @@ -25,6 +25,7 @@ import org.openjdk.skara.bot.*; import org.openjdk.skara.forge.*; import org.openjdk.skara.vcs.*; +import org.openjdk.skara.jcheck.JCheckConfiguration; import java.io.IOException; import java.io.UncheckedIOException; @@ -263,38 +264,58 @@ public void run(Path scratchPath) { var fromRepo = spec.fromRepo(); var fromBranch = spec.fromBranch(); - log.info("Deciding whether to merge " + fromRepo.name() + ":" + fromBranch.name() + " to " + toBranch.name()); - - // Checkout the branch to merge into - repo.checkout(toBranch, false); - var remoteBranch = new Branch(repo.upstreamFor(toBranch).orElseThrow(() -> - new IllegalStateException("Could not get remote branch name for " + toBranch.name()) - )); - repo.merge(remoteBranch); // should always be a fast-forward merge - var targetName = Path.of(target.name()).getFileName(); var fromName = Path.of(fromRepo.name()).getFileName(); var fromDesc = targetName.equals(fromName) ? fromBranch.name() : fromName + ":" + fromBranch.name(); // Check if merge conflict pull request is present var shouldMerge = true; - var title = "Cannot automatically merge " + fromDesc + " to " + toBranch.name(); - var marker = ""; + var title = "Merge " + fromDesc; + var marker = ""; for (var pr : prs) { if (pr.title().equals(title) && + pr.targetRef().equals(toBranch.name()) && pr.body().startsWith(marker) && currentUser.equals(pr.author())) { - var lines = pr.body().split("\n"); - var head = new Hash(lines[1].substring(5, 45)); - if (repo.contains(toBranch, head)) { - log.info("Closing resolved merge conflict PR " + pr.id() + ", will try merge"); - pr.addComment("Merge conflicts have been resolved, closing this PR"); - pr.setState(PullRequest.State.CLOSED); - } else { - log.info("Outstanding unresolved merge already present, will not merge"); - shouldMerge = false; + // Yes, this could be optimized do a merge "this turn", but it is much simpler + // to just wait until the next time the bot runs + shouldMerge = false; + + if (pr.labels().contains("ready") && !pr.labels().contains("sponsor")) { + var comments = pr.comments(); + var integrateComments = + comments.stream() + .filter(c -> c.author().equals(currentUser)) + .filter(c -> c.body().equals("/integrate")) + .collect(Collectors.toList()); + if (integrateComments.isEmpty()) { + pr.addComment("/integrate"); + } else { + var lastIntegrateComment = integrateComments.get(integrateComments.size() - 1); + var id = lastIntegrateComment.id(); + var botUserId = "43336822"; + var replyMarker = ""; + var replies = comments.stream() + .filter(c -> c.author().id().equals(botUserId)) + .filter(c -> c.body().startsWith(replyMarker)) + .collect(Collectors.toList()); + if (replies.isEmpty()) { + // No reply yet, just wait + } else { + // Got a reply and the "sponsor" label is not present, check for error + // and if we should add the `/integrate` command again + var lastReply = replies.get(replies.size() - 1); + var lines = lastReply.body().split("\n"); + var errorPrefix = "@openjdk-bot Your merge request cannot be fulfilled at this time"; + if (lines.length > 1 && lines[1].startsWith(errorPrefix)) { + // Try again + pr.addComment("/integrate"); + } + // Other reply, potentially due to rebase issue, just + // wait for the labeler to add appropriate labels. + } + } } - break; } } @@ -375,7 +396,14 @@ public void run(Path scratchPath) { continue; } - log.info("Merging " + fromRepo.name() + ":" + fromBranch.name() + " to " + toBranch.name()); + // Checkout the branch to merge into + repo.checkout(toBranch, false); + var remoteBranch = new Branch(repo.upstreamFor(toBranch).orElseThrow(() -> + new IllegalStateException("Could not get remote branch name for " + toBranch.name()) + )); + repo.merge(remoteBranch); // should always be a fast-forward merge + + log.info("Trying to merge " + fromRepo.name() + ":" + fromBranch.name() + " to " + toBranch.name()); log.info("Fetching " + fromRepo.name() + ":" + fromBranch.name()); var fetchHead = repo.fetch(fromRepo.url(), fromBranch.name()); var head = repo.resolve(toBranch.name()).orElseThrow(() -> @@ -388,7 +416,7 @@ public void run(Path scratchPath) { var isAncestor = repo.isAncestor(head, fetchHead); - log.info("Trying to merge into " + toBranch.name()); + log.info("Merging into " + toBranch.name()); IOException error = null; try { repo.merge(fetchHead); @@ -410,7 +438,7 @@ public void run(Path scratchPath) { repo.abortMerge(); var fromRepoName = Path.of(fromRepo.webUrl().getPath()).getFileName(); - var branchDesc = fromRepoName + "/" + fromBranch.name() + "->" + toBranch.name(); + var branchDesc = Integer.toString(prs.size() + 1); repo.push(fetchHead, fork.url(), branchDesc, true); log.info("Creating pull request to alert"); @@ -427,7 +455,7 @@ public void run(Path scratchPath) { message.add("Hi all,"); message.add(""); - message.add("this is an _automatically_ generated merge request to notify you that there " + + message.add("this is an _automatically_ generated pull request to notify you that there " + are + " " + numCommits + " commit" + s + " from the branch `" + fromDesc + "`" + "that can **not** be merged into the branch `" + toBranch.name() + "`:"); @@ -445,30 +473,38 @@ public void run(Path scratchPath) { } message.add(""); + var project = JCheckConfiguration.from(repo, head).map(conf -> conf.general().project()); + if (project.isPresent()) { + message.add("All Committers in this [project](https://openjdk.java.net/census#" + project + ") " + + "have access to my [personal fork](" + fork.nonTransformedWebUrl() + ") and can " + + "therefore help resolve these merge conflicts (you may want to coordinate " + + "who should do this)."); + } else { + message.add("All users with access to my [personal fork](" + fork.nonTransformedWebUrl() + ") " + + "can help resolve these merge conflicts " + + "(you may want to coordinate who should do this)."); + } message.add("The following paragraphs will give an example on how to solve these " + - "merge conflicts and create a pull request to integrate them. If you are " + - "using a workflow different from the one below, feel free to use that instead. " + - "It is important that the title of the pull request you create is " + - "`Merge " + fromDesc + "`, otherwise the bots will _not_ understand that the " + - "pull request you create is a \"merge style\" pull request."); - message.add(""); - var localBranchName = "merge-" + fromDesc.replace(":", "-") + "-into-" + toBranch.name() + "-" + commits.get(0).hash().abbreviate(); + "merge conflicts and push the resulting merge commit to this pull request."); message.add("The below commands should be run in a local clone of your " + "[personal fork](https://wiki.openjdk.java.net/display/skara#Skara-Personalforks) " + - "of the [" + target.name() + "](" + target.nonTransformedWebUrl() + ") repository. " + - "These commands will allow you to view and resolve the merge conflicts. Note that " + - "the name of the local branch in the commands, " + - "`" + localBranchName + "`" + - ", is just an example, feel free to give the local branch any name you prefer."); + "of the [" + target.name() + "](" + target.nonTransformedWebUrl() + ") repository."); message.add(""); + var localBranchName = "openjdk-bot-" + branchDesc; message.add("```bash"); + message.add("# Ensure target branch is up to date"); message.add("$ git checkout " + toBranch.name()); message.add("$ git pull " + target.nonTransformedWebUrl() + " " + toBranch.name()); - message.add("$ git checkout -b " + localBranchName); - message.add("$ git pull " + fromRepo.nonTransformedWebUrl() + " " + fromBranch.name()); + message.add(""); + message.add("# Fetch and checkout the branch for this pull request"); + message.add("$ git fetch " + fork.nonTransformedWebUrl() + " +" + branchDesc + ":" + localBranchName); + message.add("$ git checkout " + localBranchName); + message.add(""); + message.add("# Merge the target branch"); + message.add("$ git merge " + toBranch.name()); message.add("```"); message.add(""); - message.add("When you have resolved the conflicts resulting from the `git pull` command " + + message.add("When you have resolved the conflicts resulting from the `git merge` command " + "above, run the following commands to create a merge commit:"); message.add(""); message.add("```bash"); @@ -476,20 +512,15 @@ public void run(Path scratchPath) { message.add("$ git commit -m 'Merge " + fromDesc + "'"); message.add("```"); message.add(""); - message.add("The commit message does not have to be `Merge " + fromDesc + "`, " + - "but it is convenient for when you will create a pull request. Many tools " + - "will by default use the commit message of the most recent commit on a branch " + - "as the title for a pull request from that branch. This means that if you use " + - "the commit message `Merge " + fromDesc + "` as the commit message then the " + - "title of the pull request will (depending to tools used to create the " + - "pull request) be of a format that the bots expect."); message.add(""); - message.add("Proceed to [publish the local branch](https://wiki.openjdk.java.net/display/SKARA/FAQ#FAQ-HowdoIpushalocalbranchtoaremoterepository?) " + - "and [create a pull request](https://wiki.openjdk.java.net/display/SKARA/FAQ#FAQ-HowdoIcreateapullrequest?) " + - "towards the `" + toBranch.name() + "` branch in the " + - "[" + target.name() + "](" + target.webUrl() + ") repository. The resulting pull " + - "request can then integrated as usual once it has passed all required " + - "pre-integration checks."); + message.add("When you have created the merge commit, run the following command to push the merge commit " + + "to this pull request:"); + message.add(""); + message.add("```bash"); + message.add("$ git push " + fork.nonTransformedWebUrl() + " " + localBranchName + ":" + branchDesc); + message.add("```"); + message.add(""); + message.add("_Note_: if you are using SSH to push commits to GitHub, then change the URL in the above `git push` command accordingly."); message.add(""); message.add("Thanks,"); message.add("J. Duke"); diff --git a/bots/merge/src/test/java/org/openjdk/skara/bots/merge/MergeBotTests.java b/bots/merge/src/test/java/org/openjdk/skara/bots/merge/MergeBotTests.java index 624fc4f29..03444e8ea 100644 --- a/bots/merge/src/test/java/org/openjdk/skara/bots/merge/MergeBotTests.java +++ b/bots/merge/src/test/java/org/openjdk/skara/bots/merge/MergeBotTests.java @@ -44,7 +44,7 @@ class MergeBotTests { @Test void mergeMasterBranch(TestInfo testInfo) throws IOException { - try (var temp = new TemporaryDirectory(false)) { + try (var temp = new TemporaryDirectory()) { var host = TestHost.createNew(List.of(new HostUser(0, "duke", "J. Duke"))); var fromDir = temp.path().resolve("from.git"); @@ -119,7 +119,7 @@ void mergeMasterBranch(TestInfo testInfo) throws IOException { @Test void failingMergeTest(TestInfo testInfo) throws IOException { - try (var temp = new TemporaryDirectory(false)) { + try (var temp = new TemporaryDirectory()) { var host = TestHost.createNew(List.of(new HostUser(0, "duke", "J. Duke"))); var fromDir = temp.path().resolve("from.git"); @@ -183,14 +183,14 @@ void failingMergeTest(TestInfo testInfo) throws IOException { var pullRequests = toHostedRepo.pullRequests(); assertEquals(1, pullRequests.size()); var pr = pullRequests.get(0); - assertEquals("Cannot automatically merge test:master to master", pr.title()); + assertEquals("Merge test:master", pr.title()); assertTrue(pr.labels().contains("failed-auto-merge")); } } @Test void failingMergeShouldResultInOnlyOnePR(TestInfo testInfo) throws IOException { - try (var temp = new TemporaryDirectory(false)) { + try (var temp = new TemporaryDirectory()) { var host = TestHost.createNew(List.of(new HostUser(0, "duke", "J. Duke"))); var fromDir = temp.path().resolve("from.git"); @@ -255,13 +255,13 @@ void failingMergeShouldResultInOnlyOnePR(TestInfo testInfo) throws IOException { var pullRequests = toHostedRepo.pullRequests(); assertEquals(1, pullRequests.size()); var pr = pullRequests.get(0); - assertEquals("Cannot automatically merge test:master to master", pr.title()); + assertEquals("Merge test:master", pr.title()); } } @Test - void resolvedMergeConflictShouldResultInClosedPR(TestInfo testInfo) throws IOException { - try (var temp = new TemporaryDirectory(false)) { + void resolvedMergeConflictShouldResultInIntegrateCommand(TestInfo testInfo) throws IOException { + try (var temp = new TemporaryDirectory()) { var host = TestHost.createNew(List.of(new HostUser(0, "duke", "J. Duke"))); var fromDir = temp.path().resolve("from.git"); @@ -326,115 +326,31 @@ void resolvedMergeConflictShouldResultInClosedPR(TestInfo testInfo) throws IOExc var pullRequests = toHostedRepo.pullRequests(); assertEquals(1, pullRequests.size()); var pr = pullRequests.get(0); - assertEquals("Cannot automatically merge test:master to master", pr.title()); - - var fetchHead = toLocalRepo.fetch(fromHostedRepo.webUrl(), "master"); - toLocalRepo.merge(fetchHead, "ours"); - toLocalRepo.commit("Merge", "duke", "duke@openjdk.org", now); - - toCommits = toLocalRepo.commits().asList(); - assertEquals(4, toCommits.size()); + assertEquals("Merge test:master", pr.title()); + assertTrue(pr.labels().contains("failed-auto-merge")); + assertTrue(forkLocalRepo.branches().contains(new Branch("master"))); + assertTrue(forkLocalRepo.branches().contains(new Branch("1"))); + // Bot should do nothing as long as PR is presnt TestBotRunner.runPeriodicItems(bot); pullRequests = toHostedRepo.pullRequests(); - assertEquals(0, pullRequests.size()); - } - } - - @Test - void resolvedMergeConflictAndThenNewConflict(TestInfo testInfo) throws IOException { - try (var temp = new TemporaryDirectory(false)) { - var host = TestHost.createNew(List.of(new HostUser(0, "duke", "J. Duke"))); - - var fromDir = temp.path().resolve("from.git"); - var fromLocalRepo = Repository.init(fromDir, VCS.GIT); - var fromHostedRepo = new TestHostedRepository(host, "test", fromLocalRepo); - - var toDir = temp.path().resolve("to.git"); - var toLocalRepo = Repository.init(toDir, VCS.GIT); - var toGitConfig = toDir.resolve(".git").resolve("config"); - Files.write(toGitConfig, List.of("[receive]", "denyCurrentBranch = ignore"), - StandardOpenOption.APPEND); - var toHostedRepo = new TestHostedRepository(host, "test-mirror", toLocalRepo); - - var forkDir = temp.path().resolve("fork.git"); - var forkLocalRepo = Repository.init(forkDir, VCS.GIT); - var forkGitConfig = forkDir.resolve(".git").resolve("config"); - Files.write(forkGitConfig, List.of("[receive]", "denyCurrentBranch = ignore"), - StandardOpenOption.APPEND); - var toFork = new TestHostedRepository(host, "test-mirror-fork", forkLocalRepo); - - var now = ZonedDateTime.now(); - var fromFileA = fromDir.resolve("a.txt"); - Files.writeString(fromFileA, "Hello A\n"); - fromLocalRepo.add(fromFileA); - var fromHashA = fromLocalRepo.commit("Adding a.txt", "duke", "duke@openjdk.org", now); - var fromCommits = fromLocalRepo.commits().asList(); - assertEquals(1, fromCommits.size()); - assertEquals(fromHashA, fromCommits.get(0).hash()); - - var toFileA = toDir.resolve("a.txt"); - Files.writeString(toFileA, "Hello A\n"); - toLocalRepo.add(toFileA); - var toHashA = toLocalRepo.commit("Adding a.txt", "duke", "duke@openjdk.org", now); - var toCommits = toLocalRepo.commits().asList(); - assertEquals(1, toCommits.size()); - assertEquals(toHashA, toCommits.get(0).hash()); - assertEquals(fromHashA, toHashA); - - var fromFileB = fromDir.resolve("b.txt"); - Files.writeString(fromFileB, "Hello B1\n"); - fromLocalRepo.add(fromFileB); - var fromHashB = fromLocalRepo.commit("Adding b1.txt", "duke", "duke@openjdk.org", now); - - var toFileB = toDir.resolve("b.txt"); - Files.writeString(toFileB, "Hello B2\n"); - toLocalRepo.add(toFileB); - var toHashB = toLocalRepo.commit("Adding b2.txt", "duke", "duke@openjdk.org", now); - - var storage = temp.path().resolve("storage"); - var master = new Branch("master"); - var specs = List.of(new MergeBot.Spec(fromHostedRepo, master, master)); - var bot = new MergeBot(storage, toHostedRepo, toFork, specs); - TestBotRunner.runPeriodicItems(bot); - TestBotRunner.runPeriodicItems(bot); - - toCommits = toLocalRepo.commits().asList(); - assertEquals(2, toCommits.size()); - var toHashes = toCommits.stream().map(Commit::hash).collect(Collectors.toSet()); - assertTrue(toHashes.contains(toHashA)); - assertTrue(toHashes.contains(toHashB)); - - var pullRequests = toHostedRepo.pullRequests(); assertEquals(1, pullRequests.size()); - var pr = pullRequests.get(0); - assertEquals("Cannot automatically merge test:master to master", pr.title()); - - var fetchHead = toLocalRepo.fetch(fromHostedRepo.webUrl(), "master"); - toLocalRepo.merge(fetchHead, "ours"); - toLocalRepo.commit("Merge", "duke", "duke@openjdk.org", now); - - toCommits = toLocalRepo.commits().asList(); - assertEquals(4, toCommits.size()); + pr = pullRequests.get(0); + // Simulate that the merge-conflict has been resolved by adding the "ready" label + pr.addLabel("ready"); TestBotRunner.runPeriodicItems(bot); pullRequests = toHostedRepo.pullRequests(); - assertEquals(0, pullRequests.size()); - - var fromFileC = fromDir.resolve("c.txt"); - Files.writeString(fromFileC, "Hello C1\n"); - fromLocalRepo.add(fromFileC); - fromLocalRepo.commit("Adding c1", "duke", "duke@openjdk.org", now); + assertEquals(1, pullRequests.size()); - var toFileC = toDir.resolve("c.txt"); - Files.writeString(toFileC, "Hello C2\n"); - toLocalRepo.add(toFileC); - toLocalRepo.commit("Adding c2", "duke", "duke@openjdk.org", now); + pr = pullRequests.get(0); + var numComments = pr.comments().size(); + var lastComment = pr.comments().get(pr.comments().size() - 1); + assertEquals("/integrate", lastComment.body()); + // Running the bot again should not result in another comment TestBotRunner.runPeriodicItems(bot); - pullRequests = toHostedRepo.pullRequests(); - assertEquals(1, pullRequests.size()); - assertEquals("Cannot automatically merge test:master to master", pr.title()); + assertEquals(numComments, toHostedRepo.pullRequests().size()); } } @@ -457,7 +373,7 @@ public ZonedDateTime now() { @Test void testMergeHourly(TestInfo testInfo) throws IOException { - try (var temp = new TemporaryDirectory(false)) { + try (var temp = new TemporaryDirectory()) { var host = TestHost.createNew(List.of(new HostUser(0, "duke", "J. Duke"))); var fromDir = temp.path().resolve("from.git"); @@ -572,7 +488,7 @@ void testMergeHourly(TestInfo testInfo) throws IOException { @Test void testMergeDaily(TestInfo testInfo) throws IOException { - try (var temp = new TemporaryDirectory(false)) { + try (var temp = new TemporaryDirectory()) { var host = TestHost.createNew(List.of(new HostUser(0, "duke", "J. Duke"))); var fromDir = temp.path().resolve("from.git"); @@ -693,7 +609,7 @@ void testMergeDaily(TestInfo testInfo) throws IOException { @Test void testMergeWeekly(TestInfo testInfo) throws IOException { - try (var temp = new TemporaryDirectory(false)) { + try (var temp = new TemporaryDirectory()) { var host = TestHost.createNew(List.of(new HostUser(0, "duke", "J. Duke"))); var fromDir = temp.path().resolve("from.git"); @@ -814,7 +730,7 @@ void testMergeWeekly(TestInfo testInfo) throws IOException { @Test void testMergeMonthly(TestInfo testInfo) throws IOException { - try (var temp = new TemporaryDirectory(false)) { + try (var temp = new TemporaryDirectory()) { var host = TestHost.createNew(List.of(new HostUser(0, "duke", "J. Duke"))); var fromDir = temp.path().resolve("from.git"); @@ -935,7 +851,7 @@ void testMergeMonthly(TestInfo testInfo) throws IOException { @Test void testMergeYearly(TestInfo testInfo) throws IOException { - try (var temp = new TemporaryDirectory(false)) { + try (var temp = new TemporaryDirectory()) { var host = TestHost.createNew(List.of(new HostUser(0, "duke", "J. Duke"))); var fromDir = temp.path().resolve("from.git"); diff --git a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/WebrevStorage.java b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/WebrevStorage.java index fa636248f..9189bdca2 100644 --- a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/WebrevStorage.java +++ b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/WebrevStorage.java @@ -73,10 +73,9 @@ private void generate(PullRequest pr, Repository localRepository, Path folder, D var issue = Issue.fromString(pr.title()); if (issue.isPresent()) { - var files = localRepository.files(head, List.of(Path.of(".jcheck", "conf"))); - if (!files.isEmpty()) { - var conf = JCheckConfiguration.from(localRepository, head); - var project = conf.general().jbs() != null ? conf.general().jbs() : conf.general().project(); + var conf = JCheckConfiguration.from(localRepository, head); + if (!conf.isEmpty()) { + var project = conf.get().general().jbs() != null ? conf.get().general().jbs() : conf.get().general().project(); var id = issue.get().id(); IssueTracker issueTracker = null; try { diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/AdditionalConfiguration.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/AdditionalConfiguration.java index df6273ae3..a8bca6dee 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/AdditionalConfiguration.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/AdditionalConfiguration.java @@ -38,7 +38,7 @@ static List get(ReadOnlyRepository repository, Hash hash, HostUser botUs return ret; } - var currentConfiguration = JCheckConfiguration.from(repository, hash); + var currentConfiguration = JCheckConfiguration.from(repository, hash).orElseThrow(); var updatedLimits = ReviewersTracker.updatedRoleLimits(currentConfiguration, additionalReviewers.get().number(), additionalReviewers.get().role()); ret.add("[checks \"reviewers\"]"); 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 f80e6927a..c49038bf5 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 @@ -689,7 +689,9 @@ private void checkStatus() { newLabels.remove("ready"); } if (!rebasePossible) { - addOutdatedComment(comments); + if (!labels.contains("failed-auto-merge")) { + addOutdatedComment(comments); + } newLabels.add("merge-conflict"); } else { newLabels.remove("merge-conflict"); diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java index a28c2fed3..61732b7d4 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java @@ -134,21 +134,14 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst } // Finally check if the author is allowed to perform the actual push - if (!pr.title().startsWith("Merge")) { - if (!ProjectPermissions.mayCommit(censusInstance, pr.author())) { - reply.println(ReadyForSponsorTracker.addIntegrationMarker(pr.headHash())); - reply.println("Your change (at version " + pr.headHash() + ") is now ready to be sponsored by a Committer."); - if (!args.isBlank()) { - reply.println("Note that your sponsor will make the final decision onto which target hash to integrate."); - } - pr.addLabel("sponsor"); - return; - } - } else { - if (!ProjectPermissions.mayCommit(censusInstance, pr.author())) { - reply.println("Merges require Committer status."); - return; + if (!ProjectPermissions.mayCommit(censusInstance, pr.author())) { + reply.println(ReadyForSponsorTracker.addIntegrationMarker(pr.headHash())); + reply.println("Your change (at version " + pr.headHash() + ") is now ready to be sponsored by a Committer."); + if (!args.isBlank()) { + reply.println("Note that your sponsor will make the final decision onto which target hash to integrate."); } + pr.addLabel("sponsor"); + return; } // Rebase and push it! 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 158bac06b..845a895ca 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 @@ -77,7 +77,7 @@ static List filterActiveReviews(List allReviews) { return new ArrayList<>(reviewPerUser.values()); } - private String commitMessage(List activeReviews, Namespace namespace, boolean isMerge) throws IOException { + private String commitMessage(List activeReviews, Namespace namespace) throws IOException { var reviewers = activeReviews.stream() .filter(review -> !ignoreStaleReviews || review.hash().equals(headHash)) .filter(review -> review.verdict() == Review.Verdict.APPROVED) @@ -96,7 +96,7 @@ private String commitMessage(List activeReviews, Namespace namespace, bo var additionalIssues = SolvesTracker.currentSolved(pr.repository().forge().currentUser(), comments); var summary = Summary.summary(pr.repository().forge().currentUser(), comments); var issue = Issue.fromString(pr.title()); - var commitMessageBuilder = issue.map(CommitMessage::title).orElseGet(() -> CommitMessage.title(isMerge ? "Merge" : pr.title())); + var commitMessageBuilder = issue.map(CommitMessage::title).orElseGet(() -> CommitMessage.title(pr.title())); if (issue.isPresent()) { commitMessageBuilder.issues(additionalIssues); } @@ -134,11 +134,11 @@ private Hash commitSquashed(List activeReviews, Namespace namespace, Str committer = author; } - var commitMessage = commitMessage(activeReviews, namespace, false); + var commitMessage = commitMessage(activeReviews, namespace); return localRepo.commit(commitMessage, author.name(), author.email(), committer.name(), committer.email()); } - private Hash commitMerge(List activeReviews, Namespace namespace, String censusDomain) throws IOException, CommitFailure { + private Hash commitMerge(List activeReviews, Namespace namespace, String censusDomain, String sponsorId) 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); @@ -168,12 +168,19 @@ private Hash commitMerge(List activeReviews, Namespace namespace, String } var author = new Author(contributor.fullName().orElseThrow(), contributor.username() + "@" + censusDomain); - var commitMessage = commitMessage(activeReviews, namespace, true); + Author committer; + if (sponsorId != null) { + var sponsorContributor = namespace.get(sponsorId); + committer = new Author(sponsorContributor.fullName().orElseThrow(), sponsorContributor.username() + "@" + censusDomain); + } else { + committer = author; + } + var commitMessage = commitMessage(activeReviews, namespace); localRepo.checkout(commits.get(mergeCommitIndex).hash(), true); localRepo.squash(headHash); - return localRepo.amend(commitMessage, author.name(), author.email(), author.name(), author.email()); + return localRepo.amend(commitMessage, author.name(), author.email(), committer.name(), committer.email()); } Hash commit(Namespace namespace, String censusDomain, String sponsorId) throws IOException, CommitFailure { @@ -181,7 +188,7 @@ Hash commit(Namespace namespace, String censusDomain, String sponsorId) throws I if (!pr.title().startsWith("Merge")) { return commitSquashed(activeReviews, namespace, censusDomain, sponsorId); } else { - return commitMerge(activeReviews, namespace, censusDomain); + return commitMerge(activeReviews, namespace, censusDomain, sponsorId); } } @@ -223,6 +230,7 @@ Optional rebase(Hash commitHash, PrintWriter reply) { } catch (IOException e2) { throw new UncheckedIOException(e2); } + pr.addLabel("merge-conflict"); return Optional.empty(); } } else { 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 f2f89bee1..e3208c1d7 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 @@ -764,9 +764,9 @@ void invalidAuthor(TestInfo testInfo) throws IOException { pr.addComment("/integrate"); TestBotRunner.runPeriodicItems(mergeBot); - // The bot should reply with a failure message + // The bot should reply with a need for sponsor var error = pr.comments().stream() - .filter(comment -> comment.body().contains("Merges require Committer status")) + .filter(comment -> comment.body().contains("Afterwards, your sponsor types `/sponsor`")) .count(); assertEquals(1, error, () -> pr.comments().stream().map(Comment::body).collect(Collectors.joining("\n\n"))); } diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SponsorTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SponsorTests.java index 58fc7620a..2a66b2583 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SponsorTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SponsorTests.java @@ -25,6 +25,7 @@ import org.openjdk.skara.forge.Review; import org.openjdk.skara.test.*; import org.openjdk.skara.vcs.Repository; +import org.openjdk.skara.vcs.Branch; import org.junit.jupiter.api.*; @@ -612,4 +613,91 @@ void cannotRebase(TestInfo testInfo) throws IOException { assertEquals(1, error); } } + + @Test + void sponsorMergeCommit(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory(false)) { + var author = credentials.getHostedRepository(); + var integrator = credentials.getHostedRepository(); + var reviewer = credentials.getHostedRepository(); + + var reviewerId = reviewer.forge().currentUser().id(); + var censusBuilder = credentials.getCensusBuilder() + .addReviewer(reviewerId) + .addAuthor(author.forge().currentUser().id()); + var mergeBot = PullRequestBot.newBuilder().repo(integrator).censusRepo(censusBuilder.build()).build(); + + // Populate the projects repository + var localRepo = CheckableRepository.init(tempFolder.path().resolve("local.git"), author.repositoryType()); + var initialHash = localRepo.resolve("master").orElseThrow(); + assertFalse(CheckableRepository.hasBeenEdited(localRepo)); + var anotherFile = localRepo.root().resolve("ANOTHER_FILE.txt"); + Files.writeString(anotherFile, "A string\n"); + localRepo.add(anotherFile); + var masterHash = localRepo.commit("Another commit\n\nReviewed-by: " + reviewerId, "duke", "duke@openjdk.java.net"); + localRepo.push(masterHash, author.url(), "master", true); + + // Create a new branch, new commit and publish it + var editBranch = localRepo.branch(initialHash, "edit"); + localRepo.checkout(editBranch); + var editHash = CheckableRepository.appendAndCommit(localRepo); + localRepo.push(editHash, author.url(), "edit", true); + + // Prepare to merge edit into master + localRepo.checkout(new Branch("master")); + var editToMasterBranch = localRepo.branch(masterHash, "edit->master"); + localRepo.checkout(editToMasterBranch); + localRepo.merge(editHash); + var mergeHash = localRepo.commit("Merge edit", "duke", "duke@openjdk.java.net"); + localRepo.push(mergeHash, author.url(), "edit->master", true); + + + var pr = credentials.createPullRequest(author, "master", "edit->master", "Merge edit"); + + // Approve it as another user + var approvalPr = reviewer.pullRequest(pr.id()); + approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); + + // Let the bot see it + TestBotRunner.runPeriodicItems(mergeBot); + + // Issue a merge command without being a Committer + pr.addComment("/integrate"); + TestBotRunner.runPeriodicItems(mergeBot); + + //System.out.println(pr.comments()); + //for (var entry : pr.checks(pr.headHash()).entrySet()) { + // System.out.println(entry.getValue().summary().orElseThrow()); + //} + + // The bot should reply that a sponsor is required + var sponsor = pr.comments().stream() + .filter(comment -> comment.body().contains("sponsor")) + .filter(comment -> comment.body().contains("your change")) + .count(); + assertEquals(1, sponsor); + + // The bot should not have pushed the commit + var notPushed = pr.comments().stream() + .filter(comment -> comment.body().contains("Pushed as commit")) + .count(); + assertEquals(0, notPushed); + + // Reviewer now agrees to sponsor + var reviewerPr = reviewer.pullRequest(pr.id()); + reviewerPr.addComment("/sponsor"); + TestBotRunner.runPeriodicItems(mergeBot); + + // The bot should have pushed the commit + var pushed = pr.comments().stream() + .filter(comment -> comment.body().contains("Pushed as commit")) + .count(); + assertEquals(1, pushed); + + var targetRepo = Repository.clone(author.url(), tempFolder.path().resolve("target.git")); + var masterHead = targetRepo.lookup(new Branch("origin/master")).orElseThrow(); + assertEquals("Merge edit", masterHead.message().get(0)); + } + } } diff --git a/cli/src/main/java/org/openjdk/skara/cli/GitInfo.java b/cli/src/main/java/org/openjdk/skara/cli/GitInfo.java index bbad6f46b..bf8a11c40 100644 --- a/cli/src/main/java/org/openjdk/skara/cli/GitInfo.java +++ b/cli/src/main/java/org/openjdk/skara/cli/GitInfo.java @@ -63,7 +63,7 @@ private static boolean getSwitch(String name, Arguments arguments, ReadOnlyRepos } private static String jbsProject(ReadOnlyRepository repo, Hash hash) throws IOException { - var conf = JCheckConfiguration.from(repo, hash); + var conf = JCheckConfiguration.from(repo, hash).orElseThrow(); return conf.general().jbs().toUpperCase(); } diff --git a/cli/src/main/java/org/openjdk/skara/cli/pr/Utils.java b/cli/src/main/java/org/openjdk/skara/cli/pr/Utils.java index 02c6c7c46..5391fe2f0 100644 --- a/cli/src/main/java/org/openjdk/skara/cli/pr/Utils.java +++ b/cli/src/main/java/org/openjdk/skara/cli/pr/Utils.java @@ -231,7 +231,7 @@ static String jbsProjectFromJcheckConf(Repository repo, String targetBranch) thr new IOException("Could not resolve '" + targetBranch + "' branch") )); - return conf.general().jbs(); + return conf.get().general().jbs(); } static Optional getIssue(Commit commit, String project) throws IOException { diff --git a/issuetracker/src/main/java/org/openjdk/skara/issuetracker/Comment.java b/issuetracker/src/main/java/org/openjdk/skara/issuetracker/Comment.java index a3c615c36..966cc8870 100644 --- a/issuetracker/src/main/java/org/openjdk/skara/issuetracker/Comment.java +++ b/issuetracker/src/main/java/org/openjdk/skara/issuetracker/Comment.java @@ -82,4 +82,9 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hash(id, body, author, createdAt, updatedAt); } + + @Override + public String toString() { + return body; + } } diff --git a/jcheck/src/main/java/org/openjdk/skara/jcheck/JCheckConfiguration.java b/jcheck/src/main/java/org/openjdk/skara/jcheck/JCheckConfiguration.java index 3331e6d13..df2e304a7 100644 --- a/jcheck/src/main/java/org/openjdk/skara/jcheck/JCheckConfiguration.java +++ b/jcheck/src/main/java/org/openjdk/skara/jcheck/JCheckConfiguration.java @@ -157,15 +157,15 @@ public static JCheckConfiguration parse(List lines) { return new JCheckConfiguration(ini); } - public static JCheckConfiguration from(ReadOnlyRepository r, Hash h, Path p) throws IOException { - return parse(r.lines(p, h).orElse(Collections.emptyList())); + public static Optional from(ReadOnlyRepository r, Hash h, Path p) throws IOException { + return r.lines(p, h).map(lines -> parse(lines)); } - public static JCheckConfiguration from(ReadOnlyRepository r, Hash h) throws IOException { + public static Optional from(ReadOnlyRepository r, Hash h) throws IOException { return from(r, h, Path.of(".jcheck", "conf")); } - public static JCheckConfiguration from(ReadOnlyRepository r) throws IOException { + public static Optional from(ReadOnlyRepository r) throws IOException { var master = r.resolve("master") .orElseThrow(() -> new IOException("Cannot resolve 'master' branch")); return from(r, master, Path.of(".jcheck", "conf"));