Skip to content
Permalink
Browse files
362: Only amend merge commits that has the target branch as parent
Reviewed-by: ehelin
  • Loading branch information
rwestberg committed Apr 16, 2020
1 parent 87aedde commit fed35009ef8aff1e2de581ef917eee42771c8202
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 15 deletions.
@@ -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, false);
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());
}
}

@@ -746,7 +746,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)

0 comments on commit fed3500

Please sign in to comment.