Skip to content
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.

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

Restrict merge PR commit titles if required by the jcheck configuration #949

Closed
wants to merge 3 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -32,6 +32,7 @@

import java.io.*;
import java.util.*;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

public class CheckablePullRequest {
@@ -53,10 +54,7 @@ public class CheckablePullRequest {
}
}

private String commitMessage(List<Review> activeReviews, Namespace namespace, boolean manualReviewers) throws IOException {
return commitMessage(activeReviews, namespace, manualReviewers, null);
}
private String commitMessage(List<Review> activeReviews, Namespace namespace, boolean manualReviewers, Hash original) throws IOException {
private String commitMessage(Hash head, List<Review> activeReviews, Namespace namespace, boolean manualReviewers, Hash original) throws IOException {
var eligibleReviews = activeReviews.stream()
.filter(review -> !ignoreStaleReviews || review.hash().equals(pr.headHash()))
.filter(review -> review.verdict() == Review.Verdict.APPROVED)
@@ -82,16 +80,39 @@ private String commitMessage(List<Review> activeReviews, Namespace namespace, bo

var additionalIssues = SolvesTracker.currentSolved(currentUser, comments);
var summary = Summary.summary(currentUser, comments);
var issue = Issue.fromStringRelaxed(pr.title());
var commitMessageBuilder = issue.map(CommitMessage::title).orElseGet(() -> CommitMessage.title(pr.title()));
if (issue.isPresent()) {
commitMessageBuilder.issues(additionalIssues);
CommitMessageBuilder commitMessageBuilder;
if (PullRequestUtils.isMerge(pr)) {
var conf = JCheckConfiguration.from(localRepo, head);
var title = pr.title();
if (conf.isPresent() && !conf.get().checks().enabled(List.of(new MergeMessageCheck())).isEmpty()) {
var mergeConf = conf.get().checks().merge();
var pattern = Pattern.compile(mergeConf.message());
while (true) {
var matcher = pattern.matcher(title);
if (matcher.matches()) {
break;
} else {
if (title.length() > 1) {
title = title.substring(0, title.length() - 1);
} else {
throw new RuntimeException("Unable to make merge PR title '" + pr.title() + "' conform to '" + mergeConf.message() + "'");
}
}
}
}
commitMessageBuilder = CommitMessage.title(title);
} else {
var issue = Issue.fromStringRelaxed(pr.title());
commitMessageBuilder = issue.map(CommitMessage::title).orElseGet(() -> CommitMessage.title(pr.title()));
if (issue.isPresent()) {
commitMessageBuilder.issues(additionalIssues);
}
if (original != null) {
commitMessageBuilder.original(original);
}
}
commitMessageBuilder.contributors(additionalContributors)
.reviewers(new ArrayList<>(reviewers));
if (original != null) {
commitMessageBuilder.original(original);
}
summary.ifPresent(commitMessageBuilder::summary);

return String.join("\n", commitMessageBuilder.format(CommitMessageFormatters.v1));
@@ -135,14 +156,14 @@ Hash commit(Hash finalHead, Namespace namespace, String censusDomain, String spo
}

var activeReviews = filterActiveReviews(pr.reviews());
var commitMessage = commitMessage(activeReviews, namespace, false, original);
var commitMessage = commitMessage(finalHead, activeReviews, namespace, false, original);
return PullRequestUtils.createCommit(pr, localRepo, finalHead, author, committer, commitMessage);
}

Hash amendManualReviewers(Hash commit, Namespace namespace, Hash original) throws IOException {
var activeReviews = filterActiveReviews(pr.reviews());
var originalCommitMessage = commitMessage(activeReviews, namespace, false, original);
var amendedCommitMessage = commitMessage(activeReviews, namespace, true, original);
var originalCommitMessage = commitMessage(commit, activeReviews, namespace, false, original);
var amendedCommitMessage = commitMessage(commit, activeReviews, namespace, true, original);

if (originalCommitMessage.equals(amendedCommitMessage)) {
return commit;
@@ -118,6 +118,90 @@ void branchMerge(TestInfo testInfo) throws IOException {

// Author and committer should updated in the merge commit
var headCommit = pushedRepo.commits(headHash.hex() + "^.." + headHash.hex()).asList().get(0);
assertEquals("Merge " + author.name() + ":other", headCommit.message().get(0));
assertEquals("Generated Committer 1", headCommit.author().name());
assertEquals("integrationcommitter1@openjdk.java.net", headCommit.author().email());
assertEquals("Generated Committer 1", headCommit.committer().name());
assertEquals("integrationcommitter1@openjdk.java.net", headCommit.committer().email());
}
}

@Test
void branchMergeRestrictedMessage(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(),
Path.of("appendable.txt"), Set.of("merge"), "1.0");
var masterHash = localRepo.resolve("master").orElseThrow();
assertFalse(CheckableRepository.hasBeenEdited(localRepo));
localRepo.push(masterHash, author.url(), "master", true);

// Make more changes in another branch
var otherHash1 = CheckableRepository.appendAndCommit(localRepo, "First change in other",
"First other\n\nReviewed-by: integrationreviewer2");
localRepo.push(otherHash1, author.url(), "other", true);
var otherHash2 = CheckableRepository.appendAndCommit(localRepo, "Second change in other",
"Second other\n\nReviewed-by: integrationreviewer2");
localRepo.push(otherHash2, author.url(), "other");

// 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.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");

// 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 an ok message
assertLastCommentContains(pr, "Pushed as commit");

// The change should now be present on the master branch
var pushedRepoFolder = tempFolder.path().resolve("pushedrepo");
var pushedRepo = Repository.materialize(pushedRepoFolder, author.url(), "master");
assertTrue(CheckableRepository.hasBeenEdited(pushedRepo));

// The commits from the "other" branch should be preserved and not squashed (but not the merge commit)
var headHash = pushedRepo.resolve("HEAD").orElseThrow();
Set<Hash> commits;
try (var tempCommits = pushedRepo.commits(masterHash.hex() + ".." + headHash.hex())) {
commits = tempCommits.stream()
.map(Commit::hash)
.collect(Collectors.toSet());
}
assertTrue(commits.contains(otherHash1));
assertTrue(commits.contains(otherHash2));
assertFalse(commits.contains(mergeHash));

// The commit message should be just "Merge"
var headCommit = pushedRepo.commits(headHash.hex() + "^.." + headHash.hex()).asList().get(0);
assertEquals("Merge", headCommit.message().get(0));
assertEquals("Generated Committer 1", headCommit.author().name());
assertEquals("integrationcommitter1@openjdk.java.net", headCommit.author().email());
assertEquals("Generated Committer 1", headCommit.committer().name());
@@ -39,7 +39,7 @@ Iterator<Issue> check(Commit commit, CommitMessage message, JCheckConfiguration
}

var pattern = Pattern.compile(conf.checks().merge().message());
if (commit.message().size() != 1 || !pattern.matcher(commit.message().get(0)).matches()) {
if (!message.issues().isEmpty() || message.original().isPresent() || !pattern.matcher(message.title()).matches()) {
var metadata = CommitIssue.metadata(commit, message, conf, this);
log.finer("issue: wrong merge message");
return iterator(new MergeMessageIssue(pattern.toString(), metadata));
@@ -22,21 +22,15 @@
*/
package org.openjdk.skara.jcheck;

import org.openjdk.skara.vcs.Author;
import org.openjdk.skara.vcs.Commit;
import org.openjdk.skara.vcs.CommitMetadata;
import org.openjdk.skara.vcs.Hash;
import org.openjdk.skara.vcs.openjdk.CommitMessage;
import org.openjdk.skara.vcs.openjdk.CommitMessageParsers;

import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.*;
import org.openjdk.skara.vcs.*;
import org.openjdk.skara.vcs.openjdk.*;

import java.util.Iterator;
import java.util.List;
import java.util.ArrayList;
import java.time.ZonedDateTime;
import java.io.IOException;
import java.time.ZonedDateTime;
import java.util.*;

import static org.junit.jupiter.api.Assertions.*;

class MergeMessageCheckTests {
private static final List<String> CONFIGURATION = List.of(
@@ -94,14 +88,13 @@ void incorrectMessageShouldFail() throws IOException {
}

@Test
void multiLineMessageShouldFail() throws IOException {
void multiLineMessageShouldWork() throws IOException {
var commit = commit(List.of("Merge", "", "This is a summary"));
var message = message(commit);
var check = new MergeMessageCheck();
var issues = toList(check.check(commit, message, conf(), null));

assertEquals(1, issues.size());
assertTrue(issues.get(0) instanceof MergeMessageIssue);
assertEquals(List.of(), issues);
}

@Test