Fix VersionedMetaData for multiple commits in one batch

I noticed that if you make a modification to the notes tree
(i.e. insert comments) and then make a modification to the commit at
the meta ref (i.e. insert approvals) within one BatchMetaDataUpdate,
then the notes tree update will be squashed by the second update. I
added tests to show this case and corrected this by using the tree
from the parent commit unless there are new contents in the DirCache
to be written.

Change-Id: I83f09e08f472c21dc4bc06f00c3706a18d170f9c

gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java

@@ -240,13 +240,23 @@ public abstract class VersionedMetaData {
           return;
         }
 
-        ObjectId res = newTree.writeTree(inserter);
+        // Reuse tree from parent commit unless there are contents in newTree or
+        // there is no tree for a parent commit.
+        ObjectId res = newTree.getEntryCount() != 0 || srcTree == null
+            ? newTree.writeTree(inserter) : srcTree.copy();
         if (res.equals(srcTree) && !update.allowEmpty()
             && (commit.getTreeId() == null)) {
           // If there are no changes to the content, don't create the commit.
           return;
         }
 
+        // If changes are made to the DirCache and those changes are written as
+        // a commit and then the tree ID is set for the CommitBuilder, then
+        // those previous DirCache changes will be ignored and the commit's
+        // tree will be replaced with the ID in the CommitBuilder. The same is
+        // true if you explicitly set tree ID in a commit and then make changes
+        // to the DirCache; that tree ID will be ignored and replaced by that of
+        // the tree for the updated DirCache.
         if (commit.getTreeId() == null) {
           commit.setTreeId(res);
         } else {

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

@@ -412,7 +412,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
         builder.setTreeId(treeId);
       }
     }
-    batch.write(builder);
+    batch.write(this, builder);
   }
 
   @Override

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

@@ -24,6 +24,7 @@ import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.LinkedListMultimap;
@@ -47,7 +48,9 @@ import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.NullProgressMonitor;
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.notes.Note;
+import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.ReceiveCommand;
 import org.junit.Test;
@@ -375,6 +378,62 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertEquals((short) 2, psas.get(1).getValue());
   }
 
+  @Test
+  public void multipleUpdatesIncludingComments() throws Exception {
+    Change c = newChange();
+    ChangeUpdate update1 = newUpdate(c, otherUser);
+    String uuid1 = "uuid1";
+    String message1 = "comment 1";
+    CommentRange range1 = new CommentRange(1, 1, 2, 1);
+    Timestamp time1 = TimeUtil.nowTs();
+    PatchSet.Id psId = c.currentPatchSetId();
+    BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate();
+    BatchMetaDataUpdate batch = update1.openUpdateInBatch(bru);
+    PatchLineComment comment1 = newPublishedPatchLineComment(psId, "file1",
+        uuid1, range1, range1.getEndLine(), otherUser, null, time1, message1,
+        (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234");
+    update1.setPatchSetId(psId);
+    update1.upsertComment(comment1);
+    update1.writeCommit(batch);
+    ChangeUpdate update2 = newUpdate(c, otherUser);
+    update2.putApproval("Code-Review", (short) 2);
+    update2.writeCommit(batch);
+
+    RevWalk rw = new RevWalk(repo);
+    try {
+      batch.commit();
+      bru.execute(rw, NullProgressMonitor.INSTANCE);
+
+      ChangeNotes notes = newNotes(c);
+      ObjectId tip = notes.getRevision();
+      RevCommit commitWithApprovals = rw.parseCommit(tip);
+      assertNotNull(commitWithApprovals);
+      RevCommit commitWithComments = commitWithApprovals.getParent(0);
+      assertNotNull(commitWithComments);
+
+      ChangeNotesParser notesWithComments =
+          new ChangeNotesParser(c, commitWithComments.copy(), rw, repoManager);
+      notesWithComments.parseAll();
+      ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals1 =
+          notesWithComments.buildApprovals();
+      assertEquals(0, approvals1.size());
+      assertEquals(1, notesWithComments.commentsForBase.size());
+      notesWithComments.close();
+
+      ChangeNotesParser notesWithApprovals =
+          new ChangeNotesParser(c, commitWithApprovals.copy(), rw, repoManager);
+      notesWithApprovals.parseAll();
+      ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals2 =
+          notesWithApprovals.buildApprovals();
+      assertEquals(1, approvals2.size());
+      assertEquals(1, notesWithApprovals.commentsForBase.size());
+      notesWithApprovals.close();
+    } finally {
+      batch.close();
+      rw.release();
+    }
+  }
+
   @Test
   public void multipleUpdatesAcrossRefs() throws Exception {
     Change c1 = newChange();