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

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

var contributor = namespace.get(pr.author().id());
@@ -183,9 +186,13 @@ private Hash commitMerge(List<Review> activeReviews, Namespace namespace, String
return localRepo.amend(commitMessage, author.name(), author.email(), committer.name(), committer.email());
}

private boolean isMergeCommit() {
return pr.title().startsWith("Merge");
}

Hash commit(Namespace namespace, String censusDomain, String sponsorId) throws IOException, CommitFailure {
var activeReviews = filterActiveReviews(pr.reviews());
if (!pr.title().startsWith("Merge")) {
if (!isMergeCommit()) {
return commitSquashed(activeReviews, namespace, censusDomain, sponsorId);
} else {
return commitMerge(activeReviews, namespace, censusDomain, sponsorId);
@@ -215,6 +222,12 @@ Optional<Hash> rebase(Hash commitHash, PrintWriter reply) {

try {
var commit = localRepo.lookup(commitHash).orElseThrow();
if (isMergeCommit()) {
// TODO: We can solve this with retroactive rerere
reply.println("Merge PRs cannot currently be automatically rebased. You will need to rebase it manually.");
return Optional.empty();
}

localRepo.rebase(targetHash, commit.committer().name(), commit.committer().email());
reply.println();
reply.println("Your commit was automatically rebased without conflicts.");
@@ -209,6 +209,7 @@ void branchMergeShortName(TestInfo testInfo) throws IOException {
}

@Test
@Disabled
void branchMergeRebase(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory()) {
@@ -303,6 +304,7 @@ void branchMergeRebase(TestInfo testInfo) throws IOException {
}

@Test
@Disabled
void branchMergeAdditionalCommits(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory()) {
@@ -466,7 +468,7 @@ void invalidMergeCommit(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("- 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());
assertEquals("- It was not possible to create a commit for the changes in this PR: A merge PR is only allowed to contain a single merge commit. You will need to amend your commits.", check.summary().orElseThrow());
}
}

@@ -683,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");
@@ -711,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());
}
}

@@ -836,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());
}
}

@@ -727,7 +727,7 @@ public boolean isAncestor(Hash ancestor, Hash descendant) throws IOException {

@Override
public void rebase(Hash hash, String committerName, String committerEmail) throws IOException {
try (var p = Process.capture("git", "rebase", "--onto", hash.hex(), "--root", "--rebase-merges")
try (var p = Process.capture("git", "rebase", "--onto", hash.hex(), "--root")
.environ("GIT_COMMITTER_NAME", committerName)
.environ("GIT_COMMITTER_EMAIL", committerEmail)
.workdir(dir)