Make PatchSetInserter a BatchUpdate.Op

Callers that already have a BatchUpdate can just add the inserter
directly, rather than attempting to use the alternate factory method
removed a few commits ago. The code within PatchSetInserter is much
cleaner this way. (There are no such callers yet.)

Change-Id: If267dcf1236ee1cd41681f12f5aa53834eda4013

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

@@ -31,6 +31,7 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.events.CommitReceivedEvent;
+import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.CodeReviewCommit;
 import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
 import com.google.gerrit.server.git.GitRepositoryManager;
@@ -86,6 +87,7 @@ public class CherryPickChange {
   private final MergeUtil.Factory mergeUtilFactory;
   private final ChangeMessagesUtil changeMessagesUtil;
   private final ChangeUpdate.Factory updateFactory;
+  private final BatchUpdate.Factory batchUpdateFactory;
 
   @Inject
   CherryPickChange(Provider<ReviewDb> db,
@@ -98,7 +100,8 @@ public class CherryPickChange {
       PatchSetInserter.Factory patchSetInserterFactory,
       MergeUtil.Factory mergeUtilFactory,
       ChangeMessagesUtil changeMessagesUtil,
-      ChangeUpdate.Factory updateFactory) {
+      ChangeUpdate.Factory updateFactory,
+      BatchUpdate.Factory batchUpdateFactory) {
     this.db = db;
     this.queryProvider = queryProvider;
     this.gitManager = gitManager;
@@ -110,6 +113,7 @@ public class CherryPickChange {
     this.mergeUtilFactory = mergeUtilFactory;
     this.changeMessagesUtil = changeMessagesUtil;
     this.updateFactory = updateFactory;
+    this.batchUpdateFactory = batchUpdateFactory;
   }
 
   public Change.Id cherryPick(Change change, PatchSet patch,
@@ -210,23 +214,26 @@ public class CherryPickChange {
     }
   }
 
-  private Change.Id insertPatchSet(Repository git, RevWalk revWalk, Change change,
-      CodeReviewCommit cherryPickCommit, RefControl refControl,
+  private Change.Id insertPatchSet(Repository git, RevWalk revWalk,
+      Change change, CodeReviewCommit cherryPickCommit, RefControl refControl,
       IdentifiedUser identifiedUser)
-      throws InvalidChangeOperationException, IOException, OrmException,
-      UpdateException, RestApiException {
+      throws IOException, OrmException, UpdateException, RestApiException {
     final ChangeControl changeControl =
         refControl.getProjectControl().controlFor(change);
     final PatchSetInserter inserter = patchSetInserterFactory
         .create(git, revWalk, changeControl, cherryPickCommit);
     final PatchSet.Id newPatchSetId = inserter.getPatchSetId();
     PatchSet current = db.get().patchSets().get(change.currentPatchSetId());
-    inserter
+
+    try (BatchUpdate bu = batchUpdateFactory.create(
+        db.get(), change.getDest().getParentKey(), TimeUtil.nowTs())) {
+      bu.addOp(changeControl, inserter
           .setMessage("Uploaded patch set " + newPatchSetId.get() + ".")
           .setDraft(current.isDraft())
           .setUploader(identifiedUser.getAccountId())
-      .setSendMail(false)
-      .insert();
+          .setSendMail(false));
+      bu.execute();
+    }
     return change.getId();
   }
 

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

@@ -28,6 +28,7 @@ import com.google.common.collect.Multimap;
 import com.google.common.collect.MultimapBuilder;
 import com.google.gerrit.common.FooterConstants;
 import com.google.gerrit.common.Nullable;
+import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.api.changes.FixInput;
 import com.google.gerrit.extensions.common.ProblemInfo;
 import com.google.gerrit.extensions.common.ProblemInfo.Status;
@@ -42,12 +43,12 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.change.PatchSetInserter.ValidatePolicy;
+import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.project.InvalidChangeOperationException;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gwtorm.server.AtomicUpdate;
@@ -111,6 +112,7 @@ public class ConsistencyChecker {
   private final ChangeControl.GenericFactory changeControlFactory;
   private final PatchSetInfoFactory patchSetInfoFactory;
   private final PatchSetInserter.Factory patchSetInserterFactory;
+  private final BatchUpdate.Factory updateFactory;
 
   private FixInput fix;
   private Change change;
@@ -131,7 +133,8 @@ public class ConsistencyChecker {
       @GerritPersonIdent Provider<PersonIdent> serverIdent,
       ChangeControl.GenericFactory changeControlFactory,
       PatchSetInfoFactory patchSetInfoFactory,
-      PatchSetInserter.Factory patchSetInserterFactory) {
+      PatchSetInserter.Factory patchSetInserterFactory,
+      BatchUpdate.Factory updateFactory) {
     this.db = db;
     this.repoManager = repoManager;
     this.user = user;
@@ -139,6 +142,7 @@ public class ConsistencyChecker {
     this.changeControlFactory = changeControlFactory;
     this.patchSetInfoFactory = patchSetInfoFactory;
     this.patchSetInserterFactory = patchSetInserterFactory;
+    this.updateFactory = updateFactory;
     reset();
   }
 
@@ -461,21 +465,24 @@ public class ConsistencyChecker {
       ChangeControl ctl = changeControlFactory.controlFor(change, user.get());
       PatchSetInserter inserter =
           patchSetInserterFactory.create(repo, rw, ctl, commit);
-      change = inserter.setValidatePolicy(ValidatePolicy.NONE)
+      try (BatchUpdate bu = updateFactory.create(
+          db.get(), change.getDest().getParentKey(), TimeUtil.nowTs())) {
+        bu.addOp(ctl, inserter.setValidatePolicy(ValidatePolicy.NONE)
             .setRunHooks(false)
             .setSendMail(false)
             .setAllowClosed(true)
             .setUploader(((IdentifiedUser) user.get()).getAccountId())
             // TODO: fix setMessage to work without init()
             .setMessage(
-              "Patch set for merged commit inserted by consistency checker")
-          .insert();
+                "Patch set for merged commit inserted by consistency checker"));
+        bu.execute();
+      }
+      change = inserter.getChange();
       PatchSet.Id psId = change.currentPatchSetId();
       p.status = Status.FIXED;
       p.outcome = "Inserted as patch set " + psId.get();
       return psId;
-    } catch (InvalidChangeOperationException | IOException
-        | NoSuchChangeException | UpdateException | RestApiException e) {
+    } catch (NoSuchChangeException | UpdateException | RestApiException e) {
       warn(e);
       p.status = Status.FIX_FAILED;
       p.outcome = "Error inserting new patch set";

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

@@ -17,11 +17,10 @@ package com.google.gerrit.server.change;
 import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.collect.SetMultimap;
 import com.google.gerrit.common.ChangeHooks;
-import com.google.gerrit.common.TimeUtil;
-import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -41,7 +40,6 @@ 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.GroupCollector;
-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;
@@ -69,7 +67,7 @@ import org.slf4j.LoggerFactory;
 import java.io.IOException;
 import java.util.Collections;
 
-public class PatchSetInserter {
+public class PatchSetInserter extends BatchUpdate.Op {
   private static final Logger log =
       LoggerFactory.getLogger(PatchSetInserter.class);
 
@@ -91,7 +89,6 @@ public class PatchSetInserter {
   private final ChangeHooks hooks;
   private final PatchSetInfoFactory patchSetInfoFactory;
   private final ReviewDb db;
-  private final BatchUpdate.Factory batchUpdateFactory;
   private final CommitValidators.Factory commitValidatorsFactory;
   private final ReplacePatchSetSender.Factory replacePatchSetFactory;
   private final ApprovalsUtil approvalsUtil;
@@ -119,10 +116,15 @@ public class PatchSetInserter {
   private Account.Id uploader;
   private boolean allowClosed;
 
+  // Fields set during some phase of BatchUpdate.Op.
+  private Change change;
+  private PatchSet patchSet;
+  private ChangeMessage changeMessage;
+  private SetMultimap<ReviewerState, Account.Id> oldReviewers;
+
   @AssistedInject
   public PatchSetInserter(ChangeHooks hooks,
       ReviewDb db,
-      BatchUpdate.Factory batchUpdateFactory,
       ApprovalsUtil approvalsUtil,
       ApprovalCopier approvalCopier,
       ChangeMessagesUtil cmUtil,
@@ -135,7 +137,6 @@ public class PatchSetInserter {
       @Assisted RevCommit commit) {
     this.hooks = hooks;
     this.db = db;
-    this.batchUpdateFactory = batchUpdateFactory;
     this.approvalsUtil = approvalsUtil;
     this.approvalCopier = approvalCopier;
     this.cmUtil = cmUtil;
@@ -210,28 +211,16 @@ public class PatchSetInserter {
     return this;
   }
 
-  public Change insert() throws InvalidChangeOperationException, IOException,
-       UpdateException, RestApiException {
-    init();
-    validate();
-
-    Op op = new Op();
-    try (BatchUpdate bu = batchUpdateFactory.create(
-          db, ctl.getChange().getProject(), TimeUtil.nowTs())) {
-      bu.addOp(ctl, op);
-      bu.execute();
+  public Change getChange() {
+    checkState(change != null, "getChange() only valid after executing update");
+    return change;
   }
-    return op.change;
-  }
-
-  private class Op extends BatchUpdate.Op {
-    private Change change;
-    private PatchSet patchSet;
-    private ChangeMessage changeMessage;
-    private SetMultimap<ReviewerState, Account.Id> oldReviewers;
 
   @Override
-    public void updateRepo(RepoContext ctx) throws IOException {
+  public void updateRepo(RepoContext ctx)
+      throws InvalidChangeOperationException, IOException {
+    init();
+    validate();
     ctx.addRefUpdate(new ReceiveCommand(ObjectId.zeroId(),
         commit, getPatchSetId().toRefName(), ReceiveCommand.Type.CREATE));
   }
@@ -326,7 +315,6 @@ public class PatchSetInserter {
       hooks.doPatchsetCreatedHook(change, patchSet, ctx.getDb());
     }
   }
-  }
 
   private void init() {
     if (sshInfo == null) {

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

@@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.change.PatchSetInserter.ValidatePolicy;
+import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MergeConflictException;
 import com.google.gerrit.server.git.MergeUtil;
@@ -64,6 +65,7 @@ public class RebaseChange {
   private final TimeZone serverTimeZone;
   private final MergeUtil.Factory mergeUtilFactory;
   private final PatchSetInserter.Factory patchSetInserterFactory;
+  private final BatchUpdate.Factory updateFactory;
 
   @Inject
   RebaseChange(ChangeControl.GenericFactory changeControlFactory,
@@ -71,13 +73,15 @@ public class RebaseChange {
       @GerritPersonIdent PersonIdent myIdent,
       GitRepositoryManager gitManager,
       MergeUtil.Factory mergeUtilFactory,
-      PatchSetInserter.Factory patchSetInserterFactory) {
+      PatchSetInserter.Factory patchSetInserterFactory,
+      BatchUpdate.Factory updateFactory) {
     this.changeControlFactory = changeControlFactory;
     this.db = db;
     this.gitManager = gitManager;
     this.serverTimeZone = myIdent.getTimeZone();
     this.mergeUtilFactory = mergeUtilFactory;
     this.patchSetInserterFactory = patchSetInserterFactory;
+    this.updateFactory = updateFactory;
   }
 
   /**
@@ -282,12 +286,15 @@ public class RebaseChange {
         .setSendMail(false)
         .setRunHooks(runHooks);
 
-    Change newChange = patchSetInserter
-        .setMessage("Patch Set " + patchSetInserter.getPatchSetId().get()
-          + ": Patch Set " + patchSetId.get() + " was rebased")
-        .insert();
+    try (BatchUpdate bu = updateFactory.create(
+        db.get(), change.getDest().getParentKey(), TimeUtil.nowTs())) {
+      bu.addOp(changeControl, patchSetInserter.setMessage(
+          "Patch Set " + patchSetInserter.getPatchSetId().get()
+          + ": Patch Set " + patchSetId.get() + " was rebased"));
+      bu.execute();
+    }
 
-    return db.get().patchSets().get(newChange.currentPatchSetId());
+    return db.get().patchSets().get(patchSetInserter.getPatchSetId());
   }
 
   /**

gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.edit;
 import static com.google.common.base.Preconditions.checkArgument;
 
 import com.google.common.base.Optional;
+import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.RestApiException;
@@ -30,11 +31,11 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.change.ChangeKind;
 import com.google.gerrit.server.change.ChangeKindCache;
 import com.google.gerrit.server.change.PatchSetInserter;
+import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.index.ChangeIndexer;
 import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.project.InvalidChangeOperationException;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectState;
@@ -70,6 +71,7 @@ public class ChangeEditUtil {
   private final Provider<ReviewDb> db;
   private final Provider<CurrentUser> user;
   private final ChangeKindCache changeKindCache;
+  private final BatchUpdate.Factory updateFactory;
 
   @Inject
   ChangeEditUtil(GitRepositoryManager gitManager,
@@ -79,7 +81,8 @@ public class ChangeEditUtil {
       ProjectCache projectCache,
       Provider<ReviewDb> db,
       Provider<CurrentUser> user,
-      ChangeKindCache changeKindCache) {
+      ChangeKindCache changeKindCache,
+      BatchUpdate.Factory updateFactory) {
     this.gitManager = gitManager;
     this.patchSetInserterFactory = patchSetInserterFactory;
     this.changeControlFactory = changeControlFactory;
@@ -88,6 +91,7 @@ public class ChangeEditUtil {
     this.db = db;
     this.user = user;
     this.changeKindCache = changeKindCache;
+    this.updateFactory = updateFactory;
   }
 
   /**
@@ -161,16 +165,12 @@ public class ChangeEditUtil {
             "only edit for current patch set can be published");
       }
 
-      try {
       Change updatedChange =
           insertPatchSet(edit, change, repo, rw, basePatchSet,
               squashEdit(rw, inserter, edit.getEditCommit(), basePatchSet));
       // TODO(davido): This should happen in the same BatchRefUpdate.
       deleteRef(repo, edit);
       indexer.index(db.get(), updatedChange);
-      } catch (InvalidChangeOperationException e) {
-        throw new ResourceConflictException(e.getMessage());
-      }
     }
   }
 
@@ -217,11 +217,10 @@ public class ChangeEditUtil {
   private Change insertPatchSet(ChangeEdit edit, Change change,
       Repository repo, RevWalk rw, PatchSet basePatchSet, RevCommit squashed)
       throws NoSuchChangeException, RestApiException, UpdateException,
-      InvalidChangeOperationException, IOException {
+      IOException {
+    ChangeControl ctl = changeControlFactory.controlFor(change, edit.getUser());
     PatchSetInserter inserter =
-        patchSetInserterFactory.create(repo, rw,
-            changeControlFactory.controlFor(change, edit.getUser()),
-            squashed);
+        patchSetInserterFactory.create(repo, rw, ctl, squashed);
 
     StringBuilder message = new StringBuilder("Patch set ")
       .append(inserter.getPatchSetId().get())
@@ -239,11 +238,16 @@ public class ChangeEditUtil {
         .append(".");
     }
 
-    return inserter
+    try (BatchUpdate bu = updateFactory.create(
+        db.get(), change.getDest().getParentKey(), TimeUtil.nowTs())) {
+      bu.addOp(ctl, inserter
         .setDraft(change.getStatus() == Status.DRAFT ||
             basePatchSet.isDraft())
-        .setMessage(message.toString())
-        .insert();
+        .setMessage(message.toString()));
+      bu.execute();
+    }
+
+    return inserter.getChange();
   }
 
   private static void deleteRef(Repository repo, ChangeEdit edit)

gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java

@@ -54,6 +54,7 @@ import com.google.gerrit.server.change.ChangeInserter;
 import com.google.gerrit.server.change.ChangeTriplet;
 import com.google.gerrit.server.change.PatchSetInserter;
 import com.google.gerrit.server.change.PatchSetInserter.ValidatePolicy;
+import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.index.IndexCollection;
 import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.project.ChangeControl;
@@ -113,6 +114,7 @@ public abstract class AbstractQueryChangesTest {
 
   @ConfigSuite.Parameter public Config config;
   @Inject protected AccountManager accountManager;
+  @Inject protected BatchUpdate.Factory updateFactory;
   @Inject protected ChangeInserter.Factory changeFactory;
   @Inject protected PatchSetInserter.Factory patchSetFactory;
   @Inject protected ChangeControl.GenericFactory changeControlFactory;
@@ -1326,12 +1328,19 @@ public abstract class AbstractQueryChangesTest {
             .add("file" + n, "contents " + n)
             .create());
     ChangeControl ctl = changeControlFactory.controlFor(c.getId(), user);
-    return patchSetFactory.create(
+
+    PatchSetInserter inserter = patchSetFactory.create(
           repo.getRepository(), repo.getRevWalk(), ctl, commit)
         .setSendMail(false)
         .setRunHooks(false)
-        .setValidatePolicy(ValidatePolicy.NONE)
-        .insert();
+        .setValidatePolicy(ValidatePolicy.NONE);
+    try (BatchUpdate bu = updateFactory.create(
+        db, c.getDest().getParentKey(), TimeUtil.nowTs())) {
+      bu.addOp(ctl, inserter);
+      bu.execute();
+    }
+
+    return inserter.getChange();
   }
 
   protected void assertBadQuery(Object query) throws Exception {