Skip to content
Permalink
Browse files
304: Squash additional commits into merge commits for merge PRs
Reviewed-by: ehelin
  • Loading branch information
rwestberg committed Mar 20, 2020
1 parent ac71433 commit 92d7d97ae8e0b065c80bd946ce941060c5f15c82
Showing 3 changed files with 124 additions and 7 deletions.
@@ -164,8 +164,15 @@ private List<String> 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 {
if (!commits.get(0).isMerge()) {
ret.add("The top commit must be a merge commit.");
// Find the last 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;
}
}
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();
@@ -176,8 +183,8 @@ private List<String> botSpecificChecks() throws IOException {
);
try {
var sourceHash = prInstance.localRepo().fetch(mergeSourceRepo.url(), source.get().branchName);
if (!prInstance.localRepo().isAncestor(commits.get(1).hash(), sourceHash)) {
ret.add("The merge contains commits that are not ancestors of the source");
if (!prInstance.localRepo().isAncestor(commits.get(mergeCommitIndex + 1).hash(), sourceHash)) {
ret.add("The merge contains commits that are not ancestors of the source.");
}
} catch (IOException e) {
ret.add("Could not fetch branch `" + source.get().branchName + "` from project `" +
@@ -133,16 +133,26 @@ private Hash commitSquashed(List<Review> activeReviews, Namespace namespace, Str
}

private Hash commitMerge(List<Review> activeReviews, Namespace namespace, String censusDomain) throws IOException {
localRepo.checkout(headHash, true);
// 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();
int mergeCommitIndex = commits.size();
for (int i = 0; i < commits.size() - 1; ++i) {
if (commits.get(i).isMerge()) {
mergeCommitIndex = i;
}
}

var contributor = namespace.get(pr.author().id());
if (contributor == null) {
throw new RuntimeException("Merges can only be performed by Committers");
}

var author = new Author(contributor.fullName().orElseThrow(), contributor.username() + "@" + censusDomain);

var commitMessage = commitMessage(activeReviews, namespace, true);

localRepo.checkout(commits.get(mergeCommitIndex).hash(), true);
localRepo.squash(headHash);

return localRepo.amend(commitMessage, author.name(), author.email(), author.name(), author.email());
}

@@ -296,6 +296,106 @@ void branchMergeRebase(TestInfo testInfo) throws IOException {
}
}

@Test
void branchMergeAdditionalCommits(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 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);
var mergeHash = localRepo.commit("Our own 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 something new to master
localRepo.checkout(updatedMaster, true);
var newMaster = Files.writeString(localRepo.root().resolve("newmaster.txt"), "New on master", StandardCharsets.UTF_8);
localRepo.add(newMaster);
var newMasterHash = localRepo.commit("New commit on master", "some", "some@one");
localRepo.push(newMasterHash, author.url(), "master");

// Let the bot notice
TestBotRunner.runPeriodicItems(mergeBot);

// Add another commit on top of the merge commit
localRepo.checkout(mergeHash, true);
var extraHash = CheckableRepository.appendAndCommit(localRepo, "Fixing up stuff after merge");
localRepo.push(extraHash, author.url(), "edit");

// Let the bot notice again
TestBotRunner.runPeriodicItems(mergeBot);

// Push it
pr.addComment("/integrate");
TestBotRunner.runPeriodicItems(mergeBot);

// The bot should reply with an ok message
var pushed = pr.comments().stream()
.filter(comment -> comment.body().contains("Pushed as commit"))
.count();
assertEquals(1, pushed, () -> pr.comments().stream().map(Comment::body).collect(Collectors.joining("\n\n")));

// 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();
String commits;
try (var tempCommits = pushedRepo.commits(masterHash.hex() + ".." + headHash.hex())) {
commits = tempCommits.stream()
.map(c -> c.hash().hex() + ":" + c.message().get(0))
.collect(Collectors.joining(","));
}
assertTrue(commits.contains(otherHash1.hex() + ":First other"));
assertTrue(commits.contains(otherHash2.hex() + ":Second other"));
assertFalse(commits.contains("Our own merge commit"));

// Author and committer should updated in the merge commit
var headCommit = pushedRepo.commits(headHash.hex() + "^.." + headHash.hex()).asList().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 invalidSourceRepo(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
@@ -471,7 +571,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 not ancestors of the source.", check.summary().orElseThrow());
}
}

0 comments on commit 92d7d97

Please sign in to comment.