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

362: Only amend merge commits that has the target branch as parent #584

Closed
wants to merge 3 commits into from
Closed
Changes from 2 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
@@ -178,11 +178,12 @@ 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 {
// Find the last merge commit - the very last commit is not eligible (as the merge needs a parent)
// Find the first 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;
break;
}
}
if (mergeCommitIndex >= commits.size() - 1) {
@@ -198,10 +199,12 @@ private List<String> botSpecificChecks() throws IOException {
try {
var sourceHash = prInstance.localRepo().fetch(mergeSourceRepo.url(), source.get().branchName);
var mergeCommit = commits.get(mergeCommitIndex);
for (int i = 1; i < mergeCommit.parents().size(); ++i) {
for (int i = 0; i < mergeCommit.parents().size(); ++i) {
if (!prInstance.localRepo().isAncestor(mergeCommit.parents().get(i), sourceHash)) {
ret.add("The merge contains commits that are not ancestors of the source.");
break;
if (!mergeCommit.parents().get(i).equals(prInstance.targetHash())) {
ret.add("The merge contains commits that are neither ancestors of the source nor the target.");
break;
}
}
}
} catch (IOException e) {
@@ -139,7 +139,7 @@ private Hash commitSquashed(List<Review> activeReviews, Namespace namespace, Str
}

private Hash commitMerge(List<Review> activeReviews, Namespace namespace, String censusDomain, String sponsorId) throws IOException, CommitFailure {
// Find the single merge commit with an incoming parent outside of the merge target
// 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();
@@ -152,11 +152,8 @@ private Hash commitMerge(List<Review> activeReviews, Namespace namespace, String
}
}
if (isSourceMerge) {
if (mergeCommitIndex != commits.size()) {
// TODO: We could allow this
throw new CommitFailure("A merge PR is only allowed to contain a single merge commit with incoming changes. Please amend!");
}
mergeCommitIndex = i;
break;
} else {
// TODO: We can solve this with retroactive rerere
throw new CommitFailure("A merge PR is only allowed to contain a single merge commit. You will need to amend your commits.");
@@ -685,6 +685,7 @@ void wrongSourceBranch(TestInfo testInfo) throws IOException {
localRepo.push(other2Hash, author.url(), "other2", true);

// Make a change with a corresponding PR
localRepo.checkout(masterHash, true);
var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8);
localRepo.add(unrelated);
var updatedMaster = localRepo.commit("Unrelated", "some", "some@one");
@@ -713,7 +714,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 neither ancestors of the source nor the target.", check.summary().orElseThrow());
}
}

@@ -838,11 +839,14 @@ void unrelatedHistory(TestInfo testInfo) throws IOException {
pr.addComment("/integrate");
TestBotRunner.runPeriodicItems(mergeBot);

// The bot should reply with an ok message as we currently allow this
var pushed = pr.comments().stream()
.filter(comment -> comment.body().contains("Pushed as commit"))
.count();
assertEquals(1, pushed);
// 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("- The merge contains commits that are neither ancestors of the source nor the target.", check.summary().orElseThrow());
}
}