Skip to content
Permalink
Browse files
Proper selection of merge commit to squash commits onto for merge sty…
…le PRs

Reviewed-by: ehelin
  • Loading branch information
rwestberg committed Mar 30, 2020
1 parent 870bf78 commit c0a0a5088383b2889fd87e7fac713a04c445be1e
Showing 3 changed files with 134 additions and 20 deletions.
@@ -590,10 +590,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());
@@ -608,9 +614,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);

0 comments on commit c0a0a50

Please sign in to comment.