MergeOp: Simplify updateChangeStatus

After updating branch tips, we only need to consider status codes that
indicate successful merging, and add appropriate messages for those
status codes. The rest of the status codes should have been caught in
the previous call to checkMergeStrategyResults.

This also means we no longer need the setNew method, which was left
over from when changes were moved into state SUBMITTED at the
beginning of the process. Now that we are appropriately failing fast
and no longer have that state, we don't actually need to record in the
changes what went wrong during the merge; it all comes from the
ResourceConflictException message handed to the user.

If there are any errors in this phase, give a more detailed report of
which changes failed, even though we can't really do anything about it
at this point. This is slightly different from the existing failFast
method because we don't want the header boilerplate saying that
submitting all the changes failed, since some may have succeeded.

Change-Id: I8c1380551449f7f1b5dc7a29af84a79259495461

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

@@ -65,7 +65,6 @@ import com.google.gerrit.server.git.strategy.SubmitStrategyFactory;
 import com.google.gerrit.server.git.validators.MergeValidationException;
 import com.google.gerrit.server.git.validators.MergeValidators;
 import com.google.gerrit.server.index.ChangeIndexer;
-import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
@@ -912,93 +911,59 @@ public class MergeOp implements AutoCloseable {
   }
 
   private void updateChangeStatus(OpenBranch ob, List<ChangeData> submitted,
-      IdentifiedUser caller) throws NoSuchChangeException, IntegrationException,
+      IdentifiedUser caller) throws ResourceConflictException {
-      ResourceConflictException, OrmException {
+    List<Change.Id> problemChanges = new ArrayList<>(submitted.size());
     logDebug("Updating change status for {} changes", submitted.size());
 
     for (ChangeData cd : submitted) {
-      Change c = cd.change();
+      Change.Id id = cd.getId();
-      CodeReviewCommit commit = commits.get(c.getId());
-      CommitMergeStatus s = commit != null ? commit.getStatusCode() : null;
-      checkState(s != null,
-          "status not set for change %s; expected to previously fail fast",
-          c.getId());
-
       try {
+        Change c = cd.change();
+        CodeReviewCommit commit = commits.get(id);
+        CommitMergeStatus s = commit != null ? commit.getStatusCode() : null;
+        logDebug("Status of change {} ({}) on {}: {}", id, commit.name(),
+            c.getDest(), s);
+        checkState(s != null,
+            "status not set for change %s; expected to previously fail fast",
+            id);
         setApproval(cd, caller);
-      } catch (IOException e) {
-        throw new OrmException(e);
-      }
 
-      String txt = s.getMessage();
+        ObjectId mergeResultRev = ob.mergeTip != null
-      logDebug("Status of change {} ({}) on {}: {}", c.getId(), commit.name(),
+            ? ob.mergeTip.getMergeResults().get(commit) : null;
-          c.getDest(), s);
+        String txt = s.getMessage();
-      // If mergeTip is null merge failed and mergeResultRev will not be read.
-      ObjectId mergeResultRev = ob.mergeTip != null
-          ? ob.mergeTip.getMergeResults().get(commit) : null;
-      // The change notes must be forcefully reloaded so that the SUBMIT
-      // approval that we added earlier is visible
-      commit.notes().reload();
-      try {
-        ChangeMessage msg;
-        switch (s) {
-          case CLEAN_MERGE:
-            setMerged(c, message(c, txt + getByAccountName(commit)),
-                mergeResultRev);
-            break;
 
-          case CLEAN_REBASE:
+        // The change notes must be forcefully reloaded so that the SUBMIT
-          case CLEAN_PICK:
+        // approval that we added earlier is visible
-            setMerged(c, message(c, txt + " as " + commit.name()
+        commit.notes().reload();
-                + getByAccountName(commit)), mergeResultRev);
+        if (s == CommitMergeStatus.CLEAN_MERGE) {
-            break;
+          setMerged(c, message(c, txt + getByAccountName(commit)),
-
+              mergeResultRev);
-          case ALREADY_MERGED:
+        } else if (s == CommitMergeStatus.CLEAN_REBASE
-            setMerged(c, null, mergeResultRev);
+            || s == CommitMergeStatus.CLEAN_PICK) {
-            break;
+          setMerged(c, message(c, txt + " as " + commit.name()
-
+              + getByAccountName(commit)), mergeResultRev);
-          case PATH_CONFLICT:
+        } else if (s == CommitMergeStatus.ALREADY_MERGED) {
-          case REBASE_MERGE_CONFLICT:
+          setMerged(c, null, mergeResultRev);
-          case MANUAL_RECURSIVE_MERGE:
+        } else {
-          case CANNOT_CHERRY_PICK_ROOT:
+          throw new IllegalStateException("unexpected status " + s +
-          case NOT_FAST_FORWARD:
+              " for change " + c.getId() + "; expected to previously fail fast");
-          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:
-            setNew(commit.notes(), message(c, txt));
-            throw new ResourceConflictException("Cannot merge " + commit.name()
-                + "\n" + s.getMessage());
-
-          case MISSING_DEPENDENCY:
-            logDebug("Change {} is missing dependency", c.getId());
-            throw new IntegrationException(
-                "Cannot merge " + commit.name() + "\n" + s.getMessage());
-
-          case REVISION_GONE:
-            logDebug("Commit not found for change {}", c.getId());
-            msg = new ChangeMessage(
-                new ChangeMessage.Key(
-                    c.getId(),
-                    ChangeUtil.messageUUID(db)),
-                null,
-                TimeUtil.nowTs(),
-                c.currentPatchSetId());
-            msg.setMessage("Failed to read commit for this patch set");
-            setNew(commit.notes(), msg);
-            throw new IntegrationException(msg.getMessage());
-
-          default:
-            msg = message(c, "Unspecified merge failure: " + s.name());
-            setNew(commit.notes(), msg);
-            throw new IntegrationException(msg.getMessage());
         }
       } catch (OrmException | IOException err) {
-        logWarn("Error updating change status for " + c.getId(), err);
+        logWarn("Error updating change status for " + id, err);
+        problemChanges.add(id);
       }
     }
+
+    if (problemChanges.isEmpty()) {
+      return;
+    }
+    StringBuilder msg = new StringBuilder("Error updating status of change");
+    if (problemChanges.size() == 1) {
+      msg.append(' ').append(problemChanges.iterator().next());
+    } else {
+      msg.append('s').append(Joiner.on(", ").join(problemChanges));
+    }
+    throw new ResourceConflictException(msg.toString());
   }
 
   private void updateSubmoduleSubscriptions(OpenBranch ob, SubmoduleOp subOp) {
@@ -1242,69 +1207,6 @@ public class MergeOp implements AutoCloseable {
     }
   }
 
-  private ChangeControl changeControl(Change c) throws NoSuchChangeException {
-    return changeControlFactory.controlFor(
-        c, identifiedUserFactory.create(c.getOwner()));
-  }
-
-  private void setNew(ChangeNotes notes, final ChangeMessage msg)
-      throws NoSuchChangeException, IOException {
-    Change c = notes.getChange();
-
-    Change change = null;
-    ChangeUpdate update = null;
-    try {
-      db.changes().beginTransaction(c.getId());
-      try {
-        change = db.changes().atomicUpdate(
-            c.getId(),
-            new AtomicUpdate<Change>() {
-          @Override
-          public Change update(Change c) {
-            if (c.getStatus().isOpen()) {
-              c.setStatus(Change.Status.NEW);
-              ChangeUtil.updated(c);
-            }
-            return c;
-          }
-        });
-        ChangeControl control = changeControl(change);
-
-        //TODO(yyonas): atomic change is not propagated.
-        update = updateFactory.create(control, c.getLastUpdatedOn());
-        if (msg != null) {
-          cmUtil.addChangeMessage(db, update, msg);
-        }
-        db.commit();
-      } finally {
-        db.rollback();
-      }
-    } catch (OrmException err) {
-      logWarn("Cannot record merge failure message", err);
-    }
-    if (update != null) {
-      update.commit();
-    }
-    indexer.index(db, change);
-
-    PatchSetApproval submitter = null;
-    try {
-      submitter = approvalsUtil.getSubmitter(
-          db, notes, notes.getChange().currentPatchSetId());
-    } catch (Exception e) {
-      logError("Cannot get submitter for change " + notes.getChangeId(), e);
-    }
-    if (submitter != null) {
-      try {
-        hooks.doMergeFailedHook(c,
-            accountCache.get(submitter.getAccountId()).getAccount(),
-            db.patchSets().get(c.currentPatchSetId()), msg.getMessage(), db);
-      } catch (OrmException ex) {
-        logError("Cannot run hook for merge failed " + c.getId(), ex);
-      }
-    }
-  }
-
   private void abandonAllOpenChanges(Project.NameKey destProject)
       throws NoSuchChangeException {
     try {