BatchUpdate: Add a combined operation spanning all phases

A common pattern thus far has been to communicate between ChangeOps
and subsequent Callables using AtomicReferences in the caller. This
works, but using so many AtomicReferences smells kind of wrong,
particularly since the caller-provided portions of BatchUpdate are
single-threaded.

Instead, create a single class Op, which may have steps for each of
the several phases, including a new repo-updating phase. Each of these
steps is passed in a Context object, which includes a phase-specific
view of the BatchUpdate. This encapsulation prevents operations from
doing unsupported things like adding new operations in the middle of
the execute process.

Ops can now communicate between different phases or with the caller
using instance variables.

Change-Id: Id97dbb772d2e3051d6a74bb8e819d691843a45e1

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

@@ -253,9 +253,10 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
     assertThat(c.revisions.get(expectedId.name())._number).isEqualTo(expectedNum);
     try (Repository repo =
         repoManager.openRepository(new Project.NameKey(c.project))) {
-      Ref ref = repo.getRef(
-          new PatchSet.Id(new Change.Id(c._number), expectedNum).toRefName());
-      assertThat(ref).isNotNull();
+      String refName = new PatchSet.Id(new Change.Id(c._number), expectedNum)
+          .toRefName();
+      Ref ref = repo.getRef(refName);
+      assertThat(ref).named(refName).isNotNull();
       assertThat(ref.getObjectId()).isEqualTo(expectedId);
     }
   }

gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java

@@ -33,11 +33,11 @@ import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.git.BatchUpdate;
-import com.google.gerrit.server.git.BatchUpdate.ChangeOp;
+import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
+import com.google.gerrit.server.git.BatchUpdate.Context;
 import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.mail.AbandonedSender;
 import com.google.gerrit.server.mail.ReplyToChangeSender;
-import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -48,8 +48,6 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.util.Collections;
-import java.util.concurrent.Callable;
-import java.util.concurrent.atomic.AtomicReference;
 
 @Singleton
 public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
@@ -92,64 +90,85 @@ public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
   }
 
   public Change abandon(ChangeControl control,
-      final String msgTxt, final Account acc) throws RestApiException,
-      UpdateException {
-    final Change.Id id = control.getChange().getId();
-    final AtomicReference<Change> change = new AtomicReference<>();
-    final AtomicReference<PatchSet> patchSet = new AtomicReference<>();
-    final AtomicReference<ChangeMessage> message = new AtomicReference<>();
-
+      final String msgTxt, final Account account)
+      throws RestApiException, UpdateException {
+    Op op = new Op(msgTxt, account);
     try (BatchUpdate u = batchUpdateFactory.create(dbProvider.get(),
         control.getChange().getProject(), TimeUtil.nowTs())) {
-      u.addChangeOp(new ChangeOp(control) {
-        @Override
-        public void call(ReviewDb db, ChangeUpdate update) throws OrmException,
-            ResourceConflictException {
-          Change c = db.changes().get(id);
-          if (c == null || !c.getStatus().isOpen()) {
-            throw new ResourceConflictException("change is " + status(c));
-          } else if (c.getStatus() == Change.Status.DRAFT) {
-            throw new ResourceConflictException(
-                "draft changes cannot be abandoned");
-          }
-          c.setStatus(Change.Status.ABANDONED);
-          ChangeUtil.updated(c);
-          db.changes().update(Collections.singleton(c));
-
-          ChangeMessage m = newMessage(
-              msgTxt, acc != null ? acc.getId() : null, c);
-          cmUtil.addChangeMessage(db, update, m);
-
-          change.set(c);
-          message.set(m);
-          patchSet.set(db.patchSets().get(c.currentPatchSetId()));
-        }
-      });
-      u.addPostOp(new Callable<Void>() {
-        @Override
-        public Void call() throws OrmException {
-          Change c = change.get();
-          try {
-            ReplyToChangeSender cm = abandonedSenderFactory.create(id);
-            if (acc != null) {
-              cm.setFrom(acc.getId());
-            }
-            cm.setChangeMessage(message.get());
-            cm.send();
-          } catch (Exception e) {
-            log.error("Cannot email update for change " + id, e);
-          }
-          hooks.doChangeAbandonedHook(c,
-              acc,
-              patchSet.get(),
-              Strings.emptyToNull(msgTxt),
-              dbProvider.get());
-          return null;
-        }
-      });
-      u.execute();
+      u.addOp(control, op).execute();
+    }
+    return op.change;
+  }
+
+  private class Op extends BatchUpdate.Op {
+    private final Account account;
+    private final String msgTxt;
+
+    private Change change;
+    private PatchSet patchSet;
+    private ChangeMessage message;
+
+    private Op(String msgTxt, Account account) {
+      this.account = account;
+      this.msgTxt = msgTxt;
+    }
+
+    @Override
+    public void updateChange(ChangeContext ctx) throws OrmException,
+        ResourceConflictException {
+      change = ctx.readChange();
+      if (change == null || !change.getStatus().isOpen()) {
+        throw new ResourceConflictException("change is " + status(change));
+      } else if (change.getStatus() == Change.Status.DRAFT) {
+        throw new ResourceConflictException(
+            "draft changes cannot be abandoned");
+      }
+      patchSet = ctx.getDb().patchSets().get(change.currentPatchSetId());
+      change.setStatus(Change.Status.ABANDONED);
+      change.setLastUpdatedOn(ctx.getWhen());
+      ctx.getDb().changes().update(Collections.singleton(change));
+
+      message = newMessage(ctx.getDb());
+      cmUtil.addChangeMessage(ctx.getDb(), ctx.getChangeUpdate(), message);
+    }
+
+    private ChangeMessage newMessage(ReviewDb db) throws OrmException {
+      StringBuilder msg = new StringBuilder();
+      msg.append("Abandoned");
+      if (!Strings.nullToEmpty(msgTxt).trim().isEmpty()) {
+        msg.append("\n\n");
+        msg.append(msgTxt.trim());
+      }
+
+      ChangeMessage message = new ChangeMessage(
+          new ChangeMessage.Key(
+              change.getId(),
+              ChangeUtil.messageUUID(db)),
+          account != null ? account.getId() : null,
+          change.getLastUpdatedOn(),
+          change.currentPatchSetId());
+      message.setMessage(msg.toString());
+      return message;
+    }
+
+    @Override
+    public void postUpdate(Context ctx) throws OrmException {
+      try {
+        ReplyToChangeSender cm = abandonedSenderFactory.create(change.getId());
+        if (account != null) {
+          cm.setFrom(account.getId());
+        }
+        cm.setChangeMessage(message);
+        cm.send();
+      } catch (Exception e) {
+        log.error("Cannot email update for change " + change.getId(), e);
+      }
+      hooks.doChangeAbandonedHook(change,
+          account,
+          patchSet,
+          Strings.emptyToNull(msgTxt),
+          ctx.getDb());
     }
-    return change.get();
   }
 
   @Override
@@ -162,26 +181,6 @@ public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
           && resource.getControl().canAbandon());
   }
 
-  private ChangeMessage newMessage(String msgTxt, Account.Id accId,
-      Change change) throws OrmException {
-    StringBuilder msg = new StringBuilder();
-    msg.append("Abandoned");
-    if (!Strings.nullToEmpty(msgTxt).trim().isEmpty()) {
-      msg.append("\n\n");
-      msg.append(msgTxt.trim());
-    }
-
-    ChangeMessage message = new ChangeMessage(
-        new ChangeMessage.Key(
-            change.getId(),
-            ChangeUtil.messageUUID(dbProvider.get())),
-        accId,
-        change.getLastUpdatedOn(),
-        change.currentPatchSetId());
-    message.setMessage(msg.toString());
-    return message;
-  }
-
   private static String status(Change change) {
     return change != null ? change.getStatus().name().toLowerCase() : "deleted";
   }

gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java

@@ -37,13 +37,14 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.events.CommitReceivedEvent;
 import com.google.gerrit.server.git.BanCommit;
 import com.google.gerrit.server.git.BatchUpdate;
-import com.google.gerrit.server.git.BatchUpdate.ChangeOp;
 import com.google.gerrit.server.git.GroupCollector;
+import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
+import com.google.gerrit.server.git.BatchUpdate.Context;
+import com.google.gerrit.server.git.BatchUpdate.RepoContext;
 import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.git.validators.CommitValidationException;
 import com.google.gerrit.server.git.validators.CommitValidators;
 import com.google.gerrit.server.mail.ReplacePatchSetSender;
-import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.notedb.ReviewerState;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.project.ChangeControl;
@@ -68,8 +69,6 @@ import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.util.Collections;
-import java.util.concurrent.Callable;
-import java.util.concurrent.atomic.AtomicReference;
 
 public class PatchSetInserter {
   private static final Logger log =
@@ -274,9 +273,6 @@ public class PatchSetInserter {
       IOException, NoSuchChangeException, UpdateException, RestApiException {
     init();
     validate();
-    final AtomicReference<Change> updatedChange = new AtomicReference<>();
-    final AtomicReference<SetMultimap<ReviewerState, Account.Id>> oldReviewers
-        = new AtomicReference<>();
 
     // TODO(dborowitz): Kill once callers are migrated.
     // Eventually, callers should always be responsible for executing.
@@ -288,103 +284,12 @@ public class PatchSetInserter {
       executeBatch = true;
     }
 
+    Op op = new Op();
     try {
-      bu.getBatchRefUpdate().addCommand(new ReceiveCommand(ObjectId.zeroId(),
-          commit, patchSet.getRefName(), ReceiveCommand.Type.CREATE));
-      bu.addChangeOp(new ChangeOp(ctl) {
-        @Override
-        public void call(ReviewDb db, ChangeUpdate update)
-            throws Exception {
-          Change c = db.changes().get(update.getChange().getId());
-          final PatchSet.Id currentPatchSetId = c.currentPatchSetId();
-          if (!c.getStatus().isOpen() && !allowClosed) {
-            throw new InvalidChangeOperationException(String.format(
-                "Change %s is closed", c.getId()));
-          }
-
-          ChangeUtil.insertAncestors(db, patchSet.getId(), commit);
-          if (groups != null) {
-            patchSet.setGroups(groups);
-          } else {
-            patchSet.setGroups(GroupCollector.getCurrentGroups(db, c));
-          }
-          db.patchSets().insert(Collections.singleton(patchSet));
-
-          if (sendMail) {
-            oldReviewers.set(approvalsUtil.getReviewers(db, ctl.getNotes()));
-          }
-
-          updatedChange.set(
-              db.changes().atomicUpdate(c.getId(), new AtomicUpdate<Change>() {
-                @Override
-                public Change update(Change change) {
-                  if (change.getStatus().isClosed() && !allowClosed) {
-                    return null;
-                  }
-                  if (!change.currentPatchSetId().equals(currentPatchSetId)) {
-                    return null;
-                  }
-                  if (change.getStatus() != Change.Status.DRAFT
-                      && !allowClosed) {
-                    change.setStatus(Change.Status.NEW);
-                  }
-                  change.setCurrentPatchSet(patchSetInfoFactory.get(commit,
-                      patchSet.getId()));
-                  ChangeUtil.updated(change);
-                  return change;
-                }
-              }));
-          if (updatedChange.get() == null) {
-            throw new ChangeModifiedException(String.format(
-                "Change %s was modified", c.getId()));
-          }
-
-          approvalCopier.copy(db, ctl, patchSet);
-          if (messageIsForChange()) {
-            cmUtil.addChangeMessage(db, update, changeMessage);
-          }
-        }
-      });
-
+      bu.addOp(ctl, op);
       if (!messageIsForChange()) {
         commitMessageNotForChange(bu);
       }
-
-      if (sendMail) {
-        bu.addPostOp(new Callable<Void>() {
-          @Override
-          public Void call() {
-            Change c = updatedChange.get();
-            try {
-              PatchSetInfo info =
-                  patchSetInfoFactory.get(commit, patchSet.getId());
-              ReplacePatchSetSender cm =
-                  replacePatchSetFactory.create(c.getId());
-              cm.setFrom(user.getAccountId());
-              cm.setPatchSet(patchSet, info);
-              cm.setChangeMessage(changeMessage);
-              cm.addReviewers(oldReviewers.get().get(ReviewerState.REVIEWER));
-              cm.addExtraCC(oldReviewers.get().get(ReviewerState.CC));
-              cm.send();
-            } catch (Exception err) {
-              log.error("Cannot send email for new patch set on change "
-                  + c.getId(), err);
-            }
-            return null;
-          }
-        });
-      }
-
-      if (runHooks) {
-        bu.addPostOp(new Callable<Void>() {
-          @Override
-          public Void call() throws OrmException {
-            hooks.doPatchsetCreatedHook(updatedChange.get(), patchSet, db);
-            return null;
-          }
-        });
-      }
-
       if (executeBatch) {
         bu.execute();
       }
@@ -393,7 +298,97 @@ public class PatchSetInserter {
         bu.close();
       }
     }
-    return updatedChange.get();
+    return op.change;
+  }
+
+  private class Op extends BatchUpdate.Op {
+    private Change change;
+    private SetMultimap<ReviewerState, Account.Id> oldReviewers;
+
+    @Override
+    public void updateRepo(RepoContext ctx) throws IOException {
+      ctx.getBatchRefUpdate().addCommand(new ReceiveCommand(ObjectId.zeroId(),
+          commit, patchSet.getRefName(), ReceiveCommand.Type.CREATE));
+    }
+
+    @Override
+    public void updateChange(ChangeContext ctx) throws OrmException,
+        InvalidChangeOperationException {
+      change = ctx.readChange();
+      Change.Id id = change.getId();
+      final PatchSet.Id currentPatchSetId = change.currentPatchSetId();
+      if (!change.getStatus().isOpen() && !allowClosed) {
+        throw new InvalidChangeOperationException(String.format(
+            "Change %s is closed", change.getId()));
+      }
+
+      ChangeUtil.insertAncestors(db, patchSet.getId(), commit);
+      if (groups != null) {
+        patchSet.setGroups(groups);
+      } else {
+        patchSet.setGroups(GroupCollector.getCurrentGroups(db, change));
+      }
+      db.patchSets().insert(Collections.singleton(patchSet));
+
+      if (sendMail) {
+        oldReviewers = approvalsUtil.getReviewers(db, ctl.getNotes());
+      }
+
+      // TODO(dborowitz): Throw ResourceConflictException instead of using
+      // AtomicUpdate.
+      change = db.changes().atomicUpdate(id, new AtomicUpdate<Change>() {
+        @Override
+        public Change update(Change change) {
+          if (change.getStatus().isClosed() && !allowClosed) {
+            return null;
+          }
+          if (!change.currentPatchSetId().equals(currentPatchSetId)) {
+            return null;
+          }
+          if (change.getStatus() != Change.Status.DRAFT && !allowClosed) {
+            change.setStatus(Change.Status.NEW);
+          }
+          change.setCurrentPatchSet(patchSetInfoFactory.get(commit,
+              patchSet.getId()));
+          ChangeUtil.updated(change);
+          return change;
+        }
+      });
+      if (change == null) {
+        throw new ChangeModifiedException(String.format(
+            "Change %s was modified", id));
+      }
+
+      approvalCopier.copy(db, ctl, patchSet);
+      if (messageIsForChange()) {
+        cmUtil.addChangeMessage(db, ctx.getChangeUpdate(), changeMessage);
+      }
+    }
+
+    @Override
+    public void postUpdate(Context ctx) throws OrmException {
+      if (sendMail) {
+        try {
+          PatchSetInfo info =
+              patchSetInfoFactory.get(commit, patchSet.getId());
+          ReplacePatchSetSender cm = replacePatchSetFactory.create(
+              change.getId());
+          cm.setFrom(user.getAccountId());
+          cm.setPatchSet(patchSet, info);
+          cm.setChangeMessage(changeMessage);
+          cm.addReviewers(oldReviewers.get(ReviewerState.REVIEWER));
+          cm.addExtraCC(oldReviewers.get(ReviewerState.CC));
+          cm.send();
+        } catch (Exception err) {
+          log.error("Cannot send email for new patch set on change "
+              + change.getId(), err);
+        }
+      }
+
+      if (runHooks) {
+        hooks.doPatchsetCreatedHook(change, patchSet, ctx.getDb());
+      }
+    }
   }
 
   private void commitMessageNotForChange(BatchUpdate bu)
@@ -401,11 +396,12 @@ public class PatchSetInserter {
     if (changeMessage == null) {
       return;
     }
-    bu.addChangeOp(new ChangeOp(ctlFactory.controlFor(
-        changeMessage.getPatchSetId().getParentKey(), user)) {
+    bu.addOp(ctlFactory.controlFor(
+        changeMessage.getPatchSetId().getParentKey(), user), new Op() {
       @Override
-      public void call(ReviewDb db, ChangeUpdate update) throws OrmException {
-        cmUtil.addChangeMessage(db, update, changeMessage);
+      public void updateChange(ChangeContext ctx) throws OrmException {
+        cmUtil.addChangeMessage(
+            ctx.getDb(), ctx.getChangeUpdate(), changeMessage);
       }
     });
   }

gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java

@@ -19,7 +19,6 @@ import com.google.gerrit.common.ChangeHooks;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.DefaultInput;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
@@ -32,9 +31,9 @@ import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.change.PutTopic.Input;
 import com.google.gerrit.server.git.BatchUpdate;
-import com.google.gerrit.server.git.BatchUpdate.ChangeOp;
+import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
+import com.google.gerrit.server.git.BatchUpdate.Context;
 import com.google.gerrit.server.git.UpdateException;
-import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -42,10 +41,7 @@ import com.google.inject.Provider;
 import com.google.inject.Singleton;
 
 import java.io.IOException;
-import java.sql.Timestamp;
 import java.util.Collections;
-import java.util.concurrent.Callable;
-import java.util.concurrent.atomic.AtomicReference;
 
 @Singleton
 public class PutTopic implements RestModifyView<ChangeResource, Input>,
@@ -75,73 +71,73 @@ public class PutTopic implements RestModifyView<ChangeResource, Input>,
   public Response<String> apply(ChangeResource req, Input input)
       throws AuthException, UpdateException, RestApiException, OrmException,
       IOException {
-    if (input == null) {
-      input = new Input();
-    }
-    final String inputTopic = input.topic;
-
-    ChangeControl control = req.getControl();
-    if (!control.canEditTopicName()) {
+    ChangeControl ctl = req.getControl();
+    if (!ctl.canEditTopicName()) {
       throw new AuthException("changing topic not permitted");
     }
 
-    final Change.Id id = req.getChange().getId();
-    final IdentifiedUser caller = (IdentifiedUser) control.getCurrentUser();
-    final AtomicReference<Change> change = new AtomicReference<>();
-    final AtomicReference<String> oldTopicName = new AtomicReference<>();
-    final AtomicReference<String> newTopicName = new AtomicReference<>();
-
-    final Timestamp now = TimeUtil.nowTs();
+    Op op = new Op(ctl, input != null ? input : new Input());
     try (BatchUpdate u = batchUpdateFactory.create(dbProvider.get(),
-        req.getChange().getProject(), now)) {
-      u.addChangeOp(new ChangeOp(req.getControl()) {
-        @Override
-        public void call(ReviewDb db, ChangeUpdate update) throws OrmException,
-            ResourceConflictException {
-          Change c = db.changes().get(id);
-          String n = Strings.nullToEmpty(inputTopic);
-          String o = Strings.nullToEmpty(c.getTopic());
-          if (o.equals(n)) {
-            return;
-          }
-          String summary;
-          if (o.isEmpty()) {
-            summary = "Topic set to " + n;
-          } else if (n.isEmpty()) {
-            summary = "Topic " + o + " removed";
-          } else {
-            summary = String.format("Topic changed from %s to %s", o, n);
-          }
-          c.setTopic(Strings.emptyToNull(n));
-          ChangeUtil.updated(c);
-          db.changes().update(Collections.singleton(c));
-
-          ChangeMessage cmsg = new ChangeMessage(
-              new ChangeMessage.Key(id, ChangeUtil.messageUUID(db)),
-              caller.getAccountId(), now, c.currentPatchSetId());
-          cmsg.setMessage(summary);
-          cmUtil.addChangeMessage(db, update, cmsg);
-
-          change.set(c);
-          oldTopicName.set(o);
-          newTopicName.set(n);
-        }
-      });
-      u.addPostOp(new Callable<Void>() {
-        @Override
-        public Void call() throws OrmException {
-          Change c = change.get();
-          if (c != null) {
-            hooks.doTopicChangedHook(change.get(), caller.getAccount(),
-                oldTopicName.get(), dbProvider.get());
-          }
-          return null;
-        }
-      });
+        req.getChange().getProject(), TimeUtil.nowTs())) {
+      u.addOp(ctl, op);
       u.execute();
     }
-    String n = newTopicName.get();
-    return Strings.isNullOrEmpty(n) ? Response.<String> none() : Response.ok(n);
+    return Strings.isNullOrEmpty(op.newTopicName)
+        ? Response.<String> none()
+        : Response.ok(op.newTopicName);
+  }
+
+  private class Op extends BatchUpdate.Op {
+    private final Input input;
+    private final IdentifiedUser caller;
+
+    private Change change;
+    private String oldTopicName;
+    private String newTopicName;
+
+    public Op(ChangeControl ctl, Input input) {
+      this.input = input;
+      this.caller = (IdentifiedUser) ctl.getCurrentUser();
+    }
+
+    @Override
+    public void updateChange(ChangeContext ctx) throws OrmException {
+      change = ctx.readChange();
+      String newTopicName = Strings.nullToEmpty(input.topic);
+      String oldTopicName = Strings.nullToEmpty(change.getTopic());
+      if (oldTopicName.equals(newTopicName)) {
+        return;
+      }
+      String summary;
+      if (oldTopicName.isEmpty()) {
+        summary = "Topic set to " + newTopicName;
+      } else if (newTopicName.isEmpty()) {
+        summary = "Topic " + oldTopicName + " removed";
+      } else {
+        summary = String.format("Topic changed from %s to %s",
+            oldTopicName, newTopicName);
+      }
+      change.setTopic(Strings.emptyToNull(newTopicName));
+      ChangeUtil.updated(change);
+      ctx.getDb().changes().update(Collections.singleton(change));
+
+      ChangeMessage cmsg = new ChangeMessage(
+          new ChangeMessage.Key(
+              change.getId(),
+              ChangeUtil.messageUUID(ctx.getDb())),
+          caller.getAccountId(), ctx.getWhen(),
+          change.currentPatchSetId());
+      cmsg.setMessage(summary);
+      cmUtil.addChangeMessage(ctx.getDb(), ctx.getChangeUpdate(), cmsg);
+    }
+
+    @Override
+    public void postUpdate(Context ctx) throws OrmException {
+      if (change != null) {
+        hooks.doTopicChangedHook(change, caller.getAccount(),
+            oldTopicName, ctx.getDb());
+      }
+    }
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java

@@ -33,11 +33,11 @@ import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.git.BatchUpdate;
-import com.google.gerrit.server.git.BatchUpdate.ChangeOp;
+import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
+import com.google.gerrit.server.git.BatchUpdate.Context;
 import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.mail.ReplyToChangeSender;
 import com.google.gerrit.server.mail.RestoredSender;
-import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -49,8 +49,6 @@ import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.util.Collections;
-import java.util.concurrent.Callable;
-import java.util.concurrent.atomic.AtomicReference;
 
 @Singleton
 public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
@@ -87,55 +85,79 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
     if (!ctl.canRestore()) {
       throw new AuthException("restore not permitted");
     }
-    final Change.Id id = req.getChange().getId();
-    final IdentifiedUser caller = (IdentifiedUser) ctl.getCurrentUser();
-    final AtomicReference<Change> change = new AtomicReference<>();
-    final AtomicReference<PatchSet> patchSet = new AtomicReference<>();
-    final AtomicReference<ChangeMessage> message = new AtomicReference<>();
 
+    Op op = new Op(input);
     try (BatchUpdate u = batchUpdateFactory.create(dbProvider.get(),
         req.getChange().getProject(), TimeUtil.nowTs())) {
-      u.addChangeOp(new ChangeOp(req.getControl()) {
-        @Override
-        public void call(ReviewDb db, ChangeUpdate update) throws Exception {
-          Change c = db.changes().get(id);
-          if (c == null || c.getStatus() != Status.ABANDONED) {
-            throw new ResourceConflictException("change is " + status(c));
-          }
-          c.setStatus(Status.NEW);
-          ChangeUtil.updated(c);
-          db.changes().update(Collections.singleton(c));
-
-          ChangeMessage m = newMessage(input, caller, c);
-          cmUtil.addChangeMessage(db, update, m);
-
-          change.set(c);
-          message.set(m);
-          patchSet.set(db.patchSets().get(c.currentPatchSetId()));
-        }
-      });
-      u.addPostOp(new Callable<Void>() {
-        @Override
-        public Void call() throws OrmException {
-          try {
-            ReplyToChangeSender cm = restoredSenderFactory.create(id);
-            cm.setFrom(caller.getAccountId());
-            cm.setChangeMessage(message.get());
-            cm.send();
-          } catch (Exception e) {
-            log.error("Cannot email update for change " + id, e);
-          }
-          hooks.doChangeRestoredHook(change.get(),
-              caller.getAccount(),
-              patchSet.get(),
-              Strings.emptyToNull(input.message),
-              dbProvider.get());
-          return null;
-        }
-      });
-      u.execute();
+      u.addOp(ctl, op).execute();
+    }
+    return json.create(ChangeJson.NO_OPTIONS).format(op.change);
+  }
+
+  private class Op extends BatchUpdate.Op {
+    private final RestoreInput input;
+
+    private Change change;
+    private PatchSet patchSet;
+    private ChangeMessage message;
+    private IdentifiedUser caller;
+
+    private Op(RestoreInput input) {
+      this.input = input;
+    }
+
+    @Override
+    public void updateChange(ChangeContext ctx) throws OrmException,
+        ResourceConflictException {
+      caller = (IdentifiedUser) ctx.getUser();
+      change = ctx.readChange();
+      if (change == null || change.getStatus() != Status.ABANDONED) {
+        throw new ResourceConflictException("change is " + status(change));
+      }
+      patchSet = ctx.getDb().patchSets().get(change.currentPatchSetId());
+      change.setStatus(Status.NEW);
+      change.setLastUpdatedOn(ctx.getWhen());
+      ctx.getDb().changes().update(Collections.singleton(change));
+
+      message = newMessage(ctx.getDb());
+      cmUtil.addChangeMessage(ctx.getDb(), ctx.getChangeUpdate(), message);
+    }
+
+    private ChangeMessage newMessage(ReviewDb db) throws OrmException {
+      StringBuilder msg = new StringBuilder();
+      msg.append("Restored");
+      if (!Strings.nullToEmpty(input.message).trim().isEmpty()) {
+        msg.append("\n\n");
+        msg.append(input.message.trim());
+      }
+
+      ChangeMessage message = new ChangeMessage(
+          new ChangeMessage.Key(
+              change.getId(),
+              ChangeUtil.messageUUID(db)),
+          caller.getAccountId(),
+          change.getLastUpdatedOn(),
+          change.currentPatchSetId());
+      message.setMessage(msg.toString());
+      return message;
+    }
+
+    @Override
+    public void postUpdate(Context ctx) throws OrmException {
+      try {
+        ReplyToChangeSender cm = restoredSenderFactory.create(change.getId());
+        cm.setFrom(caller.getAccountId());
+        cm.setChangeMessage(message);
+        cm.send();
+      } catch (Exception e) {
+        log.error("Cannot email update for change " + change.getId(), e);
+      }
+      hooks.doChangeRestoredHook(change,
+          caller.getAccount(),
+          patchSet,
+          Strings.emptyToNull(input.message),
+          ctx.getDb());
     }
-    return json.create(ChangeJson.NO_OPTIONS).format(change.get());
   }
 
   @Override
@@ -147,26 +169,6 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
           && resource.getControl().canRestore());
   }
 
-  private ChangeMessage newMessage(RestoreInput input, IdentifiedUser caller,
-      Change change) throws OrmException {
-    StringBuilder msg = new StringBuilder();
-    msg.append("Restored");
-    if (!Strings.nullToEmpty(input.message).trim().isEmpty()) {
-      msg.append("\n\n");
-      msg.append(input.message.trim());
-    }
-
-    ChangeMessage message = new ChangeMessage(
-        new ChangeMessage.Key(
-            change.getId(),
-            ChangeUtil.messageUUID(dbProvider.get())),
-        caller.getAccountId(),
-        change.getLastUpdatedOn(),
-        change.currentPatchSetId());
-    message.setMessage(msg.toString());
-    return message;
-  }
-
   private static String status(Change change) {
     return change != null ? change.getStatus().name().toLowerCase() : "deleted";
   }

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

@@ -26,10 +26,12 @@ import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.index.ChangeIndexer;
 import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.project.ChangeControl;
+import com.google.gwtorm.server.OrmException;
 import com.google.inject.assistedinject.Assisted;
 import com.google.inject.assistedinject.AssistedInject;
 
@@ -47,7 +49,6 @@ import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.concurrent.Callable;
 
 /**
  * Context for a set of updates that should be applied for a site.
@@ -77,16 +78,74 @@ public class BatchUpdate implements AutoCloseable {
         Timestamp when);
   }
 
-  public abstract static class ChangeOp {
-    private final ChangeControl ctl;
-
-    public ChangeOp(ChangeControl ctl) {
-      this.ctl = ctl;
+  public class Context {
+    public Timestamp getWhen() {
+      return when;
     }
 
-    // TODO(dborowitz): Document that update contains the old change info.
-    public abstract void call(ReviewDb db, ChangeUpdate update)
-        throws Exception;
+    public ReviewDb getDb() {
+      return db;
+    }
+  }
+
+  public class RepoContext extends Context {
+    public Repository getRepository() throws IOException {
+      initRepository();
+      return repo;
+    }
+
+    public RevWalk getRevWalk() throws IOException {
+      initRepository();
+      return revWalk;
+    }
+
+    public ObjectInserter getInserter() throws IOException {
+      initRepository();
+      return inserter;
+    }
+
+    public BatchRefUpdate getBatchRefUpdate() throws IOException {
+      initRepository();
+      if (batchRefUpdate == null) {
+        batchRefUpdate = repo.getRefDatabase().newBatchUpdate();
+      }
+      return batchRefUpdate;
+    }
+  }
+
+  public class ChangeContext extends Context {
+    private final ChangeUpdate update;
+
+    private ChangeContext(ChangeUpdate update) {
+      this.update = update;
+    }
+
+    public ChangeUpdate getChangeUpdate() {
+      return update;
+    }
+
+    public Change readChange() throws OrmException {
+      return db.changes().get(update.getChange().getId());
+    }
+
+    public CurrentUser getUser() {
+      return update.getUser();
+    }
+  }
+
+  public static class Op {
+    @SuppressWarnings("unused")
+    public void updateRepo(RepoContext ctx) throws Exception {
+    }
+
+    @SuppressWarnings("unused")
+    public void updateChange(ChangeContext ctx) throws Exception {
+    }
+
+    // TODO(dborowitz): Support async operations?
+    @SuppressWarnings("unused")
+    public void postUpdate(Context ctx) throws Exception {
+    }
   }
 
   private final ReviewDb db;
@@ -98,10 +157,8 @@ public class BatchUpdate implements AutoCloseable {
   private final Project.NameKey project;
   private final Timestamp when;
 
-  private final ListMultimap<Change.Id, ChangeOp> changeOps =
-      ArrayListMultimap.create();
+  private final ListMultimap<Change.Id, Op> ops = ArrayListMultimap.create();
   private final Map<Change.Id, ChangeControl> changeControls = new HashMap<>();
-  private final List<Callable<?>> postOps = new ArrayList<>();
   private final List<CheckedFuture<?, IOException>> indexFutures =
       new ArrayList<>();
 
@@ -147,7 +204,7 @@ public class BatchUpdate implements AutoCloseable {
     return this;
   }
 
-  public Repository getRepository() throws IOException {
+  private void initRepository() throws IOException {
     if (repo == null) {
       this.repo = repoManager.openRepository(project);
       closeRepo = true;
@@ -155,49 +212,37 @@ public class BatchUpdate implements AutoCloseable {
       revWalk = new RevWalk(inserter.newReader());
       batchRefUpdate = repo.getRefDatabase().newBatchUpdate();
     }
+  }
+
+  public Repository getRepository() throws IOException {
+    initRepository();
     return repo;
   }
 
   public RevWalk getRevWalk() throws IOException {
-    if (revWalk == null) {
-      getRepository();
-    }
+    initRepository();
     return revWalk;
   }
 
   public ObjectInserter getObjectInserter() throws IOException {
-    if (inserter == null) {
-      getRepository();
-    }
+    initRepository();
     return inserter;
   }
 
-  public BatchRefUpdate getBatchRefUpdate() throws IOException {
-    if (batchRefUpdate == null) {
-      getRepository();
-    }
-    return batchRefUpdate;
-  }
-
   public BatchUpdate addRefUpdate(ReceiveCommand cmd) throws IOException {
-    getBatchRefUpdate().addCommand(cmd);
+    initRepository();
+    batchRefUpdate.addCommand(cmd);
     return this;
   }
 
-  public BatchUpdate addChangeOp(ChangeOp op) {
-    Change.Id id = op.ctl.getChange().getId();
+  public BatchUpdate addOp(ChangeControl ctl, Op op) {
+    Change.Id id = ctl.getChange().getId();
     ChangeControl old = changeControls.get(id);
     // TODO(dborowitz): Not sure this is guaranteed in general.
-    checkArgument(old == null || old == op.ctl,
+    checkArgument(old == null || old == ctl,
         "mismatched ChangeControls for change %s", id);
-    changeOps.put(id, op);
-    changeControls.put(id, op.ctl);
-    return this;
-  }
-
-  // TODO(dborowitz): Support async operations?
-  public BatchUpdate addPostOp(Callable<?> update) {
-    postOps.add(update);
+    ops.put(id, op);
+    changeControls.put(id, ctl);
     return this;
   }
 
@@ -229,6 +274,16 @@ public class BatchUpdate implements AutoCloseable {
   }
 
   private void executeRefUpdates() throws IOException, UpdateException {
+    try {
+      RepoContext ctx = new RepoContext();
+      for (Op op : ops.values()) {
+        op.updateRepo(ctx);
+      }
+    } catch (Exception e) {
+      Throwables.propagateIfPossible(e);
+      throw new UpdateException(e);
+    }
+
     if (repo == null || batchRefUpdate == null
         || batchRefUpdate.getCommands().isEmpty()) {
       return;
@@ -249,15 +304,14 @@ public class BatchUpdate implements AutoCloseable {
 
   private void executeChangeOps() throws UpdateException {
     try {
-      for (Map.Entry<Change.Id, Collection<ChangeOp>> e
-          : changeOps.asMap().entrySet()) {
+      for (Map.Entry<Change.Id, Collection<Op>> e : ops.asMap().entrySet()) {
         Change.Id id = e.getKey();
         ChangeUpdate update =
             changeUpdateFactory.create(changeControls.get(id), when);
         db.changes().beginTransaction(id);
         try {
-          for (ChangeOp op : e.getValue()) {
-            op.call(db, update);
+          for (Op op : e.getValue()) {
+            op.updateChange(new ChangeContext(update));
           }
           db.commit();
         } finally {
@@ -267,6 +321,7 @@ public class BatchUpdate implements AutoCloseable {
         indexFutures.add(indexer.indexAsync(id));
       }
     } catch (Exception e) {
+      Throwables.propagateIfPossible(e);
       throw new UpdateException(e);
     }
   }
@@ -276,8 +331,9 @@ public class BatchUpdate implements AutoCloseable {
   }
 
   private void executePostOps() throws Exception {
-    for (Callable<?> op : postOps) {
-      op.call();
+    Context ctx = new Context();
+    for (Op op : ops.values()) {
+      op.postUpdate(ctx);
     }
   }
 }