Merge "Add dryRun option to CommentJsonMigrator"

java/com/google/gerrit/server/notedb/CommentJsonMigrator.java

@@ -14,9 +14,11 @@
 
 package com.google.gerrit.server.notedb;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.gerrit.server.notedb.RevisionNote.MAX_NOTE_SZ;
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchLineComment.Status;
@@ -29,6 +31,7 @@ import com.google.gerrit.server.update.RefUpdateUtil;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 import java.io.IOException;
+import java.util.List;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.internal.storage.file.FileRepository;
 import org.eclipse.jgit.internal.storage.file.PackInserter;
@@ -54,7 +57,7 @@ public class CommentJsonMigrator {
   public static class ProjectMigrationResult {
     public int skipped;
     public boolean ok;
-    public int refsUpdated;
+    public List<String> refsUpdated;
   }
 
   private final LegacyChangeNoteRead legacyChangeNoteRead;
@@ -77,9 +80,12 @@ public class CommentJsonMigrator {
     this.allUsers = allUsers;
   }
 
-  public ProjectMigrationResult migrateProject(Project.NameKey project, Repository repo) {
+  public ProjectMigrationResult migrateProject(
+      Project.NameKey project, Repository repo, boolean dryRun) {
     ProjectMigrationResult progress = new ProjectMigrationResult();
     progress.ok = true;
+    progress.skipped = 0;
+    progress.refsUpdated = ImmutableList.of();
     try (RevWalk rw = new RevWalk(repo);
         ObjectInserter ins = newPackInserter(repo)) {
       BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate();
@@ -89,17 +95,20 @@ public class CommentJsonMigrator {
         progress.ok &= migrateDrafts(allUsers, repo, rw, ins, bru);
       }
 
-      progress.refsUpdated += bru.getCommands().size();
+      progress.refsUpdated =
+          bru.getCommands().stream().map(c -> c.getRefName()).collect(toImmutableList());
       if (!bru.getCommands().isEmpty()) {
-        ins.flush();
-        RefUpdateUtil.executeChecked(bru, rw);
-
+        if (!dryRun) {
+          ins.flush();
+          RefUpdateUtil.executeChecked(bru, rw);
+        }
       } else {
         progress.skipped++;
       }
     } catch (IOException e) {
       progress.ok = false;
     }
+
     return progress;
   }
 

java/com/google/gerrit/server/schema/Schema_169.java

@@ -72,7 +72,7 @@ public class Schema_169 extends SchemaVersion {
     int skipped = 0;
     for (Project.NameKey project : projects) {
       try (Repository repo = repoManager.openRepository(project)) {
-        ProjectMigrationResult progress = migrator.migrateProject(project, repo);
+        ProjectMigrationResult progress = migrator.migrateProject(project, repo, false);
         skipped += progress.skipped;
       } catch (IOException e) {
         ok = false;

javatests/com/google/gerrit/server/notedb/CommentJsonMigratorTest.java

@@ -38,11 +38,10 @@ import com.google.gerrit.server.notedb.CommentJsonMigrator.ProjectMigrationResul
 import com.google.gerrit.testing.TestChanges;
 import com.google.inject.Inject;
 import java.io.ByteArrayOutputStream;
+import java.util.List;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.eclipse.jgit.junit.TestRepository;
-import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
@@ -92,7 +91,7 @@ public class CommentJsonMigratorTest extends AbstractChangeNotesTest {
             getRevId(notes, 2), ps2Comment.toString());
 
     ChangeNotes oldNotes = notes;
-    migrate(project, 0);
+    checkMigrate(project, ImmutableList.of());
     assertNoDifferences(notes, oldNotes);
     assertThat(notes.getMetaId()).isEqualTo(oldNotes.getMetaId());
   }
@@ -165,7 +164,7 @@ public class CommentJsonMigratorTest extends AbstractChangeNotesTest {
         .containsExactly(ps1Comment1.key, true, ps1Comment2.key, true, ps2Comment1.key, true);
 
     ChangeNotes oldNotes = notes;
-    migrate(project, 1);
+    checkMigrate(project, ImmutableList.of(RefNames.changeMetaRef(c.getId())));
 
     // Comment content is the same.
     notes = newNotes(c);
@@ -267,7 +266,11 @@ public class CommentJsonMigratorTest extends AbstractChangeNotesTest {
         .containsExactly(otherCommentPs1.key, true);
 
     ChangeNotes oldNotes = notes;
-    migrate(allUsers, 2);
+    checkMigrate(
+        allUsers,
+        ImmutableList.of(
+            RefNames.refsDraftComments(c.getId(), changeOwner.getAccountId()),
+            RefNames.refsDraftComments(c.getId(), otherUser.getAccountId())));
     assertNoDifferences(notes, oldNotes);
 
     // Migration doesn't touch change ref.
@@ -368,7 +371,7 @@ public class CommentJsonMigratorTest extends AbstractChangeNotesTest {
         .containsExactly(ps1Comment.key, true, ps2Comment.key, false, ps3Comment.key, true);
 
     ChangeNotes oldNotes = notes;
-    migrate(project, 1);
+    checkMigrate(project, ImmutableList.of(RefNames.changeMetaRef(c.getId())));
     assertNoDifferences(notes, oldNotes);
 
     // Comment content is the same.
@@ -393,23 +396,12 @@ public class CommentJsonMigratorTest extends AbstractChangeNotesTest {
         .containsExactly(ps1Comment.key, false, ps2Comment.key, false, ps3Comment.key, false);
   }
 
-  @FunctionalInterface
-  interface MigrateFunction {
-    boolean call(
-        Project.NameKey project,
-        Repository repo,
-        RevWalk rw,
-        ObjectInserter ins,
-        BatchRefUpdate bru)
-        throws Exception;
-  }
-
-  private void migrate(Project.NameKey project, int expectedCommands) throws Exception {
+  private void checkMigrate(Project.NameKey project, List<String> expectedRefs) throws Exception {
     try (Repository repo = repoManager.openRepository(project)) {
-      ProjectMigrationResult progress = migrator.migrateProject(project, repo);
+      ProjectMigrationResult progress = migrator.migrateProject(project, repo, false);
 
       assertThat(progress.ok).isTrue();
-      assertThat(progress.refsUpdated).isEqualTo(expectedCommands);
+      assertThat(progress.refsUpdated).isEqualTo(expectedRefs);
     }
   }