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

Proper selection of merge commit to squash commits onto for merge style PRs #552

Closed
wants to merge 1 commit 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
@@ -577,10 +577,16 @@ private void checkStatus() {
// Post check in-progress
log.info("Starting to run jcheck on PR head");
pr.createCheck(checkBuilder.build());
var localHash = prInstance.commit(censusInstance.namespace(), censusDomain, null);
List<String> additionalErrors = List.of();
Hash localHash;
try {
localHash = prInstance.commit(censusInstance.namespace(), censusDomain, null);
} catch (CommitFailure e) {
additionalErrors = List.of("It was not possible to create a commit for the changes in this PR: " + e.getMessage());
localHash = prInstance.baseHash();
}
boolean rebasePossible = true;
PullRequestCheckIssueVisitor visitor = prInstance.createVisitor(localHash, censusInstance);
List<String> additionalErrors;
if (!localHash.equals(prInstance.baseHash())) {
// Try to rebase
var ignored = new PrintWriter(new StringWriter());
@@ -595,9 +601,10 @@ private void checkStatus() {
var additionalConfiguration = AdditionalConfiguration.get(prInstance.localRepo(), localHash, pr.repository().forge().currentUser(), comments);
prInstance.executeChecks(localHash, censusInstance, visitor, additionalConfiguration);
additionalErrors = botSpecificChecks();
}
else {
additionalErrors = List.of("This PR contains no changes");
} else {
if (additionalErrors.isEmpty()) {
additionalErrors = List.of("This PR contains no changes");
}
}
updateCheckBuilder(checkBuilder, visitor, additionalErrors);
updateReadyForReview(visitor, additionalErrors);
@@ -35,6 +35,12 @@
import java.util.*;
import java.util.stream.Collectors;

class CommitFailure extends Exception {
CommitFailure(String reason) {
super(reason);
}
}

class PullRequestInstance {
private final PullRequest pr;
private final Repository localRepo;
@@ -132,19 +138,33 @@ private Hash commitSquashed(List<Review> activeReviews, Namespace namespace, Str
return localRepo.commit(commitMessage, author.name(), author.email(), committer.name(), committer.email());
}

private Hash commitMerge(List<Review> activeReviews, Namespace namespace, String censusDomain) throws IOException {
// 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();
private Hash commitMerge(List<Review> activeReviews, Namespace namespace, String censusDomain) 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);
int mergeCommitIndex = commits.size();
for (int i = 0; i < commits.size() - 1; ++i) {
if (commits.get(i).isMerge()) {
mergeCommitIndex = i;
boolean isSourceMerge = false;
for (int j = 1; j < commits.get(i).parents().size(); ++j) {
if (!localRepo.isAncestor(baseHash, commits.get(i).parents().get(j))) {
isSourceMerge = true;
}
}
if (isSourceMerge) {
mergeCommitIndex = i;
break;
}
}
}

if (mergeCommitIndex == commits.size()) {
throw new CommitFailure("No merge commit containing commits from another branch than the target was found");
}

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

var author = new Author(contributor.fullName().orElseThrow(), contributor.username() + "@" + censusDomain);
@@ -156,7 +176,7 @@ private Hash commitMerge(List<Review> activeReviews, Namespace namespace, String
return localRepo.amend(commitMessage, author.name(), author.email(), author.name(), author.email());
}

Hash commit(Namespace namespace, String censusDomain, String sponsorId) throws IOException {
Hash commit(Namespace namespace, String censusDomain, String sponsorId) throws IOException, CommitFailure {
var activeReviews = filterActiveReviews(pr.reviews());
if (!pr.title().startsWith("Merge")) {
return commitSquashed(activeReviews, namespace, censusDomain, sponsorId);
@@ -165,16 +185,21 @@ Hash commit(Namespace namespace, String censusDomain, String sponsorId) throws I
}
}

List<Commit> divergingCommits() {
List<CommitMetadata> divergingCommits() {
return divergingCommits(headHash);
}

private List<CommitMetadata> divergingCommits(Hash commitHash) {
try {
return localRepo.commits(baseHash + ".." + targetHash).asList();
var updatedBase = localRepo.mergeBase(targetHash, commitHash);
return localRepo.commitMetadata(updatedBase, targetHash);
} catch (IOException e) {
throw new RuntimeException(e);
}
}

Optional<Hash> rebase(Hash commitHash, PrintWriter reply) {
var divergingCommits = divergingCommits();
var divergingCommits = divergingCommits(commitHash);
if (divergingCommits.size() > 0) {
reply.print("The following commits have been pushed to ");
reply.print(pr.targetRef());
@@ -73,8 +73,10 @@ void branchMerge(TestInfo testInfo) throws IOException {
// Make a change with a corresponding PR
var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8);
localRepo.add(unrelated);
localRepo.commit("Unrelated", "some", "some@one");
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");
@@ -155,7 +157,9 @@ void branchMergeShortName(TestInfo testInfo) throws IOException {
// Make a change with a corresponding PR
var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8);
localRepo.add(unrelated);
localRepo.commit("Unrelated", "some", "some@one");
var updatedMaster = localRepo.commit("Unrelated", "some", "some@one");
localRepo.push(updatedMaster, author.url(), "master");

localRepo.merge(otherHash2);
var mergeHash = localRepo.commit("Merge commit", "some", "some@one");
localRepo.push(mergeHash, author.url(), "edit", true);
@@ -238,6 +242,8 @@ void branchMergeRebase(TestInfo testInfo) throws IOException {
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.push(updatedMaster, author.url(), "master");

localRepo.merge(otherHash2);
var mergeHash = localRepo.commit("Merge commit", "some", "some@one");
localRepo.push(mergeHash, author.url(), "edit", true);
@@ -330,6 +336,8 @@ void branchMergeAdditionalCommits(TestInfo testInfo) throws IOException {
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.push(updatedMaster, author.url(), "master");

localRepo.merge(otherHash2);
var mergeHash = localRepo.commit("Our own merge commit", "some", "some@one");
localRepo.push(mergeHash, author.url(), "edit", true);
@@ -360,6 +368,14 @@ void branchMergeAdditionalCommits(TestInfo testInfo) throws IOException {
// Let the bot notice again
TestBotRunner.runPeriodicItems(mergeBot);

// Merge the latest from master
localRepo.merge(newMasterHash);
var latestMergeHash = localRepo.commit("Our to be squashed merge commit", "some", "some@one");
localRepo.push(latestMergeHash, author.url(), "edit");

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

// Push it
pr.addComment("/integrate");
TestBotRunner.runPeriodicItems(mergeBot);
@@ -397,7 +413,7 @@ void branchMergeAdditionalCommits(TestInfo testInfo) throws IOException {
}

@Test
void invalidSourceRepo(TestInfo testInfo) throws IOException {
void invalidMergeCommit(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory()) {

@@ -427,6 +443,66 @@ void invalidSourceRepo(TestInfo testInfo) throws IOException {
var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8);
localRepo.add(unrelated);
localRepo.commit("Unrelated", "some", "some@one");
localRepo.merge(otherHash);
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 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")));

var check = pr.checks(mergeHash).get("jcheck");
assertEquals("- It was not possible to create a commit for the changes in this PR: No merge commit containing commits from another branch than the target was found", check.summary().orElseThrow());
}
}

@Test
void invalidSourceRepo(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 a change in another branch
var otherHash = CheckableRepository.appendAndCommit(localRepo, "Change in other",
"Other\n\nReviewed-by: integrationreviewer2");
localRepo.push(otherHash, author.url(), "other", true);

// 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.push(updatedMaster, author.url(), "master");

localRepo.merge(otherHash);
var mergeHash = localRepo.commit("Merge commit", "some", "some@one");
localRepo.push(mergeHash, author.url(), "edit", true);
@@ -484,7 +560,9 @@ void invalidSourceBranch(TestInfo testInfo) throws IOException {
// Make a change with a corresponding PR
var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8);
localRepo.add(unrelated);
localRepo.commit("Unrelated", "some", "some@one");
var updatedMaster = localRepo.commit("Unrelated", "some", "some@one");
localRepo.push(updatedMaster, author.url(), "master");

localRepo.merge(otherHash);
var mergeHash = localRepo.commit("Merge commit", "some", "some@one");
localRepo.push(mergeHash, author.url(), "edit", true);
@@ -547,7 +625,9 @@ void wrongSourceBranch(TestInfo testInfo) throws IOException {
// Make a change with a corresponding PR
var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8);
localRepo.add(unrelated);
localRepo.commit("Unrelated", "some", "some@one");
var updatedMaster = localRepo.commit("Unrelated", "some", "some@one");
localRepo.push(updatedMaster, author.url(), "master");

localRepo.merge(other1Hash, "ours");
var mergeHash = localRepo.commit("Merge commit", "some", "some@one");
localRepo.push(mergeHash, author.url(), "edit", true);
@@ -605,7 +685,9 @@ void invalidAuthor(TestInfo testInfo) throws IOException {
// Make a change with a corresponding PR
var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8);
localRepo.add(unrelated);
localRepo.commit("Unrelated", "some", "some@one");
var updatedMaster = localRepo.commit("Unrelated", "some", "some@one");
localRepo.push(updatedMaster, author.url(), "master");

localRepo.merge(otherHash);
var mergeHash = localRepo.commit("Merge commit", "some", "some@one");
localRepo.push(mergeHash, author.url(), "edit", true);