MergeOp: Check commit status separately after merge strategies

The updateChangeStatus method was a little weird, basically following
disjoint codepaths depending on the value of the boolean dryRun
argument. The first time through, which happened immediately after
running the submit strategies, might throw exceptions for missing
dependencies or other invalid statuses. The second time through, after
executing ref updates, was actually responsible for setting the status
on the change objects in the database. (It could theoretically throw
in the same cases as the first time it was called, but those should
have already thrown already so it's essentially dead code.)

Split the first set of checks into a new method,
checkMergeStrategyResults, that has an explicit name for what it does,
and uses the same problem/failFast structure as we've started adopting
elsewhere.

Change-Id: I3ea6e5a47d3173749773c0cf7fa175c8048b1a0a

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java

@@ -26,7 +26,6 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.ApprovalsUtil;
-import com.google.gerrit.server.git.CommitMergeStatus;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -117,7 +116,8 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
         push("refs/for/master%submit", "other change", "a.txt", "other content");
     r.assertErrorStatus();
     r.assertChange(Change.Status.NEW, null);
-    r.assertMessage(CommitMergeStatus.PATH_CONFLICT.getMessage());
+    r.assertMessage("Change " + r.getChange().getId()
+        + ": change could not be merged due to a path conflict.");
   }
 
   @Test

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java

@@ -77,8 +77,9 @@ public abstract class AbstractSubmitByMerge extends AbstractSubmit {
     PushOneCommit.Result change2 =
         createChange("Change 2", "a.txt", "other content");
     submitWithConflict(change2.getChangeId(),
-        "Cannot merge " + change2.getCommit().name() + "\n" +
+        "Failed to submit 1 change due to the following problems:\n" +
-        "Change could not be merged due to a path conflict.\n\n" +
+        "Change " + change2.getChange().getId() + ": " +
+        "Change could not be merged due to a path conflict. " +
         "Please rebase the change locally " +
         "and upload the rebased commit for review.");
     assertThat(getRemoteHead()).isEqualTo(oldHead);

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java

@@ -16,7 +16,6 @@ package com.google.gerrit.acceptance.rest.change;
 
 import static com.google.common.truth.Truth.assertThat;
 
-import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.TestProjectInput;
 import com.google.gerrit.extensions.client.ChangeStatus;
@@ -106,9 +105,9 @@ public class SubmitByCherryPickIT extends AbstractSubmit {
     PushOneCommit.Result change2 =
         createChange("Change 2", "a.txt", "other content");
     submitWithConflict(change2.getChangeId(),
-        "Cannot merge " + change2.getCommitId().name() + "\n" +
+        "Failed to submit 1 change due to the following problems:\n" +
-        "Change could not be merged due to a path conflict.\n\n" +
+        "Change " + change2.getChange().getId() + ": Change could not be " +
-        "Please rebase the change locally and " +
+        "merged due to a path conflict. Please rebase the change locally and " +
         "upload the rebased commit for review.");
 
     assertThat(getRemoteHead()).isEqualTo(oldHead);
@@ -151,9 +150,9 @@ public class SubmitByCherryPickIT extends AbstractSubmit {
     PushOneCommit.Result change3 =
         createChange("Change 3", "b.txt", "different content");
     submitWithConflict(change3.getChangeId(),
-        "Cannot merge " + change3.getCommitId().name() + "\n" +
+        "Failed to submit 1 change due to the following problems:\n" +
-        "Change could not be merged due to a path conflict.\n\n" +
+        "Change " + change3.getChange().getId() + ": Change could not be " +
-        "Please rebase the change locally and " +
+        "merged due to a path conflict. Please rebase the change locally and " +
         "upload the rebased commit for review.");
 
     assertThat(getRemoteHead()).isEqualTo(oldHead);
@@ -227,15 +226,13 @@ public class SubmitByCherryPickIT extends AbstractSubmit {
     // Submit fails; change3 contains the delta "b1" -> "b2", which cannot be
     // applied against tip.
     submitWithConflict(change3.getChangeId(),
-        "Cannot merge " + change3.getCommitId().name() + "\n" +
+        "Failed to submit 1 change due to the following problems:\n" +
-        "Change could not be merged due to a path conflict.\n\n" +
+        "Change " + change3.getChange().getId() + ": Change could not be " +
-        "Please rebase the change locally and " +
+        "merged due to a path conflict. Please rebase the change locally and " +
         "upload the rebased commit for review.");
 
     ChangeInfo info3 = get(change3.getChangeId(), ListChangesOption.MESSAGES);
     assertThat(info3.status).isEqualTo(ChangeStatus.NEW);
-    assertThat(Iterables.getLast(info3.messages).message.toLowerCase())
-        .contains("path conflict");
 
     // Tip has not changed.
     List<RevCommit> log = getRemoteLog();

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java

@@ -100,9 +100,10 @@ public class SubmitByFastForwardIT extends AbstractSubmit {
     assertThat(info.enabled).isNull();
 
     submitWithConflict(change2.getChangeId(),
-        "Cannot merge " + change2.getCommitId().name() + "\n" +
+        "Failed to submit 1 change due to the following problems:\n" +
-        "Project policy requires all submissions to be a fast-forward.\n\n" +
+        "Change " + change2.getChange().getId() + ": Project policy requires " +
-        "Please rebase the change locally and upload again for review.");
+        "all submissions to be a fast-forward. Please rebase the change " +
+        "locally and upload again for review.");
     assertThat(getRemoteHead()).isEqualTo(oldHead);
     assertSubmitter(change.getChangeId(), 1);
   }

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java

@@ -181,9 +181,9 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge {
 
     if (isSubmitWholeTopicEnabled()) {
       submitWithConflict(change1b.getChangeId(),
-          "Cannot merge " + change3.getCommit().name() + "\n" +
+          "Failed to submit 5 changes due to the following problems:\n" +
-          "Change could not be merged due to a path conflict.\n\n" +
+          "Change " + change3.getChange().getId() + ": Change could not be " +
-          "Please rebase the change locally " +
+          "merged due to a path conflict. Please rebase the change locally " +
           "and upload the rebased commit for review.");
     } else {
       submit(change1b.getChangeId());
@@ -296,7 +296,11 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge {
         "a.txt", "1", "a-topic-here");
     approve(change3b.getChangeId());
 
-    submitWithConflict(change3a.getChangeId(), "Merge Conflict");
+    String cnt = isSubmitWholeTopicEnabled() ? "2 changes" : "1 change";
+    submitWithConflict(change3a.getChangeId(),
+        "Failed to submit " + cnt + " due to the following problems:\n"
+        + "Change " + change3a.getChange().getId() + ": depends on change that"
+        + " was not submitted");
 
     RevCommit tipbranch = getRemoteLog(project, "branch").get(0);
     assertThat(tipbranch.getShortMessage()).isEqualTo(

gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java

@@ -18,9 +18,12 @@ import static com.google.common.base.Preconditions.checkState;
 import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
 
 import com.google.auto.value.AutoValue;
+import com.google.common.base.CharMatcher;
+import com.google.common.base.Function;
 import com.google.common.base.Joiner;
 import com.google.common.base.Optional;
 import com.google.common.base.Predicate;
+import com.google.common.collect.FluentIterable;
 import com.google.common.collect.HashBasedTable;
 import com.google.common.collect.HashMultimap;
 import com.google.common.collect.ImmutableList;
@@ -495,17 +498,17 @@ public class MergeOp implements AutoCloseable {
       }
       failFast(cs); // Done checks that don't involve running submit strategies.
 
-      for (Project.NameKey project : br.keySet()) {
+      for (Branch.NameKey branch : cbb.keySet()) {
-        OpenRepo or = openRepo(project);
+        OpenRepo or = openRepo(branch.getParentKey());
-        for (Branch.NameKey branch : br.get(project)) {
         OpenBranch ob = or.getBranch(branch);
         BranchBatch submitting = toSubmit.get(branch);
         SubmitStrategy strategy = createStrategy(or, branch,
             submitting.submitType(), ob.oldTip, caller);
         ob.mergeTip = preMerge(strategy, submitting.changes(), ob.oldTip);
-          updateChangeStatus(ob, submitting.changes(), true, caller);
-          or.ins.flush();
       }
+      checkMergeStrategyResults(cs, toSubmit.values());
+      for (Project.NameKey project : br.keySet()) {
+        openRepo(project).ins.flush();
       }
 
       Set<Branch.NameKey> done =
@@ -519,7 +522,7 @@ public class MergeOp implements AutoCloseable {
           boolean updated = updateBranch(or, branch, caller);
 
           BranchBatch submitting = toSubmit.get(branch);
-          updateChangeStatus(ob, submitting.changes(), false, caller);
+          updateChangeStatus(ob, submitting.changes(), caller);
           updateSubmoduleSubscriptions(ob, subOp);
           if (updated) {
             fireRefUpdated(ob);
@@ -846,36 +849,86 @@ public class MergeOp implements AutoCloseable {
     return "";
   }
 
-  private void updateChangeStatus(OpenBranch ob, List<ChangeData> submitted,
+  private Iterable<ChangeData> flattenBatches(Collection<BranchBatch> batches) {
-      boolean dryRun, IdentifiedUser caller) throws NoSuchChangeException,
+    return FluentIterable.from(batches)
-      IntegrationException, ResourceConflictException, OrmException {
+        .transformAndConcat(new Function<BranchBatch, List<ChangeData>>() {
-    if (!dryRun) {
+          @Override
-      logDebug("Updating change status for {} changes", submitted.size());
+          public List<ChangeData> apply(BranchBatch batch) {
-    } else {
+            return batch.changes();
-      logDebug("Checking change state for {} changes in a dry run",
-          submitted.size());
           }
+        });
+  }
+
+  private void checkMergeStrategyResults(ChangeSet cs,
+      Collection<BranchBatch> batches) throws ResourceConflictException {
+    for (ChangeData cd : flattenBatches(batches)) {
+      Change.Id id = cd.getId();
+      CodeReviewCommit commit = commits.get(id);
+      CommitMergeStatus s = commit != null ? commit.getStatusCode() : null;
+      if (s == null) {
+        problems.put(id,
+            "internal error: change not processed by merge strategy");
+        continue;
+      }
+      switch (s) {
+        case CLEAN_MERGE:
+        case CLEAN_REBASE:
+        case CLEAN_PICK:
+        case ALREADY_MERGED:
+          break; // Merge strategy accepted this change.
+
+        case PATH_CONFLICT:
+        case REBASE_MERGE_CONFLICT:
+        case MANUAL_RECURSIVE_MERGE:
+        case CANNOT_CHERRY_PICK_ROOT:
+        case NOT_FAST_FORWARD:
+        case INVALID_PROJECT_CONFIGURATION:
+        case INVALID_PROJECT_CONFIGURATION_PLUGIN_VALUE_NOT_PERMITTED:
+        case INVALID_PROJECT_CONFIGURATION_PLUGIN_VALUE_NOT_EDITABLE:
+        case INVALID_PROJECT_CONFIGURATION_PARENT_PROJECT_NOT_FOUND:
+        case INVALID_PROJECT_CONFIGURATION_ROOT_PROJECT_CANNOT_HAVE_PARENT:
+        case SETTING_PARENT_PROJECT_ONLY_ALLOWED_BY_ADMIN:
+          // TODO(dborowitz): Reformat these messages to be more appropriate for
+          // short problem descriptions.
+          problems.put(id,
+              CharMatcher.is('\n').collapseFrom(s.getMessage(), ' '));
+          break;
+
+        case MISSING_DEPENDENCY:
+          problems.put(id, "depends on change that was not submitted");
+          break;
+
+        case REVISION_GONE:
+          // TODO(dborowitz): Should no longer be generated by built-in code;
+          // finish removing.
+          throw new IllegalStateException();
+
+        default:
+          problems.put(id, "unspecified merge failure: " + s);
+          break;
+      }
+    }
+    failFast(cs);
+  }
+
+  private void updateChangeStatus(OpenBranch ob, List<ChangeData> submitted,
+      IdentifiedUser caller) throws NoSuchChangeException, IntegrationException,
+      ResourceConflictException, OrmException {
+    logDebug("Updating change status for {} changes", submitted.size());
 
     for (ChangeData cd : submitted) {
       Change c = cd.change();
       CodeReviewCommit commit = commits.get(c.getId());
       CommitMergeStatus s = commit != null ? commit.getStatusCode() : null;
-      if (s == null) {
+      checkState(s != null,
-        // Shouldn't ever happen, but leave the change alone. We'll pick
+          "status not set for change %s; expected to previously fail fast",
-        // it up on the next pass.
+          c.getId());
-        //
-        logDebug("Submitted change {} did not appear in set of new commits"
-            + " produced by merge strategy", c.getId());
-        continue;
-      }
 
-      if (!dryRun) {
       try {
         setApproval(cd, caller);
       } catch (IOException e) {
         throw new OrmException(e);
       }
-      }
 
       String txt = s.getMessage();
       logDebug("Status of change {} ({}) on {}: {}", c.getId(), commit.name(),
@@ -890,24 +943,18 @@ public class MergeOp implements AutoCloseable {
         ChangeMessage msg;
         switch (s) {
           case CLEAN_MERGE:
-            if (!dryRun) {
             setMerged(c, message(c, txt + getByAccountName(commit)),
                 mergeResultRev);
-            }
             break;
 
           case CLEAN_REBASE:
           case CLEAN_PICK:
-            if (!dryRun) {
             setMerged(c, message(c, txt + " as " + commit.name()
                 + getByAccountName(commit)), mergeResultRev);
-            }
             break;
 
           case ALREADY_MERGED:
-            if (!dryRun) {
             setMerged(c, null, mergeResultRev);
-            }
             break;
 
           case PATH_CONFLICT:

gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java

@@ -179,6 +179,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -270,6 +271,9 @@ public class ReceiveCommits {
         public RestApiException apply(Exception input) {
           if (input instanceof RestApiException) {
             return (RestApiException) input;
+          } else if ((input instanceof ExecutionException)
+              && (input.getCause() instanceof RestApiException)) {
+            return (RestApiException) input.getCause();
           }
           return new RestApiException("Error inserting change/patchset", input);
         }
@@ -844,6 +848,9 @@ public class ReceiveCommits {
         f.checkedGet();
       }
       magicBranch.cmd.setResult(OK);
+    } catch (ResourceConflictException e) {
+      addMessage(e.getMessage());
+      reject(magicBranch.cmd, "conflict");
     } catch (RestApiException err) {
       log.error("Can't insert change/patchset for " + project.getName()
           + ". " + err.getMessage(), err);