Fix rebase patch set and revert change to update Git first

Like ReceiveCommits, update the Git reference before writing to
the database. This way the repository cannot be corrupted if the
server goes down between the two actions.

Change-Id: I8c1d70d65a1f3b121d4bdd8a0cc649f553eabfab

1 files changed, 144 insertions, 107 deletions

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
index 3868710301..94b216985a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server;
 
+import com.google.common.collect.Sets;
 import com.google.gerrit.common.ChangeHookRunner;
 import com.google.gerrit.common.ChangeHooks;
 import com.google.gerrit.reviewdb.client.Account;
@@ -38,7 +39,6 @@ import com.google.gerrit.server.mail.ReplacePatchSetSender;
 import com.google.gerrit.server.mail.ReplyToChangeSender;
 import com.google.gerrit.server.mail.RevertedSender;
 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;
@@ -68,6 +68,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
+import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -196,13 +197,15 @@ public class ChangeUtil {
    * Rebases a commit
    *
    * @param git Repository to find commits in
+   * @param inserter inserter to handle new trees and blobs.
    * @param original The commit to rebase
    * @param base Base to rebase against
    * @return CommitBuilder the newly rebased commit
    * @throws IOException Merged failed
    */
-  public static CommitBuilder rebaseCommit(Repository git, RevCommit original,
-      RevCommit base, PersonIdent committerIdent) throws IOException {
+  public static CommitBuilder rebaseCommit(Repository git,
+      final ObjectInserter inserter, RevCommit original, RevCommit base,
+      PersonIdent committerIdent) throws IOException {
 
     if (original.getParentCount() == 0) {
       throw new IOException(
@@ -222,6 +225,20 @@ public class ChangeUtil {
     }
 
     final ThreeWayMerger merger = MergeStrategy.RESOLVE.newMerger(git, true);
+    merger.setObjectInserter(new ObjectInserter.Filter() {
+      @Override
+      protected ObjectInserter delegate() {
+        return inserter;
+      }
+
+      @Override
+      public void flush() {
+      }
+
+      @Override
+      public void release() {
+      }
+    });
     merger.setBase(parentCommit);
     merger.merge(original, base);
 
@@ -251,7 +268,7 @@ public class ChangeUtil {
       final ApprovalsUtil approvalsUtil) throws NoSuchChangeException,
       EmailException, OrmException, MissingObjectException,
       IncorrectObjectTypeException, IOException,
-      PatchSetInfoNotAvailableException, InvalidChangeOperationException {
+      InvalidChangeOperationException {
 
     final Change.Id changeId = patchSetId.getParentKey();
     final ChangeControl changeControl =
@@ -319,104 +336,118 @@ public class ChangeUtil {
           branchTipCommit = revWalk.parseCommit(destRef.getObjectId());
         }
 
-        final RevCommit originalCommit =
-            revWalk.parseCommit(ObjectId.fromString(originalPatchSet
-                .getRevision().get()));
-
-        CommitBuilder rebasedCommitBuilder =
-            rebaseCommit(git, originalCommit, branchTipCommit, myIdent);
-
+        final RevCommit rebasedCommit;
         final ObjectInserter oi = git.newObjectInserter();
-        final ObjectId rebasedCommitId;
         try {
-          rebasedCommitId = oi.insert(rebasedCommitBuilder);
+          ObjectId oldId = ObjectId.fromString(originalPatchSet.getRevision().get());
+          ObjectId newId = oi.insert(rebaseCommit(
+              git, oi, revWalk.parseCommit(oldId), branchTipCommit, myIdent));
           oi.flush();
+          rebasedCommit = revWalk.parseCommit(newId);
         } finally {
           oi.release();
         }
 
-        Change updatedChange =
-            db.changes().atomicUpdate(changeId, new AtomicUpdate<Change>() {
-              @Override
-              public Change update(Change change) {
-                if (change.getStatus().isOpen()) {
-                  change.nextPatchSetId();
-                  return change;
-                } else {
-                  return null;
-                }
-              }
-            });
-
-        if (updatedChange == null) {
-          throw new InvalidChangeOperationException("Change is closed: "
-              + change.toString());
-        } else {
-          change = updatedChange;
-        }
-
-        final PatchSet rebasedPatchSet = new PatchSet(change.currPatchSetId());
-        rebasedPatchSet.setCreatedOn(change.getCreatedOn());
-        rebasedPatchSet.setUploader(user.getAccountId());
-        rebasedPatchSet.setRevision(new RevId(rebasedCommitId.getName()));
-
-        insertAncestors(db, rebasedPatchSet.getId(),
-            revWalk.parseCommit(rebasedCommitId));
+        change.nextPatchSetId();
+        final PatchSet newPatchSet = new PatchSet(change.currPatchSetId());
+        newPatchSet.setCreatedOn(new Timestamp(System.currentTimeMillis()));
+        newPatchSet.setUploader(user.getAccountId());
+        newPatchSet.setRevision(new RevId(rebasedCommit.name()));
 
-        db.patchSets().insert(Collections.singleton(rebasedPatchSet));
         final PatchSetInfo info =
-            patchSetInfoFactory.get(db, rebasedPatchSet.getId());
-
-        change =
-            db.changes().atomicUpdate(change.getId(),
-                new AtomicUpdate<Change>() {
-                  @Override
-                  public Change update(Change change) {
-                    change.setCurrentPatchSet(info);
-                    ChangeUtil.updated(change);
-                    return change;
-                  }
-                });
+            patchSetInfoFactory.get(rebasedCommit, newPatchSet.getId());
 
-        final RefUpdate ru = git.updateRef(rebasedPatchSet.getRefName());
-        ru.setNewObjectId(rebasedCommitId);
+        RefUpdate ru = git.updateRef(newPatchSet.getRefName());
+        ru.setExpectedOldObjectId(ObjectId.zeroId());
+        ru.setNewObjectId(rebasedCommit);
         ru.disableRefLog();
         if (ru.update(revWalk) != RefUpdate.Result.NEW) {
-          throw new IOException("Failed to create ref "
-              + rebasedPatchSet.getRefName() + " in " + git.getDirectory()
-              + ": " + ru.getResult());
+          throw new IOException(String.format(
+              "Failed to create ref %s in %s: %s", newPatchSet.getRefName(),
+              change.getDest().getParentKey().get(), ru.getResult()));
         }
-
         replication.fire(change.getProject(), ru.getName());
 
-        List<PatchSetApproval> patchSetApprovals = approvalsUtil.copyVetosToLatestPatchSet(change);
-
-        final Set<Account.Id> oldReviewers = new HashSet<Account.Id>();
-        final Set<Account.Id> oldCC = new HashSet<Account.Id>();
+        final Set<Account.Id> oldReviewers = Sets.newHashSet();
+        final Set<Account.Id> oldCC = Sets.newHashSet();
+        db.changes().beginTransaction(change.getId());
+        try {
+          Change updatedChange;
+
+          updatedChange = db.changes().atomicUpdate(changeId,
+              new AtomicUpdate<Change>() {
+                @Override
+                public Change update(Change change) {
+                  if (change.getStatus().isOpen()) {
+                    change.updateNumberOfPatchSets(newPatchSet.getPatchSetId());
+                    return change;
+                  } else {
+                    return null;
+                  }
+                }
+              });
+          if (updatedChange != null) {
+            change = updatedChange;
+          } else {
+            throw new InvalidChangeOperationException(
+                String.format("Change %s is closed", change.getId()));
+          }
 
-        for (PatchSetApproval a : patchSetApprovals) {
-          if (a.getValue() != 0) {
-            oldReviewers.add(a.getAccountId());
+          insertAncestors(db, newPatchSet.getId(), rebasedCommit);
+          db.patchSets().insert(Collections.singleton(newPatchSet));
+          updatedChange = db.changes().atomicUpdate(changeId,
+              new AtomicUpdate<Change>() {
+                @Override
+                public Change update(Change change) {
+                  if (change.getStatus().isClosed()) {
+                    return null;
+                  }
+                  if (!change.currentPatchSetId().equals(patchSetId)) {
+                    return null;
+                  }
+                  if (change.getStatus() != Change.Status.DRAFT) {
+                    change.setStatus(Change.Status.NEW);
+                  }
+                  change.setLastSha1MergeTested(null);
+                  change.setCurrentPatchSet(info);
+                  ChangeUtil.updated(change);
+                  return change;
+                }
+              });
+          if (updatedChange != null) {
+            change = updatedChange;
           } else {
-            oldCC.add(a.getAccountId());
+            throw new InvalidChangeOperationException(
+                String.format("Change %s was modified", change.getId()));
+          }
+
+          for (PatchSetApproval a : approvalsUtil.copyVetosToLatestPatchSet(change)) {
+            if (a.getValue() != 0) {
+              oldReviewers.add(a.getAccountId());
+            } else {
+              oldCC.add(a.getAccountId());
+            }
           }
-        }
 
-        final ChangeMessage cmsg =
-            new ChangeMessage(new ChangeMessage.Key(changeId,
-                ChangeUtil.messageUUID(db)), user.getAccountId(), patchSetId);
-        cmsg.setMessage("Patch Set " + patchSetId.get() + ": Rebased");
-        db.changeMessages().insert(Collections.singleton(cmsg));
+          final ChangeMessage cmsg =
+              new ChangeMessage(new ChangeMessage.Key(changeId,
+                  ChangeUtil.messageUUID(db)), user.getAccountId(), patchSetId);
+          cmsg.setMessage("Patch Set " + patchSetId.get() + ": Rebased");
+          db.changeMessages().insert(Collections.singleton(cmsg));
+          db.commit();
+        } finally {
+          db.rollback();
+        }
 
         final ReplacePatchSetSender cm =
             rebasedPatchSetSenderFactory.create(change);
         cm.setFrom(user.getAccountId());
-        cm.setPatchSet(rebasedPatchSet);
+        cm.setPatchSet(newPatchSet);
         cm.addReviewers(oldReviewers);
         cm.addExtraCC(oldCC);
         cm.send();
 
-        hooks.doPatchsetCreatedHook(change, rebasedPatchSet, db);
+        hooks.doPatchsetCreatedHook(change, newPatchSet, db);
       } finally {
         revWalk.release();
       }
@@ -432,9 +463,7 @@ public class ChangeUtil {
       final PatchSetInfoFactory patchSetInfoFactory,
       final GitReferenceUpdated replication, PersonIdent myIdent)
       throws NoSuchChangeException, EmailException, OrmException,
-      MissingObjectException, IncorrectObjectTypeException, IOException,
-      PatchSetInfoNotAvailableException {
-
+      MissingObjectException, IncorrectObjectTypeException, IOException {
     final Change.Id changeId = patchSetId.getParentKey();
     final PatchSet patch = db.patchSets().get(patchSetId);
     if (patch == null) {
@@ -446,7 +475,7 @@ public class ChangeUtil {
       git = gitManager.openRepository(db.changes().get(changeId).getProject());
     } catch (RepositoryNotFoundException e) {
       throw new NoSuchChangeException(changeId, e);
-    };
+    }
 
     final RevWalk revWalk = new RevWalk(git);
     try {
@@ -459,54 +488,62 @@ public class ChangeUtil {
       RevCommit parentToCommitToRevert = commitToRevert.getParent(0);
       revWalk.parseHeaders(parentToCommitToRevert);
 
-      CommitBuilder revertCommit = new CommitBuilder();
-      revertCommit.addParentId(commitToRevert);
-      revertCommit.setTreeId(parentToCommitToRevert.getTree());
-      revertCommit.setAuthor(authorIdent);
-      revertCommit.setCommitter(myIdent);
+      CommitBuilder revertCommitBuilder = new CommitBuilder();
+      revertCommitBuilder.addParentId(commitToRevert);
+      revertCommitBuilder.setTreeId(parentToCommitToRevert.getTree());
+      revertCommitBuilder.setAuthor(authorIdent);
+      revertCommitBuilder.setCommitter(myIdent);
 
       final ObjectId computedChangeId =
           ChangeIdUtil.computeChangeId(parentToCommitToRevert.getTree(),
               commitToRevert, authorIdent, myIdent, message);
-      revertCommit.setMessage(ChangeIdUtil.insertId(message, computedChangeId, true));
+      revertCommitBuilder.setMessage(ChangeIdUtil.insertId(message, computedChangeId, true));
 
-      final ObjectInserter oi = git.newObjectInserter();;
-      ObjectId id;
+      RevCommit revertCommit;
+      final ObjectInserter oi = git.newObjectInserter();
       try {
-        id = oi.insert(revertCommit);
+        ObjectId id = oi.insert(revertCommitBuilder);
         oi.flush();
+        revertCommit = revWalk.parseCommit(id);
       } finally {
         oi.release();
       }
 
-      Change.Key changeKey = new Change.Key("I" + computedChangeId.name());
-      final Change change =
-          new Change(changeKey, new Change.Id(db.nextChangeId()),
-              user.getAccountId(), db.changes().get(changeId).getDest());
+      final Change change = new Change(
+          new Change.Key("I" + computedChangeId.name()),
+          new Change.Id(db.nextChangeId()),
+          user.getAccountId(),
+          db.changes().get(changeId).getDest());
       change.nextPatchSetId();
 
       final PatchSet ps = new PatchSet(change.currPatchSetId());
       ps.setCreatedOn(change.getCreatedOn());
-      ps.setUploader(user.getAccountId());
-      ps.setRevision(new RevId(id.getName()));
-
-      db.patchSets().insert(Collections.singleton(ps));
+      ps.setUploader(change.getOwner());
+      ps.setRevision(new RevId(revertCommit.name()));
 
-      final PatchSetInfo info =
-          patchSetInfoFactory.get(revWalk.parseCommit(id), ps.getId());
-      change.setCurrentPatchSet(info);
+      change.setCurrentPatchSet(patchSetInfoFactory.get(revertCommit, ps.getId()));
       ChangeUtil.updated(change);
-      db.changes().insert(Collections.singleton(change));
 
       final RefUpdate ru = git.updateRef(ps.getRefName());
-      ru.setNewObjectId(id);
+      ru.setExpectedOldObjectId(ObjectId.zeroId());
+      ru.setNewObjectId(revertCommit);
       ru.disableRefLog();
       if (ru.update(revWalk) != RefUpdate.Result.NEW) {
-        throw new IOException("Failed to create ref " + ps.getRefName()
-            + " in " + git.getDirectory() + ": " + ru.getResult());
+        throw new IOException(String.format(
+            "Failed to create ref %s in %s: %s", ps.getRefName(),
+            change.getDest().getParentKey().get(), ru.getResult()));
+      }
+      replication.fire(change.getProject(), ru.getName());
+
+      db.changes().beginTransaction(change.getId());
+      try {
+        insertAncestors(db, ps.getId(), revertCommit);
+        db.patchSets().insert(Collections.singleton(ps));
+        db.changes().insert(Collections.singleton(change));
+        db.commit();
+      } finally {
+        db.rollback();
       }
-      replication.fire(db.changes().get(changeId).getProject(),
-          ru.getName());
 
       final ChangeMessage cmsg =
           new ChangeMessage(new ChangeMessage.Key(changeId,
@@ -514,7 +551,7 @@ public class ChangeUtil {
       final StringBuilder msgBuf =
           new StringBuilder("Patch Set " + patchSetId.get() + ": Reverted");
       msgBuf.append("\n\n");
-      msgBuf.append("This patchset was reverted in change: " + changeKey.get());
+      msgBuf.append("This patchset was reverted in change: " + change.getKey().get());
 
       cmsg.setMessage(msgBuf.toString());
       db.changeMessages().insert(Collections.singleton(cmsg));
@@ -596,7 +633,7 @@ public class ChangeUtil {
   public static <T extends ReplyToChangeSender> void updatedChange(
       final ReviewDb db, final IdentifiedUser user, final Change change,
       final ChangeMessage cmsg, ReplyToChangeSender.Factory<T> senderFactory)
-      throws NoSuchChangeException, EmailException, OrmException {
+      throws OrmException {
     db.changeMessages().insert(Collections.singleton(cmsg));
 
     new ApprovalsUtil(db, null).syncChangeStatus(change);