ConsistencyChecker check and fix for missing patch set refs

Change-Id: Ic485aa6e35fd0b0127362b5213e7f392312b727e

gerrit-server/src/main/java/com/google/gerrit/server/InternalUser.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.gerrit.reviewdb.client.AccountProjectWatch;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.server.account.CapabilityControl;
@@ -39,8 +40,9 @@ public class InternalUser extends CurrentUser {
     InternalUser create();
   }
 
+  @VisibleForTesting
   @Inject
-  protected InternalUser(CapabilityControl.Factory capabilityControlFactory) {
+  public InternalUser(CapabilityControl.Factory capabilityControlFactory) {
     super(capabilityControlFactory);
   }
 

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

@@ -28,6 +28,9 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 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.GerritPersonIdent;
+import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gwtorm.server.AtomicUpdate;
@@ -39,7 +42,9 @@ import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -82,6 +87,8 @@ public class ConsistencyChecker {
 
   private final Provider<ReviewDb> db;
   private final GitRepositoryManager repoManager;
+  private final Provider<CurrentUser> user;
+  private final Provider<PersonIdent> serverIdent;
 
   private FixInput fix;
   private Change change;
@@ -95,9 +102,13 @@ public class ConsistencyChecker {
 
   @Inject
   ConsistencyChecker(Provider<ReviewDb> db,
-      GitRepositoryManager repoManager) {
+      GitRepositoryManager repoManager,
+      Provider<CurrentUser> user,
+      @GerritPersonIdent Provider<PersonIdent> serverIdent) {
     this.db = db;
     this.repoManager = repoManager;
+    this.user = user;
+    this.serverIdent = serverIdent;
     reset();
   }
 
@@ -209,9 +220,11 @@ public class ConsistencyChecker {
         .treeSetValues(Ordering.natural().onResultOf(toPsId))
         .build();
     for (PatchSet ps : all) {
+      // Check revision format.
       ObjectId objId;
       String rev = ps.getRevision().get();
       int psNum = ps.getId().get();
+      String refName = ps.getId().toRefName();
       try {
         objId = ObjectId.fromString(rev);
       } catch (IllegalArgumentException e) {
@@ -221,16 +234,39 @@ public class ConsistencyChecker {
       }
       bySha.put(objId, ps);
 
+      // Check ref existence.
+      ProblemInfo refProblem = null;
+      try {
+        Ref ref = repo.getRef(refName);
+        if (ref == null) {
+          refProblem = problem("Ref missing: " + refName);
+        } else if (!objId.equals(ref.getObjectId())) {
+          String actual = ref.getObjectId() != null
+              ? ref.getObjectId().name()
+              : "null";
+          refProblem = problem(String.format(
+              "Expected %s to point to %s, found %s",
+              ref.getName(), objId.name(), actual));
+        }
+      } catch (IOException e) {
+        error("Error reading ref: " + refName, e);
+        refProblem = lastProblem();
+      }
+
+      // Check object existence.
       RevCommit psCommit = parseCommit(
           objId, String.format("patch set %d", psNum));
       if (psCommit == null) {
         continue;
+      } else if (refProblem != null && fix != null) {
+        fixPatchSetRef(refProblem, ps);
       }
       if (ps.getId().equals(change.currentPatchSetId())) {
         currPsCommit = psCommit;
       }
     }
 
+    // Check for duplicates.
     for (Map.Entry<ObjectId, Collection<PatchSet>> e
         : bySha.asMap().entrySet()) {
       if (e.getValue().size() > 1) {
@@ -305,6 +341,44 @@ public class ConsistencyChecker {
     }
   }
 
+  private void fixPatchSetRef(ProblemInfo p, PatchSet ps) {
+    try {
+      RefUpdate ru = repo.updateRef(ps.getId().toRefName());
+      ru.setForceUpdate(true);
+      ru.setNewObjectId(ObjectId.fromString(ps.getRevision().get()));
+      ru.setRefLogIdent(newRefLogIdent());
+      ru.setRefLogMessage("Repair patch set ref", true);
+      RefUpdate.Result result = ru.update();
+      switch (result) {
+        case NEW:
+        case FORCED:
+        case FAST_FORWARD:
+        case NO_CHANGE:
+          p.status = Status.FIXED;
+          p.outcome = "Repaired patch set ref";
+          return;
+        default:
+          p.status = Status.FIX_FAILED;
+          p.outcome = "Failed to update patch set ref: " + result;
+          return;
+      }
+    } catch (IOException e) {
+      String msg = "Error fixing patch set ref";
+      log.warn(msg + ' ' + ps.getId().toRefName(), e);
+      p.status = Status.FIX_FAILED;
+      p.outcome = msg;
+    }
+  }
+
+  private PersonIdent newRefLogIdent() {
+    CurrentUser u = user.get();
+    if (u.isIdentifiedUser()) {
+      return ((IdentifiedUser) u).newRefLogIdent();
+    } else {
+      return serverIdent.get();
+    }
+  }
+
   private RevCommit parseCommit(ObjectId objId, String desc) {
     try {
       return rw.parseCommit(objId);
@@ -325,6 +399,10 @@ public class ConsistencyChecker {
     return p;
   }
 
+  private ProblemInfo lastProblem() {
+    return problems.get(problems.size() - 1);
+  }
+
   private boolean error(String msg, Throwable t) {
     problem(msg);
     // TODO(dborowitz): Expose stack trace to administrators.

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

@@ -30,6 +30,8 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 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.testutil.InMemoryDatabase;
 import com.google.gerrit.testutil.InMemoryRepositoryManager;
 import com.google.gerrit.testutil.TestChanges;
@@ -38,6 +40,7 @@ import com.google.inject.util.Providers;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.junit.After;
@@ -63,7 +66,11 @@ public class ConsistencyCheckerTest {
     schemaFactory.create();
     db = schemaFactory.open();
     repoManager = new InMemoryRepositoryManager();
-    checker = new ConsistencyChecker(Providers.<ReviewDb> of(db), repoManager);
+    checker = new ConsistencyChecker(
+        Providers.<ReviewDb> of(db),
+        repoManager,
+        Providers.<CurrentUser> of(new InternalUser(null)),
+        Providers.of(new PersonIdent("server", "noreply@example.com")));
     project = new Project.NameKey("repo");
     repo = new TestRepository<>(repoManager.createRepository(project));
     userId = new Account.Id(1);
@@ -141,17 +148,64 @@ public class ConsistencyCheckerTest {
         + " fooooooooooooooooooooooooooooooooooooooo");
   }
 
+  // No test for ref existing but object missing; InMemoryRepository won't let
+  // us do such a thing.
+
   @Test
-  public void patchSetObjectMissing() throws Exception {
+  public void patchSetObjectAndRefMissing() throws Exception {
     Change c = insertChange();
     PatchSet ps = newPatchSet(c.currentPatchSetId(),
         ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"), userId);
     db.patchSets().insert(singleton(ps));
 
     assertProblems(c,
+        "Ref missing: " + ps.getId().toRefName(),
         "Object missing: patch set 1: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
   }
 
+  @Test
+  public void patchSetObjectAndRefMissingWithFix() throws Exception {
+    Change c = insertChange();
+    PatchSet ps = newPatchSet(c.currentPatchSetId(),
+        ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"), userId);
+    db.patchSets().insert(singleton(ps));
+
+    String refName = ps.getId().toRefName();
+    List<ProblemInfo> problems = checker.check(c, new FixInput()).problems();
+    ProblemInfo p = problems.get(0);
+    assertThat(p.message).isEqualTo("Ref missing: " + refName);
+    assertThat(p.status).isNull();
+  }
+
+  @Test
+  public void patchSetRefMissing() throws Exception {
+    Change c = insertChange();
+    PatchSet ps = insertPatchSet(c);
+    String refName = ps.getId().toRefName();
+    repo.update("refs/other/foo", ObjectId.fromString(ps.getRevision().get()));
+    deleteRef(refName);
+
+    assertProblems(c, "Ref missing: " + refName);
+  }
+
+  @Test
+  public void patchSetRefMissingWithFix() throws Exception {
+    Change c = insertChange();
+    PatchSet ps = insertPatchSet(c);
+    String refName = ps.getId().toRefName();
+    repo.update("refs/other/foo", ObjectId.fromString(ps.getRevision().get()));
+    deleteRef(refName);
+
+    List<ProblemInfo> problems = checker.check(c, new FixInput()).problems();
+    ProblemInfo p = problems.get(0);
+    assertThat(p.message).isEqualTo("Ref missing: " + refName);
+    assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED);
+    assertThat(p.outcome).isEqualTo("Repaired patch set ref");
+
+    assertThat(repo.getRepository().getRef(refName).getObjectId().name())
+        .isEqualTo(ps.getRevision().get());
+  }
+
   @Test
   public void currentPatchSetMissing() throws Exception {
     Change c = insertChange();
@@ -164,7 +218,8 @@ public class ConsistencyCheckerTest {
     PatchSet ps1 = insertPatchSet(c);
     String rev = ps1.getRevision().get();
     incrementPatchSet(c);
-    insertMissingPatchSet(c, rev);
+    PatchSet ps2 = insertMissingPatchSet(c, rev);
+    updatePatchSetRef(ps2);
 
     assertProblems(c,
         "Multiple patch sets pointing to " + rev + ": [1, 2]");
@@ -178,6 +233,7 @@ public class ConsistencyCheckerTest {
     Change c = insertChange();
     RevCommit commit = repo.commit().create();
     PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, userId);
+    updatePatchSetRef(ps);
     db.patchSets().insert(singleton(ps));
 
     assertProblems(c, "Destination ref not found (may be new branch): master");
@@ -248,6 +304,7 @@ public class ConsistencyCheckerTest {
     RevCommit commit = repo.branch(c.currentPatchSetId().toRefName()).commit()
         .parent(tip).create();
     PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, userId);
+    updatePatchSetRef(ps);
     db.patchSets().insert(singleton(ps));
     return ps;
   }
@@ -259,6 +316,17 @@ public class ConsistencyCheckerTest {
     return ps;
   }
 
+  private void updatePatchSetRef(PatchSet ps) throws Exception {
+    repo.update(ps.getId().toRefName(),
+        ObjectId.fromString(ps.getRevision().get()));
+  }
+
+  private void deleteRef(String refName) throws Exception {
+    RefUpdate ru = repo.getRepository().updateRef(refName, true);
+    ru.setForceUpdate(true);
+    assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED);
+  }
+
   private void assertProblems(Change c, String... expected) {
     assertThat(Lists.transform(checker.check(c).problems(),
           new Function<ProblemInfo, String>() {