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

375: Improve Merge PR RFR email #591

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
@@ -463,14 +463,21 @@ void invalidMergeCommit(TestInfo testInfo) throws IOException {
pr.addComment("/integrate");
TestBotRunner.runPeriodicItems(mergeBot);

// 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")));
// The bot will create a proper merge commit
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")));

var check = pr.checks(mergeHash).get("jcheck");
assertEquals("- The merge commit must have a commit on the target branch as one of its parents.", check.summary().orElseThrow());
// The change should now be present with correct parents on the master branch
var pushedRepoFolder = tempFolder.path().resolve("pushedrepo");
var pushedRepo = Repository.materialize(pushedRepoFolder, author.url(), "master");
assertTrue(CheckableRepository.hasBeenEdited(pushedRepo));

var head = pushedRepo.commitMetadata("HEAD^!").get(0);
assertEquals(2, head.parents().size());
assertEquals(masterHash, head.parents().get(0));
assertEquals(otherHash, head.parents().get(1));
}
}

@@ -716,7 +723,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("- A merge PR must contain a merge commit as well as at least one other commit from the merge source.", check.summary().orElseThrow());
assertEquals("- A merge PR must contain at least one commit from the source branch that is not already present in the target.", check.summary().orElseThrow());
}
}

@@ -848,7 +855,7 @@ void unrelatedHistory(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 commit must have a commit on the target branch as one of its parents.", check.summary().orElseThrow());
assertEquals("- The target and the source branches do not share common history - cannot merge them.", check.summary().orElseThrow());
}
}

@@ -29,6 +29,7 @@
import java.time.ZonedDateTime;
import java.util.*;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

public class PullRequestUtils {
private static Hash commitSquashed(PullRequest pr, Repository localRepo, Hash finalHead, Author author, Author committer, String commitMessage) throws IOException {
@@ -73,9 +74,9 @@ private static Optional<MergeSource> mergeSource(PullRequest pr, Repository loca
return Optional.of(new MergeSource(repoMatcher.group(1), repoMatcher.group(2)));
}

private static CommitMetadata findSourceMergeCommit(PullRequest pr, Repository localRepo, List<CommitMetadata> commits) throws IOException, CommitFailure {
if (commits.size() < 2) {
throw new CommitFailure("A merge PR must contain at least two commits that are not already present in the target.");
private static Hash findSourceHash(PullRequest pr, Repository localRepo, List<CommitMetadata> commits) throws IOException, CommitFailure {
if (commits.size() < 1) {
throw new CommitFailure("A merge PR must contain at least one commit that is not already present in the target.");
}

var source = mergeSource(pr, localRepo);
@@ -85,13 +86,13 @@ private static CommitMetadata findSourceMergeCommit(PullRequest pr, Repository l
}

// Fetch the source
Hash sourceHash;
Hash sourceHead;
try {
var mergeSourceRepo = pr.repository().forge().repository(source.get().repositoryName).orElseThrow(() ->
new RuntimeException("Could not find repository " + source.get().repositoryName)
);
try {
sourceHash = localRepo.fetch(mergeSourceRepo.url(), source.get().branchName, false);
sourceHead = localRepo.fetch(mergeSourceRepo.url(), source.get().branchName, false);
} catch (IOException e) {
throw new CommitFailure("Could not fetch branch `" + source.get().branchName + "` from project `" +
source.get().repositoryName + "` - check that they are correct.");
@@ -101,53 +102,32 @@ private static CommitMetadata findSourceMergeCommit(PullRequest pr, Repository l
source.get().repositoryName + "` - check that it is correct.");
}


// Find the first merge commit with a parent that is an ancestor of the source
int mergeCommitIndex = commits.size();
for (int i = 0; i < commits.size() - 1; ++i) {
if (commits.get(i).isMerge()) {
boolean isSourceMerge = false;
for (int j = 0; j < commits.get(i).parents().size(); ++j) {
if (localRepo.isAncestor(commits.get(i).parents().get(j), sourceHash)) {
isSourceMerge = true;
}
}
if (isSourceMerge) {
mergeCommitIndex = i;
break;
}
}
// Ensure that the source and the target are related
try {
localRepo.mergeBase(pr.targetHash(), sourceHead);
} catch (IOException e) {
throw new CommitFailure("The target and the source branches do not share common history - cannot merge them.");
}
if (mergeCommitIndex >= commits.size() - 1) {
throw new CommitFailure("A merge PR must contain a merge commit as well as at least one other commit from the merge source.");

// Find the most recent commit from the merge source not present in the target
var sourceHash = localRepo.mergeBase(pr.headHash(), sourceHead);
var commitHashes = commits.stream()
.map(CommitMetadata::hash)
.collect(Collectors.toSet());
if (!commitHashes.contains(sourceHash)) {
throw new CommitFailure("A merge PR must contain at least one commit from the source branch that is not already present in the target.");
}

return commits.get(mergeCommitIndex);
return sourceHash;
}

private static Hash commitMerge(PullRequest pr, Repository localRepo, Hash finalHead, Author author, Author committer, String commitMessage) throws IOException, CommitFailure {
var commits = localRepo.commitMetadata(baseHash(pr, localRepo), finalHead);
var mergeCommit = findSourceMergeCommit(pr, localRepo, commits);

// Find the parent which is on the target branch - we will replace it with the target hash (if there were no merge conflicts)
Hash firstParent = null;
var finalParents = new ArrayList<Hash>();
for (int i = 0; i < mergeCommit.parents().size(); ++i) {
if (localRepo.isAncestor(mergeCommit.parents().get(i), pr.targetHash())) {
if (firstParent == null) {
firstParent = localRepo.mergeBase(pr.targetHash(), finalHead);
continue;
}
}
finalParents.add(mergeCommit.parents().get(i));
}
if (firstParent == null) {
throw new CommitFailure("The merge commit must have a commit on the target branch as one of its parents.");
}
finalParents.add(0, firstParent);
var sourceHash = findSourceHash(pr, localRepo, commits);
var parents = List.of(localRepo.mergeBase(pr.targetHash(), finalHead), sourceHash);

return localRepo.commit(commitMessage, author.name(), author.email(), ZonedDateTime.now(),
committer.name(), committer.email(), ZonedDateTime.now(), finalParents, localRepo.tree(finalHead));
committer.name(), committer.email(), ZonedDateTime.now(), parents, localRepo.tree(finalHead));
}

public static Repository materialize(HostedRepositoryPool hostedRepositoryPool, PullRequest pr, Path path) throws IOException {