ConsistencyChecker fix to delete patch sets with missing objects

After I0594aef6 it is possible for these to result from the merge
queue; even before that, manual ref deletion by someone with raw
filesystem access was a possibility.

Change-Id: I20dba162bffddf0d994b1ec11dcad3d09016d2c9

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/FixInput.java

@@ -15,4 +15,5 @@
 package com.google.gerrit.extensions.api.changes;
 
 public class FixInput {
+  public boolean deletePatchSetIfCommitMissing;
 }

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ProblemInfo.java

@@ -22,4 +22,15 @@ public class ProblemInfo {
   public String message;
   public Status status;
   public String outcome;
+
+  @Override
+  public String toString() {
+    StringBuilder sb = new StringBuilder(getClass().getSimpleName())
+        .append('[').append(message);
+    if (status != null || outcome != null) {
+      sb.append(" (").append(status).append(": ").append(outcome)
+          .append(')');
+    }
+    return sb.append(']').toString();
+  }
 }

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

@@ -17,6 +17,7 @@ package com.google.gerrit.server.change;
 import com.google.auto.value.AutoValue;
 import com.google.common.base.Function;
 import com.google.common.collect.Collections2;
+import com.google.common.collect.Lists;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.MultimapBuilder;
 import com.google.common.collect.Ordering;
@@ -32,6 +33,8 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.patch.PatchSetInfoFactory;
+import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gwtorm.server.AtomicUpdate;
 import com.google.gwtorm.server.OrmException;
@@ -54,6 +57,7 @@ import org.slf4j.LoggerFactory;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 
@@ -89,6 +93,7 @@ public class ConsistencyChecker {
   private final GitRepositoryManager repoManager;
   private final Provider<CurrentUser> user;
   private final Provider<PersonIdent> serverIdent;
+  private final PatchSetInfoFactory patchSetInfoFactory;
 
   private FixInput fix;
   private Change change;
@@ -104,11 +109,13 @@ public class ConsistencyChecker {
   ConsistencyChecker(Provider<ReviewDb> db,
       GitRepositoryManager repoManager,
       Provider<CurrentUser> user,
-      @GerritPersonIdent Provider<PersonIdent> serverIdent) {
+      @GerritPersonIdent Provider<PersonIdent> serverIdent,
+      PatchSetInfoFactory patchSetInfoFactory) {
     this.db = db;
     this.repoManager = repoManager;
     this.user = user;
     this.serverIdent = serverIdent;
+    this.patchSetInfoFactory = patchSetInfoFactory;
     reset();
   }
 
@@ -203,21 +210,29 @@ public class ConsistencyChecker {
     }
   }
 
+  private static final Function<PatchSet, Integer> TO_PS_ID =
+      new Function<PatchSet, Integer>() {
+        @Override
+        public Integer apply(PatchSet in) {
+          return in.getId().get();
+        }
+      };
+
+  private static final Ordering<PatchSet> PS_ID_ORDER = Ordering.natural()
+    .onResultOf(TO_PS_ID);
+
   private boolean checkPatchSets() {
     List<PatchSet> all;
     try {
-      all = db.get().patchSets().byChange(change.getId()).toList();
+      all = Lists.newArrayList(db.get().patchSets().byChange(change.getId()));
     } catch (OrmException e) {
       return error("Failed to look up patch sets", e);
     }
-    Function<PatchSet, Integer> toPsId = new Function<PatchSet, Integer>() {
+    // Iterate in descending order so deletePatchSet can assume the latest patch
-      @Override
+    // set exists.
-      public Integer apply(PatchSet in) {
+    Collections.sort(all, PS_ID_ORDER.reverse());
-        return in.getId().get();
-      }
-    };
     Multimap<ObjectId, PatchSet> bySha = MultimapBuilder.hashKeys(all.size())
-        .treeSetValues(Ordering.natural().onResultOf(toPsId))
+        .treeSetValues(PS_ID_ORDER)
         .build();
     for (PatchSet ps : all) {
       // Check revision format.
@@ -257,6 +272,9 @@ public class ConsistencyChecker {
       RevCommit psCommit = parseCommit(
           objId, String.format("patch set %d", psNum));
       if (psCommit == null) {
+        if (fix != null && fix.deletePatchSetIfCommitMissing) {
+          deletePatchSet(lastProblem(), ps.getId());
+        }
         continue;
       } else if (refProblem != null && fix != null) {
         fixPatchSetRef(refProblem, ps);
@@ -272,7 +290,7 @@ public class ConsistencyChecker {
       if (e.getValue().size() > 1) {
         problem(String.format("Multiple patch sets pointing to %s: %s",
             e.getKey().name(),
-            Collections2.transform(e.getValue(), toPsId)));
+            Collections2.transform(e.getValue(), TO_PS_ID)));
       }
     }
 
@@ -370,6 +388,66 @@ public class ConsistencyChecker {
     }
   }
 
+  private void deletePatchSet(ProblemInfo p, PatchSet.Id psId) {
+    ReviewDb db = this.db.get();
+    Change.Id cid = psId.getParentKey();
+    try {
+      db.changes().beginTransaction(cid);
+      try {
+        Change c = db.changes().get(cid);
+        if (c == null) {
+          throw new OrmException("Change missing: " + cid);
+        }
+
+        if (psId.equals(c.currentPatchSetId())) {
+          List<PatchSet> all = Lists.newArrayList(db.patchSets().byChange(cid));
+          if (all.size() == 1 && all.get(0).getId().equals(psId)) {
+            p.status = Status.FIX_FAILED;
+            p.outcome = "Cannot delete patch set; no patch sets would remain";
+            return;
+          }
+          // If there were multiple missing patch sets, assumes deletePatchSet
+          // has been called in decreasing order, so the max remaining PatchSet
+          // is the effective current patch set.
+          Collections.sort(all, PS_ID_ORDER.reverse());
+          PatchSet.Id latest = null;
+          for (PatchSet ps : all) {
+            latest = ps.getId();
+            if (!ps.getId().equals(psId)) {
+              break;
+            }
+          }
+          c.setCurrentPatchSet(patchSetInfoFactory.get(db, latest));
+          db.changes().update(Collections.singleton(c));
+        }
+
+        // Delete dangling primary key references. Don't delete ChangeMessages,
+        // which don't use patch sets as a primary key, and may provide useful
+        // historical information.
+        db.accountPatchReviews().delete(
+            db.accountPatchReviews().byPatchSet(psId));
+        db.patchSetAncestors().delete(
+            db.patchSetAncestors().byPatchSet(psId));
+        db.patchSetApprovals().delete(
+            db.patchSetApprovals().byPatchSet(psId));
+        db.patchComments().delete(
+            db.patchComments().byPatchSet(psId));
+        db.patchSets().deleteKeys(Collections.singleton(psId));
+        db.commit();
+
+        p.status = Status.FIXED;
+        p.outcome = "Deleted patch set";
+      } finally {
+        db.rollback();
+      }
+    } catch (PatchSetInfoNotAvailableException | OrmException e) {
+      String msg = "Error deleting patch set";
+      log.warn(msg + ' ' + psId, e);
+      p.status = Status.FIX_FAILED;
+      p.outcome = msg;
+    }
+  }
+
   private PersonIdent newRefLogIdent() {
     CurrentUser u = user.get();
     if (u.isIdentifiedUser()) {

gerrit-server/src/test/java/com/google/gerrit/server/change/ConsistencyCheckerTest.java

@@ -32,6 +32,8 @@ 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.InternalUser;
+import com.google.gerrit.server.patch.PatchSetInfoFactory;
+import com.google.gerrit.testutil.FakeAccountByEmailCache;
 import com.google.gerrit.testutil.InMemoryDatabase;
 import com.google.gerrit.testutil.InMemoryRepositoryManager;
 import com.google.gerrit.testutil.TestChanges;
@@ -62,6 +64,7 @@ public class ConsistencyCheckerTest {
 
   @Before
   public void setUp() throws Exception {
+    FakeAccountByEmailCache accountCache = new FakeAccountByEmailCache();
     schemaFactory = InMemoryDatabase.newDatabase();
     schemaFactory.create();
     db = schemaFactory.open();
@@ -70,10 +73,12 @@ public class ConsistencyCheckerTest {
         Providers.<ReviewDb> of(db),
         repoManager,
         Providers.<CurrentUser> of(new InternalUser(null)),
-        Providers.of(new PersonIdent("server", "noreply@example.com")));
+        Providers.of(new PersonIdent("server", "noreply@example.com")),
+        new PatchSetInfoFactory(repoManager, accountCache));
     project = new Project.NameKey("repo");
     repo = new TestRepository<>(repoManager.createRepository(project));
     userId = new Account.Id(1);
+    accountCache.putAny(userId);
     db.accounts().insert(singleton(new Account(userId, TimeUtil.nowTs())));
     tip = repo.branch("master").commit().create();
   }
@@ -206,6 +211,109 @@ public class ConsistencyCheckerTest {
         .isEqualTo(ps.getRevision().get());
   }
 
+  @Test
+  public void patchSetObjectAndRefMissingWithDeletingPatchSet()
+      throws Exception {
+    Change c = insertChange();
+    PatchSet ps1 = insertPatchSet(c);
+    incrementPatchSet(c);
+    PatchSet ps2 = insertMissingPatchSet(c,
+        "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
+
+    FixInput fix = new FixInput();
+    fix.deletePatchSetIfCommitMissing = true;
+    List<ProblemInfo> problems = checker.check(c, fix).problems();
+    assertThat(problems).hasSize(2);
+    ProblemInfo p = problems.get(0);
+    assertThat(p.message).isEqualTo("Ref missing: " + ps2.getId().toRefName());
+    assertThat(p.status).isNull();
+    p = problems.get(1);
+    assertThat(p.message).isEqualTo(
+        "Object missing: patch set 2: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
+    assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED);
+    assertThat(p.outcome).isEqualTo("Deleted patch set");
+
+    c = db.changes().get(c.getId());
+    assertThat(c.currentPatchSetId().get()).isEqualTo(1);
+    assertThat(db.patchSets().get(ps1.getId())).isNotNull();
+    assertThat(db.patchSets().get(ps2.getId())).isNull();
+  }
+
+  @Test
+  public void patchSetMultipleObjectsMissingWithDeletingPatchSets()
+      throws Exception {
+    Change c = insertChange();
+    PatchSet ps1 = insertPatchSet(c);
+
+    incrementPatchSet(c);
+    PatchSet ps2 = insertMissingPatchSet(c,
+        "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
+
+    incrementPatchSet(c);
+    PatchSet ps3 = insertPatchSet(c);
+
+    incrementPatchSet(c);
+    PatchSet ps4 = insertMissingPatchSet(c,
+        "c0ffeeeec0ffeeeec0ffeeeec0ffeeeec0ffeeee");
+
+    FixInput fix = new FixInput();
+    fix.deletePatchSetIfCommitMissing = true;
+    List<ProblemInfo> problems = checker.check(c, fix).problems();
+    assertThat(problems).hasSize(4);
+
+    ProblemInfo p = problems.get(0);
+    assertThat(p.message).isEqualTo("Ref missing: " + ps4.getId().toRefName());
+    assertThat(p.status).isNull();
+
+    p = problems.get(1);
+    assertThat(p.message).isEqualTo(
+        "Object missing: patch set 4: c0ffeeeec0ffeeeec0ffeeeec0ffeeeec0ffeeee");
+    assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED);
+    assertThat(p.outcome).isEqualTo("Deleted patch set");
+
+    p = problems.get(2);
+    assertThat(p.message).isEqualTo("Ref missing: " + ps2.getId().toRefName());
+    assertThat(p.status).isNull();
+
+    p = problems.get(3);
+    assertThat(p.message).isEqualTo(
+        "Object missing: patch set 2: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
+    assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED);
+    assertThat(p.outcome).isEqualTo("Deleted patch set");
+
+    c = db.changes().get(c.getId());
+    assertThat(c.currentPatchSetId().get()).isEqualTo(3);
+    assertThat(db.patchSets().get(ps1.getId())).isNotNull();
+    assertThat(db.patchSets().get(ps2.getId())).isNull();
+    assertThat(db.patchSets().get(ps3.getId())).isNotNull();
+    assertThat(db.patchSets().get(ps4.getId())).isNull();
+  }
+
+  @Test
+  public void onlyPatchSetObjectMissingWithFix() throws Exception {
+    Change c = insertChange();
+    PatchSet ps1 = insertMissingPatchSet(c,
+        "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
+
+    FixInput fix = new FixInput();
+    fix.deletePatchSetIfCommitMissing = true;
+    List<ProblemInfo> problems = checker.check(c, fix).problems();
+    assertThat(problems).hasSize(2);
+    ProblemInfo p = problems.get(0);
+    assertThat(p.message).isEqualTo("Ref missing: " + ps1.getId().toRefName());
+    assertThat(p.status).isNull();
+    p = problems.get(1);
+    assertThat(p.message).isEqualTo(
+        "Object missing: patch set 1: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
+    assertThat(p.status).isEqualTo(ProblemInfo.Status.FIX_FAILED);
+    assertThat(p.outcome)
+        .isEqualTo("Cannot delete patch set; no patch sets would remain");
+
+    c = db.changes().get(c.getId());
+    assertThat(c.currentPatchSetId().get()).isEqualTo(1);
+    assertThat(db.patchSets().get(ps1.getId())).isNotNull();
+  }
+
   @Test
   public void currentPatchSetMissing() throws Exception {
     Change c = insertChange();

gerrit-server/src/test/java/com/google/gerrit/testutil/FakeAccountByEmailCache.java

@@ -0,0 +1,55 @@
+// Copyright (C) 2015 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.testutil;
+
+import com.google.common.collect.HashMultimap;
+import com.google.common.collect.SetMultimap;
+import com.google.common.collect.Sets;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.server.account.AccountByEmailCache;
+
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+
+/** Fake implementation of {@link AccountByEmailCache} for testing. */
+public class FakeAccountByEmailCache implements AccountByEmailCache {
+  private final SetMultimap<String, Account.Id> byEmail;
+  private final Set<Account.Id> anyEmail;
+
+  public FakeAccountByEmailCache() {
+    byEmail = HashMultimap.create();
+    anyEmail = new HashSet<>();
+  }
+
+  @Override
+  public synchronized Set<Account.Id> get(String email) {
+    return Collections.unmodifiableSet(
+        Sets.union(byEmail.get(email), anyEmail));
+  }
+
+  @Override
+  public synchronized void evict(String email) {
+    // Do nothing.
+  }
+
+  public synchronized void put(String email, Account.Id id) {
+    byEmail.put(email, id);
+  }
+
+  public synchronized void putAny(Account.Id id) {
+    anyEmail.add(id);
+  }
+}