BatchUpdate: Bump lastUpdatedOn by default

There are only two cases where we don't currently bump lastUpdatedOn:
adding reviewers and adding draft comments. The vast majority of other
cases we do want to bump lastUpdatedOn.

Change the ChangeContext#saveChange method to #bumpLastUpdatedOn, to
be explicit about what this actually does. Default to true to make it
easier on callers.

Change-Id: I3d8b13f70b082f8f4ef15df02f72840ed42eed85

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java

@@ -692,6 +692,7 @@ public class GetRelatedIT extends AbstractDaemonTest {
           PatchSet ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
           psUtil.setGroups(ctx.getDb(), ctx.getUpdate(psId), ps,
               ImmutableList.<String> of());
+          ctx.bumpLastUpdatedOn(false);
           return true;
         }
       });

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

@@ -132,7 +132,6 @@ public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
       patchSet = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
       change.setStatus(Change.Status.ABANDONED);
       change.setLastUpdatedOn(ctx.getWhen());
-      ctx.saveChange();
 
       update.setStatus(change.getStatus());
       message = newMessage(ctx);

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

@@ -332,7 +332,6 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
     }
     patchSet = psUtil.insert(ctx.getDb(), ctx.getRevWalk(), update, psId,
         commit, draft, newGroups, null);
-    ctx.saveChange();
 
     /* TODO: fixStatus is used here because the tests
      * (byStatusClosed() in AbstractQueryChangesTest)

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

@@ -286,7 +286,6 @@ public class CherryPickChange {
       changeMessage.setMessage(sb.toString());
 
       cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(psId), changeMessage);
-      ctx.saveChange(); // Bump lastUpdatedOn to match message.
       return true;
     }
   }

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

@@ -127,6 +127,7 @@ public class CreateDraftComment implements RestModifyView<RevisionResource, Draf
           comment, patchListCache, ctx.getChange(), ps);
       plcUtil.putComments(
           ctx.getDb(), ctx.getUpdate(psId), Collections.singleton(comment));
+      ctx.bumpLastUpdatedOn(false);
       return true;
     }
   }

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

@@ -160,7 +160,6 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
       if (c.currentPatchSetId().equals(psId)) {
         c.setCurrentPatchSet(previousPatchSetInfo(ctx));
       }
-      ctx.saveChange();
     }
 
     private boolean deletedOnlyPatchSet() {

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

@@ -168,7 +168,6 @@ public class DeleteVote implements RestModifyView<VoteResource, Input> {
       if (psa == null) {
         throw new ResourceNotFoundException();
       }
-      ctx.saveChange();
       ctx.getDb().patchSetApprovals().update(Collections.singleton(psa));
 
       if (msg.length() > 0) {

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

@@ -186,7 +186,6 @@ public class Move implements RestModifyView<ChangeResource, MoveInput> {
       cmsg.setMessage(msgBuf.toString());
       cmUtil.addChangeMessage(ctx.getDb(), update, cmsg);
 
-      ctx.saveChange();
       return true;
     }
   }

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

@@ -246,7 +246,6 @@ public class PatchSetInserter extends BatchUpdate.Op {
       change.setStatus(Change.Status.NEW);
     }
     change.setCurrentPatchSet(patchSetInfo);
-    ctx.saveChange();
     if (copyApprovals) {
       approvalCopier.copy(db, ctl, patchSet);
     }

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

@@ -370,9 +370,6 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
       dirty |= insertComments(ctx);
       dirty |= updateLabels(ctx);
       dirty |= insertMessage(ctx);
-      if (dirty) {
-        ctx.saveChange();
-      }
       return dirty;
     }
 

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

@@ -275,10 +275,12 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
               rsrc.getChange(),
               reviewers.keySet());
 
-      if (!added.isEmpty()) {
+      if (added.isEmpty()) {
-        patchSet = psUtil.current(dbProvider.get(), rsrc.getNotes());
+        return false;
       }
-      return !added.isEmpty();
+      patchSet = psUtil.current(dbProvider.get(), rsrc.getNotes());
+      ctx.bumpLastUpdatedOn(false);
+      return true;
     }
 
     @Override

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

@@ -192,7 +192,6 @@ public class PublishDraftPatchSet implements RestModifyView<RevisionResource, In
       if (wasDraftChange) {
         change.setStatus(Change.Status.NEW);
         update.setStatus(change.getStatus());
-        ctx.saveChange();
       }
     }
 
@@ -202,10 +201,6 @@ public class PublishDraftPatchSet implements RestModifyView<RevisionResource, In
         throw new ResourceConflictException("Patch set is not a draft");
       }
       psUtil.publish(ctx.getDb(), ctx.getUpdate(psId), patchSet);
-      // Force ETag invalidation if not done already
-      if (!wasDraftChange) {
-        ctx.saveChange();
-      }
     }
 
     private void addReviewers(ChangeContext ctx)

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

@@ -155,6 +155,7 @@ public class PutDraftComment implements RestModifyView<DraftCommentResource, Dra
         plcUtil.putComments(ctx.getDb(), update,
             Collections.singleton(update(comment, in, ctx.getWhen())));
       }
+      ctx.bumpLastUpdatedOn(false);
       return true;
     }
   }

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

@@ -114,7 +114,6 @@ public class PutTopic implements RestModifyView<ChangeResource, Input>,
       }
       change.setTopic(Strings.emptyToNull(newTopicName));
       update.setTopic(change.getTopic());
-      ctx.saveChange();
 
       ChangeMessage cmsg = new ChangeMessage(
           new ChangeMessage.Key(

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

@@ -117,7 +117,6 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
       patchSet = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
       change.setStatus(Status.NEW);
       change.setLastUpdatedOn(ctx.getWhen());
-      ctx.saveChange();
       update.setStatus(change.getStatus());
 
       message = newMessage(ctx);

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

@@ -179,7 +179,7 @@ public class BatchUpdate implements AutoCloseable {
     private final ReviewDbWrapper dbWrapper;
 
     private boolean deleted;
-    private boolean saved;
+    private boolean bumpLastUpdatedOn = true;
 
     private ChangeContext(ChangeControl ctl, ReviewDbWrapper dbWrapper) {
       this.ctl = ctl;
@@ -223,13 +223,11 @@ public class BatchUpdate implements AutoCloseable {
       return c;
     }
 
-    public void saveChange() {
+    public void bumpLastUpdatedOn(boolean bump) {
-      checkState(!deleted, "cannot both save and delete change");
+      bumpLastUpdatedOn = bump;
-      saved = true;
     }
 
     public void deleteChange() {
-      checkState(!saved, "cannot both save and delete change");
       deleted = true;
     }
   }
@@ -581,14 +579,14 @@ public class BatchUpdate implements AutoCloseable {
           }
 
           // Bump lastUpdatedOn or rowVersion and commit.
+          Iterable<Change> cs = changesToUpdate(ctx);
           if (newChanges.containsKey(id)) {
-            db.changes().insert(bumpLastUpdatedOn(ctx));
+            // Insert rather than upsert in case of a race on change IDs.
-          } else if (ctx.saved) {
+            db.changes().insert(cs);
-            db.changes().update(bumpLastUpdatedOn(ctx));
           } else if (ctx.deleted) {
-            db.changes().delete(bumpLastUpdatedOn(ctx));
+            db.changes().delete(cs);
           } else {
-            db.changes().update(bumpRowVersionNotLastUpdatedOn(ctx));
+            db.changes().update(cs);
           }
           db.commit();
         } finally {
@@ -618,19 +616,14 @@ public class BatchUpdate implements AutoCloseable {
     }
   }
 
-  private static Iterable<Change> bumpLastUpdatedOn(ChangeContext ctx) {
+  private static Iterable<Change> changesToUpdate(ChangeContext ctx) {
     Change c = ctx.getChange();
-    if (c.getLastUpdatedOn().before(ctx.getWhen())) {
+    if (ctx.bumpLastUpdatedOn && c.getLastUpdatedOn().before(ctx.getWhen())) {
       c.setLastUpdatedOn(ctx.getWhen());
     }
     return Collections.singleton(c);
   }
 
-  private static Iterable<Change> bumpRowVersionNotLastUpdatedOn(
-      ChangeContext ctx) {
-    return Collections.singleton(ctx.getChange());
-  }
-
   private ChangeContext newChangeContext(Change.Id id) throws Exception {
     Change c = newChanges.get(id);
     if (c == null) {

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

@@ -776,7 +776,6 @@ public class MergeOp implements AutoCloseable {
               cmUtil.addChangeMessage(ctx.getDb(),
                   ctx.getUpdate(change.currentPatchSetId()), msg);
 
-              ctx.saveChange();
               return true;
             }
           });

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

@@ -2516,7 +2516,6 @@ public class ReceiveCommits {
           if (change.getStatus().isOpen()) {
             change.setCurrentPatchSet(info);
             change.setStatus(Change.Status.MERGED);
-            ctx.saveChange();
 
             // we cannot reconstruct the submit records for when this change was
             // submitted, this is why we must fix the status

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

@@ -263,7 +263,6 @@ public class ReplaceOp extends BatchUpdate.Op {
     if (mergedIntoRef == null) {
       resetChange(ctx, msg);
     }
-    ctx.saveChange();
 
     return true;
   }

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

@@ -160,7 +160,6 @@ public class CherryPick extends SubmitStrategy {
           prevPs != null ? prevPs.getGroups() : ImmutableList.<String> of(),
           null);
       ctx.getChange().setCurrentPatchSet(patchSetInfo);
-      ctx.saveChange();
 
       // Don't copy approvals, as this is already taken care of by
       // SubmitStrategyOp.

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

@@ -484,7 +484,6 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op {
     logDebug("Setting change {} merged", c.getId());
     c.setStatus(Change.Status.MERGED);
     c.setSubmissionId(args.submissionId);
-    ctx.saveChange();
 
     // TODO(dborowitz): We need to be able to change the author of the message,
     // which is not the user from the update context. addMergedMessage was able