Test conversion of corrupt changes with GERRIT_CHECK_NOTEDB

ConsistencyCheckerIT creates various kinds of corrupt changes that we
think might exist in the wild. In most cases, the right thing to do
when converting these changes is to do our best and store *something*
in NoteDb, even if it can't be successfully read back. At the very
least this allows admins to go in and manually edit the change, using
interactive rebase in the same way they might poke the SQL database
today. This is preferable to throwing the data away during conversion.

The one time we can't do this is when the change belongs to a deleted
project. For now, we actually just discard these changes.

Rework the NoteDbChecker loops so that we attempt to convert all
changes before checking any. This doesn't make ConsistencyCheckerIT
pass, but it does let us verify from the stack trace that all
conversions were successful, even though changes couldn't be read
back.

Change-Id: I87caa451db4e049fa3fc494b79c6c036fba36b1a

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

@@ -73,6 +73,9 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
   @Before
   public void setUp() throws Exception {
     // TODO(dborowitz): Re-enable when ConsistencyChecker supports notedb.
+    // Note that we *do* want to enable these tests with GERRIT_CHECK_NOTEDB, as
+    // we need to be able to convert old, corrupt changes. However, those tests
+    // don't necessarily need to pass.
     assume().that(notesMigration.enabled()).isFalse();
     // Ignore client clone of project; repurpose as server-side TestRepository.
     testRepo = new TestRepository<>(

gerrit-pgm/src/main/java/com/google/gerrit/pgm/RebuildNotedb.java

@@ -152,6 +152,9 @@ public class RebuildNotedb extends SiteProgram {
               mpm.beginSubTask("failed", MultiProgressMonitor.UNKNOWN);
 
           for (Change.Id id : changesByProject.get(project)) {
+            // TODO(dborowitz): This can fail if the project no longer exists.
+            // We might not want to just skip conversion of those changes, and
+            // instead move them somewhere like a special lost+found repo.
             ListenableFuture<?> future = rebuilder.rebuildAsync(id, executor);
             futures.add(future);
             future.addListener(

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java

@@ -58,6 +58,8 @@ import com.google.inject.Inject;
 import com.google.inject.util.Providers;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.errors.InvalidObjectIdException;
+import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
@@ -520,13 +522,31 @@ public class ChangeRebuilder {
       } else {
         update.setSubjectForCommit("Create patch set " + ps.getPatchSetId());
       }
-      update.setCommit(rw, ObjectId.fromString(ps.getRevision().get()),
-          ps.getPushCertificate());
+      setRevision(update, ps);
       update.setGroups(ps.getGroups());
       if (ps.isDraft()) {
         update.setPatchSetState(PatchSetState.DRAFT);
       }
     }
+
+    private void setRevision(ChangeUpdate update, PatchSet ps)
+        throws IOException {
+      String rev = ps.getRevision().get();
+      String cert = ps.getPushCertificate();
+      ObjectId id;
+      try {
+        id = ObjectId.fromString(rev);
+      } catch (InvalidObjectIdException e) {
+        update.setRevisionForMissingCommit(rev, cert);
+        return;
+      }
+      try {
+        update.setCommit(rw, id, cert);
+      } catch (MissingObjectException e) {
+        update.setRevisionForMissingCommit(rev, cert);
+        return;
+      }
+    }
   }
 
   private static class PatchLineCommentEvent extends Event {

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java

@@ -115,7 +115,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   private String submissionId;
   private List<PatchLineComment> comments;
   private String topic;
-  private ObjectId commit;
+  private String commit;
   private Set<String> hashtags;
   private String changeMessage;
   private PatchSetState psState;
@@ -265,7 +265,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
 
   @VisibleForTesting
   ObjectId getCommit() {
-    return commit;
+    return ObjectId.fromString(commit);
   }
 
   public void setChangeMessage(String changeMessage) {
@@ -325,11 +325,20 @@ public class ChangeUpdate extends AbstractChangeUpdate {
       throws IOException {
     RevCommit commit = rw.parseCommit(id);
     rw.parseBody(commit);
-    this.commit = commit;
+    this.commit = commit.name();
     subject = commit.getShortMessage();
     this.pushCert = pushCert;
   }
 
+  /**
+   * Set the revision without depending on the commit being present in the
+   * repository; should only be used for converting old corrupt commits.
+   */
+  public void setRevisionForMissingCommit(String id, String pushCert) {
+    commit = id;
+    this.pushCert = pushCert;
+  }
+
   public void setHashtags(Set<String> hashtags) {
     this.hashtags = hashtags;
   }
@@ -366,7 +375,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     }
     if (pushCert != null) {
       checkState(commit != null);
-      cache.get(new RevId(commit.name())).setPushCertificate(pushCert);
+      cache.get(new RevId(commit)).setPushCertificate(pushCert);
     }
     Map<RevId, RevisionNoteBuilder> builders = cache.getBuilders();
     checkComments(rnm.revisionNotes, builders);
@@ -487,7 +496,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     }
 
     if (commit != null) {
-      addFooter(msg, FOOTER_COMMIT, commit.name());
+      addFooter(msg, FOOTER_COMMIT, commit);
     }
 
     Joiner comma = Joiner.on(',');

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

@@ -28,12 +28,18 @@ import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
 
+import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 
 @Singleton
 public class NoteDbChecker {
+  static final Logger log = LoggerFactory.getLogger(NoteDbChecker.class);
+
   private final Provider<ReviewDb> dbProvider;
   private final TestNotesMigration notesMigration;
   private final ChangeNotes.Factory notesFactory;
@@ -80,9 +86,24 @@ public class NoteDbChecker {
     List<String> all = new ArrayList<>();
     for (ChangeBundle expected : allExpected) {
       Change c = expected.getChange();
-      changeRebuilder.rebuild(db, c.getId());
-      ChangeBundle actual = ChangeBundle.fromNotes(
-          plcUtil, notesFactory.create(db, c.getProject(), c.getId()));
+      try {
+        changeRebuilder.rebuild(db, c.getId());
+      } catch (RepositoryNotFoundException e) {
+        all.add("Repository not found for change, cannot convert: " + c);
+      }
+    }
+    for (ChangeBundle expected : allExpected) {
+      Change c = expected.getChange();
+      ChangeBundle actual;
+      try {
+        actual = ChangeBundle.fromNotes(
+            plcUtil, notesFactory.create(db, c.getProject(), c.getId()));
+      } catch (Throwable t) {
+        String msg = "Error converting change: " + c;
+        all.add(msg);
+        log.error(msg, t);
+        continue;
+      }
       List<String> diff = expected.differencesFrom(actual);
       if (!diff.isEmpty()) {
         all.add("Differences between ReviewDb and NoteDb for " + c + ":");