CreateChange: Do not fail with 500 ISE if creating merge commit fails with NoMergeBaseException

NoMergeBaseException is thrown in 3 cases:

1. TOO_MANY_MERGE_BASES:
   The number of merge bases exceeds 200.

2. CONFLICTS_DURING_MERGE_BASE_CALCULATION:
   In order to find a single merge base it may required to merge
   together multiple common predecessors. If during these merges
   conflicts occur the merge fails with this reason.

3. MULTIPLE_MERGE_BASES_NOT_SUPPORTED:
   Multiple merge bases have been found (e.g. the commits to be merged
   have multiple common predecessors) but the merge strategy doesn't
   support this (e.g. ResolveMerge).

3. cannot happen because we always use a RecursiveMerger which does
support having multiple merge bases. If it happens anyway, that's an
error.

1. and 2. are not Gerrit errors, but the state of the input commits
don't allow us to create a merge commit. Hence in this situation we
should not fail with 500 Internal Server Error, but rather return 409
Conflict and let the user know what's the problem.

This change has a test for 1. that verifies that we return 409 Conflict
with a proper error message now.

I wasn't able to reproduce 2. from a test, but this is what we saw in
production at Google.

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ia485e24c1bf3fc3e3857eb0669b67e11623f4a58

java/com/google/gerrit/server/restapi/change/CreateChange.java

@@ -84,6 +84,8 @@ import java.util.Optional;
 import java.util.TimeZone;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.errors.InvalidObjectIdException;
+import org.eclipse.jgit.errors.NoMergeBaseException;
+import org.eclipse.jgit.errors.NoMergeBaseException.MergeBaseFailureReason;
 import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.Constants;
@@ -483,15 +485,24 @@ public class CreateChange
     String mergeStrategy =
         firstNonNull(Strings.emptyToNull(merge.strategy), mergeUtil.mergeStrategyName());
 
-    return MergeUtil.createMergeCommit(
-        oi,
-        repo.getConfig(),
-        mergeTip,
-        sourceCommit,
-        mergeStrategy,
-        authorIdent,
-        commitMessage,
-        rw);
+    try {
+      return MergeUtil.createMergeCommit(
+          oi,
+          repo.getConfig(),
+          mergeTip,
+          sourceCommit,
+          mergeStrategy,
+          authorIdent,
+          commitMessage,
+          rw);
+    } catch (NoMergeBaseException e) {
+      if (MergeBaseFailureReason.TOO_MANY_MERGE_BASES == e.getReason()
+          || MergeBaseFailureReason.CONFLICTS_DURING_MERGE_BASE_CALCULATION == e.getReason()) {
+        throw new ResourceConflictException(
+            String.format("Cannot create merge commit: %s", e.getMessage()), e);
+      }
+      throw e;
+    }
   }
 
   private static ObjectId insert(ObjectInserter inserter, CommitBuilder commit) throws IOException {

javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java

@@ -53,6 +53,7 @@ import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.submit.ChangeAlreadyMergedException;
 import com.google.gerrit.testing.FakeEmailSender.Message;
 import com.google.inject.Inject;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Callable;
@@ -348,6 +349,62 @@ public class CreateChangeIT extends AbstractDaemonTest {
     assertCreateSucceeds(in);
   }
 
+  @Test
+  public void createMergeChangeFailsWithConflictIfThereAreTooManyCommonPredecessors()
+      throws Exception {
+    // Create an initial commit in master.
+    Result initialCommit =
+        pushFactory
+            .create(user.newIdent(), testRepo, "initial commit", "readme.txt", "initial commit")
+            .to("refs/heads/master");
+    initialCommit.assertOkStatus();
+
+    String file = "shared.txt";
+    List<RevCommit> parents = new ArrayList<>();
+    // RecursiveMerger#MAX_BASES = 200, cannot use RecursiveMerger#MAX_BASES as it is not static.
+    int maxBases = 200;
+
+    // Create more than RecursiveMerger#MAX_BASES base commits.
+    for (int i = 1; i <= maxBases + 1; i++) {
+      parents.add(
+          testRepo
+              .commit()
+              .message("Base " + i)
+              .add(file, "content " + i)
+              .parent(initialCommit.getCommit())
+              .create());
+    }
+
+    // Create 2 branches.
+    String branchA = "branchA";
+    String branchB = "branchB";
+    createBranch(BranchNameKey.create(project, branchA));
+    createBranch(BranchNameKey.create(project, branchB));
+
+    // Push an octopus merge to both of the branches.
+    Result octopusA =
+        pushFactory
+            .create(user.newIdent(), testRepo)
+            .setParents(parents)
+            .to("refs/heads/" + branchA);
+    octopusA.assertOkStatus();
+
+    Result octopusB =
+        pushFactory
+            .create(user.newIdent(), testRepo)
+            .setParents(parents)
+            .to("refs/heads/" + branchB);
+    octopusB.assertOkStatus();
+
+    // Creating a merge commit for the 2 octopus commits fails, because they have more than
+    // RecursiveMerger#MAX_BASES common predecessors.
+    assertCreateFails(
+        newMergeChangeInput("branchA", "branchB", ""),
+        ResourceConflictException.class,
+        "Cannot create merge commit: No merge base could be determined."
+            + " Reason=TOO_MANY_MERGE_BASES.");
+  }
+
   @Test
   public void invalidSource() throws Exception {
     changeInTwoBranches("branchA", "a.txt", "branchB", "b.txt");