Merge "edits/TreeCreator: read base commit when creating the DirCache object"

java/com/google/gerrit/server/edit/tree/TreeCreator.java

diff --git a/java/com/google/gerrit/server/edit/tree/TreeCreator.java b/java/com/google/gerrit/server/edit/tree/TreeCreator.java
index b3ce564..ae80fe4 100644
--- a/java/com/google/gerrit/server/edit/tree/TreeCreator.java
+++ b/java/com/google/gerrit/server/edit/tree/TreeCreator.java
@@ -42,40 +42,49 @@
   private final ObjectId baseTreeId;
   private final ImmutableList<? extends ObjectId> baseParents;
   private final Optional<ObjectInserter> objectInserter;
+  private final Optional<ObjectReader> objectReader;
   private final List<TreeModification> treeModifications = new ArrayList<>();
 
   public static TreeCreator basedOn(RevCommit baseCommit) {
     requireNonNull(baseCommit, "baseCommit is required");
     return new TreeCreator(
-        baseCommit.getTree(), ImmutableList.copyOf(baseCommit.getParents()), Optional.empty());
+        baseCommit.getTree(),
+        ImmutableList.copyOf(baseCommit.getParents()),
+        Optional.empty(),
+        Optional.empty());
   }
 
   @UsedAt(UsedAt.Project.GOOGLE)
-  public static TreeCreator basedOn(RevCommit baseCommit, ObjectInserter objectInserter) {
+  public static TreeCreator basedOn(
+      RevCommit baseCommit, ObjectInserter objectInserter, ObjectReader objectReader) {
     requireNonNull(baseCommit, "baseCommit is required");
     return new TreeCreator(
         baseCommit.getTree(),
         ImmutableList.copyOf(baseCommit.getParents()),
-        Optional.of(objectInserter));
+        Optional.of(objectInserter),
+        Optional.of(objectReader));
   }
 
   public static TreeCreator basedOnTree(
       ObjectId baseTreeId, ImmutableList<? extends ObjectId> baseParents) {
     requireNonNull(baseTreeId, "baseTreeId is required");
-    return new TreeCreator(baseTreeId, baseParents, Optional.empty());
+    return new TreeCreator(baseTreeId, baseParents, Optional.empty(), Optional.empty());
   }
 
   public static TreeCreator basedOnEmptyTree() {
-    return new TreeCreator(ObjectId.zeroId(), ImmutableList.of(), Optional.empty());
+    return new TreeCreator(
+        ObjectId.zeroId(), ImmutableList.of(), Optional.empty(), Optional.empty());
   }
 
   private TreeCreator(
       ObjectId baseTreeId,
       ImmutableList<? extends ObjectId> baseParents,
-      Optional<ObjectInserter> objectInserter) {
+      Optional<ObjectInserter> objectInserter,
+      Optional<ObjectReader> objectReader) {
     this.baseTreeId = requireNonNull(baseTreeId, "baseTree is required");
     this.baseParents = baseParents;
     this.objectInserter = objectInserter;
+    this.objectReader = objectReader;
   }
 
   /**
@@ -138,14 +147,22 @@
   }
 
   private DirCache readBaseTree(Repository repository) throws IOException {
-    try (ObjectReader objectReader = repository.newObjectReader()) {
-      DirCache dirCache = DirCache.newInCore();
+    ObjectReader or = objectReader.orElseGet(() -> repository.newObjectReader());
+    try {
+      DirCache dirCache =
+          ObjectId.zeroId().equals(baseTreeId)
+              ? DirCache.newInCore()
+              : DirCache.read(or, baseTreeId);
       DirCacheBuilder dirCacheBuilder = dirCache.builder();
       if (!ObjectId.zeroId().equals(baseTreeId)) {
-        dirCacheBuilder.addTree(new byte[0], DirCacheEntry.STAGE_0, objectReader, baseTreeId);
+        dirCacheBuilder.addTree(new byte[0], DirCacheEntry.STAGE_0, or, baseTreeId);
       }
       dirCacheBuilder.finish();
       return dirCache;
+    } finally {
+      if (objectReader.isEmpty()) {
+        or.close();
+      }
     }
   }
 

javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java

diff --git a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
index 62a095f..6cfb989 100644
--- a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
+++ b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
@@ -1371,8 +1371,21 @@
 
   @Test
   public void canCombineEdits() throws Exception {
-    createEmptyEditFor(changeId);
+    String baseFileToDelete = "base_file_to_delete";
+    String baseFileToRename = "base_file_to_rename";
+    String baseFileNewName = "base_file_new_name";
+    String currPatchSetFileToRename = "current_patchset_file_to_rename";
+    String currPatchSetFileNewName = "current_patchset_file_new_name";
+    // Re-clone empty repo; TestRepository doesn't let us reset to unborn head.
+    testRepo = cloneProject(project);
+    String baseChangeId = newChangeWithFile(admin.newIdent(), baseFileToDelete, "content");
+    addNewPatchSetWithModifiedFile(baseChangeId, baseFileToRename, "content2");
+    gApi.changes().id(baseChangeId).current().review(ReviewInput.approve());
+    gApi.changes().id(baseChangeId).current().submit();
+    String changeId = newChange(admin.newIdent());
+    addNewPatchSetWithModifiedFile(changeId, currPatchSetFileToRename, "content3");
 
+    createEmptyEditFor(changeId);
     // update author
     gApi.changes()
         .id(changeId)
@@ -1394,11 +1407,20 @@
         .modifyIdentity(
             "Test Committer", "test.committer@example.com", ChangeEditIdentityType.COMMITTER);
 
-    // delete file
+    // delete current patch-set file
     gApi.changes().id(changeId).edit().deleteFile(FILE_NAME);
 
-    // rename file
-    gApi.changes().id(changeId).edit().renameFile(FILE_NAME2, FILE_NAME3);
+    // delete base file
+    gApi.changes().id(changeId).edit().deleteFile(baseFileToDelete);
+
+    // rename current patch-set file
+    gApi.changes()
+        .id(changeId)
+        .edit()
+        .renameFile(currPatchSetFileToRename, currPatchSetFileNewName);
+
+    // rename base file
+    gApi.changes().id(changeId).edit().renameFile(baseFileToRename, baseFileNewName);
 
     // publish edit
     PublishChangeEditInput publishInput = new PublishChangeEditInput();
@@ -1419,7 +1441,12 @@
     assertThat(currentCommit.author.name).isEqualTo("Test Author");
     assertThat(currentCommit.author.email).isEqualTo("test.author@example.com");
     assertThat(currentCommit.message).isEqualTo(msg);
-    assertThat(currentRevision.files.keySet()).containsExactly(newFile, FILE_NAME3);
+    assertThat(currentRevision.files.keySet())
+        .containsExactly(newFile, baseFileToDelete, baseFileNewName, currPatchSetFileNewName);
+    assertThat(currentRevision.files.get(newFile).status).isEqualTo('A');
+    assertThat(currentRevision.files.get(baseFileToDelete).status).isEqualTo('D');
+    assertThat(currentRevision.files.get(baseFileNewName).status).isEqualTo('R');
+    assertThat(currentRevision.files.get(currPatchSetFileNewName).status).isEqualTo('A');
   }
 
   private void createArbitraryEditFor(String changeId) throws Exception {
@@ -1451,6 +1478,13 @@
     return push.to("refs/for/master").getChangeId();
   }
 
+  private String newChangeWithFile(PersonIdent ident, String filePath, String fileContent)
+      throws Exception {
+    PushOneCommit push =
+        pushFactory.create(ident, testRepo, PushOneCommit.SUBJECT, filePath, fileContent);
+    return push.to("refs/for/master").getChangeId();
+  }
+
   private void addNewPatchSet(String changeId) throws Exception {
     addNewPatchSetWithModifiedFile(changeId, FILE_NAME2, new String(CONTENT_NEW2, UTF_8));
   }