Pass RevWalk to PatchSetInfoFactory

Previously #get(RevCommit, PatchSet.Id) would NPE if the body of the
commit was not parsed; callers needed to remember to either parse the
body or ensure that the RevWalk that created the commit had
setRetainBody(true). In all cases, there is already a handy RevWalk;
pass this into get and do the parseBody there, such that it's
impossible for the caller to forget. (In the BatchUpdate.Op cases, we
need to shuffle call order around a bit so the PatchSetInfo gets
populated during updateRepo.)

Change-Id: I2e42ed1cc5c4a50958b6367051065e6bbad5cc54

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

@@ -78,6 +78,7 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
   private static final Logger log =
       LoggerFactory.getLogger(ChangeInserter.class);
 
+  private final PatchSetInfoFactory patchSetInfoFactory;
   private final ChangeHooks hooks;
   private final ApprovalsUtil approvalsUtil;
   private final ChangeMessagesUtil cmUtil;
@@ -92,7 +93,6 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
   private final Change change;
   private final PatchSet patchSet;
   private final RevCommit commit;
-  private final PatchSetInfo patchSetInfo;
 
   // Fields exposed as setters.
   private String message;
@@ -109,6 +109,7 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
 
   // Fields set during the insertion process.
   private ChangeMessage changeMessage;
+  private PatchSetInfo patchSetInfo;
 
   @Inject
   ChangeInserter(PatchSetInfoFactory patchSetInfoFactory,
@@ -130,6 +131,7 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
         "RefControl for %s,%s does not match change destination %s",
         projectName, refName, change.getDest());
 
+    this.patchSetInfoFactory = patchSetInfoFactory;
     this.hooks = hooks;
     this.approvalsUtil = approvalsUtil;
     this.cmUtil = cmUtil;
@@ -156,8 +158,6 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
     patchSet.setCreatedOn(change.getCreatedOn());
     patchSet.setUploader(change.getOwner());
     patchSet.setRevision(new RevId(commit.name()));
-    patchSetInfo = patchSetInfoFactory.get(commit, patchSet.getId());
-    change.setCurrentPatchSet(patchSetInfo);
   }
 
   private static IdentifiedUser checkUser(RefControl ctl) {
@@ -240,10 +240,6 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
     return this;
   }
 
-  public PatchSetInfo getPatchSetInfo() {
-    return patchSetInfo;
-  }
-
   public ChangeMessage getChangeMessage() {
     if (message == null) {
       return null;
@@ -257,6 +253,9 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
   public void updateRepo(RepoContext ctx)
       throws InvalidChangeOperationException, IOException {
     validate(ctx);
+    patchSetInfo = patchSetInfoFactory.get(
+        ctx.getRevWalk(), commit, patchSet.getId());
+    change.setCurrentPatchSet(patchSetInfo);
     if (!updateRef) {
       return;
     }

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

@@ -111,6 +111,7 @@ public class PatchSetInserter extends BatchUpdate.Op {
   // Fields set during some phase of BatchUpdate.Op.
   private Change change;
   private PatchSet patchSet;
+  private PatchSetInfo patchSetInfo;
   private ChangeMessage changeMessage;
   private SetMultimap<ReviewerState, Account.Id> oldReviewers;
 
@@ -213,6 +214,7 @@ public class PatchSetInserter extends BatchUpdate.Op {
       throws InvalidChangeOperationException, IOException {
     init();
     validate();
+    patchSetInfo = patchSetInfoFactory.get(ctx.getRevWalk(), commit, psId);
     ctx.addRefUpdate(new ReceiveCommand(ObjectId.zeroId(),
         commit, getPatchSetId().toRefName(), ReceiveCommand.Type.CREATE));
   }
@@ -268,7 +270,7 @@ public class PatchSetInserter extends BatchUpdate.Op {
         if (change.getStatus() != Change.Status.DRAFT && !allowClosed) {
           change.setStatus(Change.Status.NEW);
         }
-        change.setCurrentPatchSet(patchSetInfoFactory.get(commit, psId));
+        change.setCurrentPatchSet(patchSetInfo);
         ChangeUtil.updated(change);
         return change;
       }
@@ -288,11 +290,10 @@ public class PatchSetInserter extends BatchUpdate.Op {
   public void postUpdate(Context ctx) throws OrmException {
     if (sendMail) {
       try {
-        PatchSetInfo info = patchSetInfoFactory.get(commit, psId);
         ReplacePatchSetSender cm = replacePatchSetFactory.create(
             change.getId());
         cm.setFrom(user.getAccountId());
-        cm.setPatchSet(patchSet, info);
+        cm.setPatchSet(patchSet, patchSetInfo);
         cm.setChangeMessage(changeMessage);
         cm.addReviewers(oldReviewers.get(ReviewerState.REVIEWER));
         cm.addExtraCC(oldReviewers.get(ReviewerState.CC));

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

@@ -2102,7 +2102,7 @@ public class ReceiveCommits {
               newPatchSet.getId()));
     }
 
-    private void newPatchSet() {
+    private void newPatchSet() throws IOException {
       PatchSet.Id id =
           ChangeUtil.nextPatchSetId(allRefs, change.currentPatchSetId());
       newPatchSet = new PatchSet(id);
@@ -2117,7 +2117,8 @@ public class ReceiveCommits {
       if (magicBranch != null && magicBranch.draft) {
         newPatchSet.setDraft(true);
       }
-      info = patchSetInfoFactory.get(newCommit, newPatchSet.getId());
+      info = patchSetInfoFactory.get(
+          rp.getRevWalk(), newCommit, newPatchSet.getId());
       cmd = new ReceiveCommand(
           ObjectId.zeroId(),
           newCommit,
@@ -2698,7 +2699,7 @@ public class ReceiveCommits {
     result.change = change;
     result.changeCtl = projectControl.controlFor(change);
     result.newPatchSet = ps;
-    result.info = patchSetInfoFactory.get(commit, psi);
+    result.info = patchSetInfoFactory.get(rp.getRevWalk(), commit, psi);
     result.mergedIntoRef = refName;
     markChangeMergedByPush(db, result, result.changeCtl);
     hooks.doChangeMergedHook(

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java

@@ -21,6 +21,7 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetAncestor;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ChangeUtil;
@@ -134,6 +135,7 @@ public class CherryPick extends SubmitStrategy {
 
     private PatchSet.Id psId;
     private CodeReviewCommit newCommit;
+    private PatchSetInfo patchSetInfo;
 
     private CherryPickOneOp(MergeTip mergeTip, CodeReviewCommit n) {
       this.mergeTip = mergeTip;
@@ -159,6 +161,8 @@ public class CherryPick extends SubmitStrategy {
             committer, cherryPickCmtMsg, args.rw);
         ctx.addRefUpdate(
             new ReceiveCommand(ObjectId.zeroId(), newCommit, psId.toRefName()));
+        patchSetInfo =
+            patchSetInfoFactory.get(ctx.getRevWalk(), newCommit, psId);
       } catch (MergeConflictException mce) {
         // Keep going in the case of a single merge failure; the goal is to
         // cherry-pick as many commits as possible.
@@ -184,7 +188,7 @@ public class CherryPick extends SubmitStrategy {
       ps.setGroups(GroupCollector.getCurrentGroups(args.db, c));
       args.db.patchSets().insert(Collections.singleton(ps));
       insertAncestors(args.db, ps.getId(), newCommit);
-      c.setCurrentPatchSet(patchSetInfoFactory.get(newCommit, ps.getId()));
+      c.setCurrentPatchSet(patchSetInfo);
       args.db.changes().update(Collections.singletonList(c));
 
       List<PatchSetApproval> approvals = Lists.newArrayList();

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java

@@ -104,7 +104,7 @@ public class RebaseIfNecessary extends SubmitStrategy {
                 ObjectId.fromString(newPatchSet.getRevision().get()));
             mergeTip.moveTipTo(newTip, newTip);
             n.change().setCurrentPatchSet(
-                patchSetInfoFactory.get(mergeTip.getCurrentTip(),
+                patchSetInfoFactory.get(args.rw, mergeTip.getCurrentTip(),
                     newPatchSet.getId()));
             mergeTip.getCurrentTip().copyFrom(n);
             mergeTip.getCurrentTip().setControl(

gerrit-server/src/main/java/com/google/gerrit/server/mail/PatchSetNotificationSender.java

@@ -84,11 +84,12 @@ public class PatchSetNotificationSender {
       throws OrmException, IOException {
     try (Repository git = repoManager.openRepository(updatedChange.getProject())) {
       final RevCommit commit;
-      try (RevWalk revWalk = new RevWalk(git)) {
+      final PatchSetInfo info;
-        commit = revWalk.parseCommit(ObjectId.fromString(
+      try (RevWalk rw = new RevWalk(git)) {
+        commit = rw.parseCommit(ObjectId.fromString(
             updatedPatchSet.getRevision().get()));
+        info = patchSetInfoFactory.get(rw, commit, updatedPatchSet.getId());
       }
-      final PatchSetInfo info = patchSetInfoFactory.get(commit, updatedPatchSet.getId());
       final List<FooterLine> footerLines = commit.getFooterLines();
       final Account.Id me = currentUser.getAccountId();
       final MailRecipients recipients =

gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java

@@ -56,7 +56,9 @@ public class PatchSetInfoFactory {
     this.byEmailCache = byEmailCache;
   }
 
-  public PatchSetInfo get(RevCommit src, PatchSet.Id psi) {
+  public PatchSetInfo get(RevWalk rw, RevCommit src, PatchSet.Id psi)
+      throws IOException {
+    rw.parseBody(src);
     PatchSetInfo info = new PatchSetInfo(psi);
     info.setSubject(src.getShortMessage());
     info.setMessage(src.getFullMessage());
@@ -83,7 +85,7 @@ public class PatchSetInfoFactory {
         RevWalk rw = new RevWalk(repo)) {
       final RevCommit src =
           rw.parseCommit(ObjectId.fromString(patchSet.getRevision().get()));
-      PatchSetInfo info = get(src, patchSet.getId());
+      PatchSetInfo info = get(rw, src, patchSet.getId());
       info.setParents(toParentInfos(src.getParents(), rw));
       return info;
     } catch (IOException e) {