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

304: Squash additional commits into merge commits for merge PRs #519

Closed
wants to merge 4 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
@@ -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) {
edvbld marked this conversation as resolved.
Show resolved Hide resolved
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);
edvbld marked this conversation as resolved.
Show resolved Hide resolved
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());
}
}