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

diff --git a/java/com/google/gerrit/server/restapi/change/CreateChange.java b/java/com/google/gerrit/server/restapi/change/CreateChange.java
index 2326027..12f87a0 100644
--- a/java/com/google/gerrit/server/restapi/change/CreateChange.java
+++ b/java/com/google/gerrit/server/restapi/change/CreateChange.java
@@ -81,6 +81,8 @@
 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.ObjectId;
@@ -456,15 +458,24 @@
     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

diff --git a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
index 79ad863..4b8a0ec 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
@@ -49,6 +49,7 @@
 import com.google.gerrit.testing.FakeEmailSender.Message;
 import com.google.gerrit.testing.TestTimeUtil;
 import com.google.inject.Inject;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import org.eclipse.jgit.lib.ObjectId;
@@ -313,6 +314,62 @@
   }
 
   @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(new Branch.NameKey(project, branchA));
+    createBranch(new Branch.NameKey(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");
     ChangeInput in = newMergeChangeInput("branchA", "invalid", "");