ChangeNotesParser: Don't open the repository

This was left over from long before passing in a previously opened
RevWalk; the repo that is opened is never used. With this change,
ChangeNotesParser no longer needs to be AutoCloseable either.

Change-Id: I46f2ddb6e81105635b7384012e33479ae4a2fd24

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

@@ -548,54 +548,52 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
       loadDefaults();
       return;
     }
-    try (ChangeNotesParser parser = new ChangeNotesParser(
+    ChangeNotesParser parser = new ChangeNotesParser(
-         project, change.getId(), rev, handle.walk(), args.repoManager,
+        change.getId(), rev, handle.walk(), args.noteUtil, args.metrics);
-         args.noteUtil, args.metrics)) {
+    parser.parseAll();
-      parser.parseAll();
 
-      if (parser.status != null) {
+    if (parser.status != null) {
-        change.setStatus(parser.status);
+      change.setStatus(parser.status);
-      }
-      approvals = parser.buildApprovals();
-      changeMessagesByPatchSet = parser.buildMessagesByPatchSet();
-      allChangeMessages = parser.buildAllMessages();
-      comments = ImmutableListMultimap.copyOf(parser.comments);
-      revisionNoteMap = parser.revisionNoteMap;
-      change.setKey(new Change.Key(parser.changeId));
-      change.setDest(new Branch.NameKey(project, parser.branch));
-      change.setTopic(Strings.emptyToNull(parser.topic));
-      change.setCreatedOn(parser.createdOn);
-      change.setLastUpdatedOn(parser.lastUpdatedOn);
-      change.setOwner(parser.ownerId);
-      change.setSubmissionId(parser.submissionId);
-      patchSets = ImmutableSortedMap.copyOf(
-          parser.patchSets, ReviewDbUtil.intKeyOrdering());
-
-      if (!patchSets.isEmpty()) {
-        change.setCurrentPatchSet(
-            parser.currentPatchSetId, parser.subject, parser.originalSubject);
-      } else {
-        // TODO(dborowitz): This should be an error, but for now it's required
-        // for some tests to pass.
-        change.clearCurrentPatchSet();
-      }
-
-      if (parser.hashtags != null) {
-        hashtags = ImmutableSet.copyOf(parser.hashtags);
-      } else {
-        hashtags = ImmutableSet.of();
-      }
-      ImmutableSetMultimap.Builder<ReviewerStateInternal, Account.Id> reviewers =
-          ImmutableSetMultimap.builder();
-      for (Map.Entry<Account.Id, ReviewerStateInternal> e
-          : parser.reviewers.entrySet()) {
-        reviewers.put(e.getValue(), e.getKey());
-      }
-      this.reviewers = reviewers.build();
-      this.allPastReviewers = ImmutableList.copyOf(parser.allPastReviewers);
-
-      submitRecords = ImmutableList.copyOf(parser.submitRecords);
     }
+    approvals = parser.buildApprovals();
+    changeMessagesByPatchSet = parser.buildMessagesByPatchSet();
+    allChangeMessages = parser.buildAllMessages();
+    comments = ImmutableListMultimap.copyOf(parser.comments);
+    revisionNoteMap = parser.revisionNoteMap;
+    change.setKey(new Change.Key(parser.changeId));
+    change.setDest(new Branch.NameKey(project, parser.branch));
+    change.setTopic(Strings.emptyToNull(parser.topic));
+    change.setCreatedOn(parser.createdOn);
+    change.setLastUpdatedOn(parser.lastUpdatedOn);
+    change.setOwner(parser.ownerId);
+    change.setSubmissionId(parser.submissionId);
+    patchSets = ImmutableSortedMap.copyOf(
+        parser.patchSets, ReviewDbUtil.intKeyOrdering());
+
+    if (!patchSets.isEmpty()) {
+      change.setCurrentPatchSet(
+          parser.currentPatchSetId, parser.subject, parser.originalSubject);
+    } else {
+      // TODO(dborowitz): This should be an error, but for now it's required for
+      // some tests to pass.
+      change.clearCurrentPatchSet();
+    }
+
+    if (parser.hashtags != null) {
+      hashtags = ImmutableSet.copyOf(parser.hashtags);
+    } else {
+      hashtags = ImmutableSet.of();
+    }
+    ImmutableSetMultimap.Builder<ReviewerStateInternal, Account.Id> reviewers =
+        ImmutableSetMultimap.builder();
+    for (Map.Entry<Account.Id, ReviewerStateInternal> e
+        : parser.reviewers.entrySet()) {
+      reviewers.put(e.getValue(), e.getKey());
+    }
+    this.reviewers = reviewers.build();
+    this.allPastReviewers = ImmutableList.copyOf(parser.allPastReviewers);
+
+    submitRecords = ImmutableList.copyOf(parser.submitRecords);
   }
 
   @Override

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

@@ -57,21 +57,17 @@ import com.google.gerrit.reviewdb.client.LabelId;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
-import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDbUtil;
-import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
 import com.google.gerrit.server.util.LabelVote;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.errors.InvalidObjectIdException;
-import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.lib.PersonIdent;
-import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.notes.NoteMap;
 import org.eclipse.jgit.revwalk.FooterKey;
 import org.eclipse.jgit.util.RawParseUtils;
@@ -92,7 +88,7 @@ import java.util.NavigableSet;
 import java.util.Set;
 import java.util.TreeMap;
 
-class ChangeNotesParser implements AutoCloseable {
+class ChangeNotesParser {
   // Sentinel RevId indicating a mutable field on a patch set was parsed, but
   // the parser does not yet know its commit SHA-1.
   private static final RevId PARTIAL_PATCH_SET =
@@ -125,20 +121,16 @@ class ChangeNotesParser implements AutoCloseable {
   private final Change.Id id;
   private final ObjectId tip;
   private final ChangeNotesRevWalk walk;
-  private final Repository repo;
   private final Map<PatchSet.Id,
       Table<Account.Id, Entry<String, String>, Optional<PatchSetApproval>>> approvals;
   private final List<ChangeMessage> allChangeMessages;
   private final Multimap<PatchSet.Id, ChangeMessage> changeMessagesByPatchSet;
 
-  ChangeNotesParser(Project.NameKey project, Change.Id changeId, ObjectId tip,
+  ChangeNotesParser(Change.Id changeId, ObjectId tip, ChangeNotesRevWalk walk,
-      ChangeNotesRevWalk walk, GitRepositoryManager repoManager,
+      ChangeNoteUtil noteUtil, NoteDbMetrics metrics) {
-      ChangeNoteUtil noteUtil, NoteDbMetrics metrics)
-      throws RepositoryNotFoundException, IOException {
     this.id = changeId;
     this.tip = tip;
     this.walk = walk;
-    this.repo = repoManager.openRepository(project);
     this.noteUtil = noteUtil;
     this.metrics = metrics;
     approvals = new HashMap<>();
@@ -152,11 +144,6 @@ class ChangeNotesParser implements AutoCloseable {
     patchSetStates = new HashMap<>();
   }
 
-  @Override
-  public void close() {
-    repo.close();
-  }
-
   void parseAll() throws ConfigInvalidException, IOException {
     // Don't include initial parse in timer, as this might do more I/O to page
     // in the block containing most commits. Later reads are not guaranteed to

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java

@@ -17,7 +17,6 @@ package com.google.gerrit.server.notedb;
 import static org.junit.Assert.fail;
 
 import com.google.gerrit.common.TimeUtil;
-import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
@@ -482,9 +481,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
   }
 
   private void assertParseSucceeds(RevCommit commit) throws Exception {
-    try (ChangeNotesParser parser = newParser(commit)) {
+    newParser(commit).parseAll();
-      parser.parseAll();
-    }
   }
 
   private void assertParseFails(String body) throws Exception {
@@ -492,8 +489,8 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
   }
 
   private void assertParseFails(RevCommit commit) throws Exception {
-    try (ChangeNotesParser parser = newParser(commit)) {
+    try {
-      parser.parseAll();
+      newParser(commit).parseAll();
       fail("Expected parse to fail:\n" + commit.getFullMessage());
     } catch (ConfigInvalidException e) {
       // Expected
@@ -501,8 +498,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
   }
 
   private ChangeNotesParser newParser(ObjectId tip) throws Exception {
-    Change c = newChange();
+    return new ChangeNotesParser(
-    return new ChangeNotesParser(c.getProject(), c.getId(), tip, walk,
+        newChange().getId(), tip, walk, noteUtil, args.metrics);
-        repoManager, noteUtil, args.metrics);
   }
 }

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java

@@ -1018,27 +1018,23 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertThat(commitWithComments).isNotNull();
 
     try (ChangeNotesRevWalk rw = ChangeNotesCommit.newRevWalk(repo)) {
-      try (ChangeNotesParser notesWithComments = new ChangeNotesParser(
+      ChangeNotesParser notesWithComments = new ChangeNotesParser(
-          project, c.getId(), commitWithComments.copy(), rw, repoManager,
+          c.getId(), commitWithComments.copy(), rw, noteUtil, args.metrics);
-          noteUtil, args.metrics)) {
+      notesWithComments.parseAll();
-        notesWithComments.parseAll();
+      ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals1 =
-        ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals1 =
+          notesWithComments.buildApprovals();
-            notesWithComments.buildApprovals();
+      assertThat(approvals1).isEmpty();
-        assertThat(approvals1).isEmpty();
+      assertThat(notesWithComments.comments).hasSize(1);
-        assertThat(notesWithComments.comments).hasSize(1);
-      }
     }
 
     try (ChangeNotesRevWalk rw = ChangeNotesCommit.newRevWalk(repo)) {
-      try (ChangeNotesParser notesWithApprovals = new ChangeNotesParser(project,
+      ChangeNotesParser notesWithApprovals = new ChangeNotesParser(c.getId(),
-          c.getId(), commitWithApprovals.copy(), rw, repoManager,
+          commitWithApprovals.copy(), rw, noteUtil, args.metrics);
-          noteUtil, args.metrics)) {
+      notesWithApprovals.parseAll();
-        notesWithApprovals.parseAll();
+      ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals2 =
-        ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals2 =
+          notesWithApprovals.buildApprovals();
-            notesWithApprovals.buildApprovals();
+      assertThat(approvals2).hasSize(1);
-        assertThat(approvals2).hasSize(1);
+      assertThat(notesWithApprovals.comments).hasSize(1);
-        assertThat(notesWithApprovals.comments).hasSize(1);
-      }
     }
   }