diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index 61aac0e511..414e2e832a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/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 submitted, - IdentifiedUser caller) throws NoSuchChangeException, IntegrationException, - ResourceConflictException, OrmException { + IdentifiedUser caller) throws ResourceConflictException { + List problemChanges = new ArrayList<>(submitted.size()); 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; - checkState(s != null, - "status not set for change %s; expected to previously fail fast", - c.getId()); - + Change.Id id = cd.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(); - logDebug("Status of change {} ({}) on {}: {}", c.getId(), commit.name(), - c.getDest(), s); - // 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; + ObjectId mergeResultRev = ob.mergeTip != null + ? ob.mergeTip.getMergeResults().get(commit) : null; + String txt = s.getMessage(); - case CLEAN_REBASE: - case CLEAN_PICK: - setMerged(c, message(c, txt + " as " + commit.name() - + getByAccountName(commit)), mergeResultRev); - break; - - case ALREADY_MERGED: - setMerged(c, null, mergeResultRev); - break; - - 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: - 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()); + // The change notes must be forcefully reloaded so that the SUBMIT + // approval that we added earlier is visible + commit.notes().reload(); + if (s == CommitMergeStatus.CLEAN_MERGE) { + setMerged(c, message(c, txt + getByAccountName(commit)), + mergeResultRev); + } else if (s == CommitMergeStatus.CLEAN_REBASE + || s == CommitMergeStatus.CLEAN_PICK) { + setMerged(c, message(c, txt + " as " + commit.name() + + getByAccountName(commit)), mergeResultRev); + } else if (s == CommitMergeStatus.ALREADY_MERGED) { + setMerged(c, null, mergeResultRev); + } else { + throw new IllegalStateException("unexpected status " + s + + " for change " + c.getId() + "; expected to previously fail fast"); } } 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() { - @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 {