TreeCreator: Explicitly reject overlapping modifications

The current implementation of TreeCreator can't handle TreeModifications
which refer to the same files even when those modifications are
specified in a logical order. Fixing this turned out to be more
difficult than expected as TreeCreator acts on top of JGit library
functions and those functions don't seem to support such overlapping
modifications out of the box.

None of our production code can currently create such overlapping
modifications. As TreeCreator behaves unexpectedly when presented with
such TreeModifications (sometimes throwing an exception, sometimes
creating a new tree with some ignored modifications), we thought it was
still helpful to explicitly throw an exception and document that
unsupported behavior.

That exception also proves useful for the test API of changes. When
developers try to create a new patchset with impossible file
combinations (e.g. delete + add the same file; rename without content
modification + provide different content for new file name), they see
a message which explicitly warns them instead of a strange error or a
silently but possibly wrongly succeeding test.

Change-Id: I17a20a2bc5a16ac7719d04fe1a7c3e2dcbbef5d6

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

@@ -19,6 +19,7 @@ import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.flogger.FluentLogger;
 import com.google.common.io.ByteStreams;
 import com.google.gerrit.extensions.restapi.RawInput;
@@ -55,8 +56,8 @@ public class ChangeFileContentModification implements TreeModification {
   }
 
   @Override
-  public String getFilePath() {
-    return filePath;
+  public ImmutableSet<String> getFilePaths() {
+    return ImmutableSet.of(filePath);
   }
 
   @VisibleForTesting

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

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.edit.tree;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import java.util.Collections;
 import java.util.List;
 import org.eclipse.jgit.dircache.DirCacheEditor;
@@ -38,7 +39,7 @@ public class DeleteFileModification implements TreeModification {
   }
 
   @Override
-  public String getFilePath() {
-    return filePath;
+  public ImmutableSet<String> getFilePaths() {
+    return ImmutableSet.of(filePath);
   }
 }

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

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.edit.tree;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import java.io.IOException;
 import java.util.List;
 import org.eclipse.jgit.dircache.DirCacheEditor;
@@ -57,7 +58,7 @@ public class RenameFileModification implements TreeModification {
   }
 
   @Override
-  public String getFilePath() {
-    return newFilePath;
+  public ImmutableSet<String> getFilePaths() {
+    return ImmutableSet.of(currentFilePath, newFilePath);
   }
 }

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

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.edit.tree;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import java.io.IOException;
 import java.util.Collections;
 import java.util.List;
@@ -62,7 +63,7 @@ public class RestoreFileModification implements TreeModification {
   }
 
   @Override
-  public String getFilePath() {
-    return filePath;
+  public ImmutableSet<String> getFilePaths() {
+    return ImmutableSet.of(filePath);
   }
 }

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

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.edit.tree;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static java.util.Objects.requireNonNull;
 
 import com.google.common.collect.ImmutableList;
@@ -56,7 +57,11 @@ public class TreeCreator {
 
   /**
    * Apply modifications to the tree which is taken as a basis. If this method is called multiple
-   * times, the modifications are applied subsequently in exactly the order they were provided.
+   * times, the modifications are applied subsequently in exactly the order they were provided
+   * (though JGit applies some internal optimizations which involve sorting, too).
+   *
+   * <p><strong>Beware:</strong> All provided {@link TreeModification}s (even from previous calls of
+   * this method) must touch different file paths!
    *
    * @param treeModifications modifications which should be applied to the base tree
    */
@@ -75,10 +80,32 @@ public class TreeCreator {
    * @throws IOException if problems arise when accessing the repository
    */
   public ObjectId createNewTreeAndGetId(Repository repository) throws IOException {
+    ensureTreeModificationsDoNotTouchSameFiles();
     DirCache newTree = createNewTree(repository);
     return writeAndGetId(repository, newTree);
   }
 
+  private void ensureTreeModificationsDoNotTouchSameFiles() {
+    // The current implementation of TreeCreator doesn't properly support modifications which touch
+    // the same files even if they are provided in a logical order. According to JGit's
+    // documentation, DirCache applies some internal sorting to optimize the index modifications.
+    // The internal sorting doesn't seem to be the only issue, though. Even applying the
+    // modifications in batches within different, subsequent DirCaches just held in memory didn't
+    // seem to work. We might need to fully write each batch to disk before creating the next.
+    ImmutableList<String> filePaths =
+        treeModifications.stream()
+            .flatMap(treeModification -> treeModification.getFilePaths().stream())
+            .collect(toImmutableList());
+    long distinctFilePathNum = filePaths.stream().distinct().count();
+    if (filePaths.size() != distinctFilePathNum) {
+      throw new IllegalStateException(
+          String.format(
+              "TreeModifications must not refer to the same file paths. This would have"
+                  + " unexpected/wrong behavior! Found file paths: %s.",
+              filePaths));
+    }
+  }
+
   private DirCache createNewTree(Repository repository) throws IOException {
     DirCache newTree = readBaseTree(repository);
     List<DirCacheEditor.PathEdit> pathEdits = getPathEdits(repository);

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

@@ -14,8 +14,8 @@
 
 package com.google.gerrit.server.edit.tree;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import java.io.IOException;
 import java.util.List;
 import org.eclipse.jgit.dircache.DirCacheEditor;
@@ -42,12 +42,10 @@ public interface TreeModification {
       throws IOException;
 
   /**
-   * Indicates a file path which is affected by this {@code TreeModification}. If the modification
-   * refers to several file paths (e.g. renaming a file), returning either of them is appropriate as
-   * long as the returned value is deterministic.
+   * Indicates all file paths affected by this {@code TreeModification}. If the modification refers
+   * to several file paths (e.g. renaming a file), all of them must be returned.
    *
-   * @return an affected file path
+   * @return all affected file paths
    */
-  @VisibleForTesting
-  String getFilePath();
+  ImmutableSet<String> getFilePaths();
 }

javatests/com/google/gerrit/server/edit/tree/ChangeFileContentModificationSubject.java

@@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertAbout;
 
 import com.google.common.io.CharStreams;
 import com.google.common.truth.FailureMetadata;
+import com.google.common.truth.IterableSubject;
 import com.google.common.truth.StringSubject;
 import com.google.common.truth.Subject;
 import com.google.gerrit.extensions.restapi.RawInput;
@@ -45,9 +46,9 @@ public class ChangeFileContentModificationSubject extends Subject {
     this.modification = modification;
   }
 
-  public StringSubject filePath() {
+  public IterableSubject filePaths() {
     isNotNull();
-    return check("getFilePath()").that(modification.getFilePath());
+    return check("getFilePaths()").that(modification.getFilePaths());
   }
 
   public StringSubject newContent() throws IOException {

javatests/com/google/gerrit/server/edit/tree/TreeCreatorTest.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.edit.tree;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
@@ -92,6 +93,48 @@ public class TreeCreatorTest {
     assertThat(isEmptyTree(newTreeId)).isTrue();
   }
 
+  @Test
+  public void modificationsMustNotReferToSameFilePaths() {
+    TreeCreator treeCreator = TreeCreator.basedOnEmptyTree();
+    treeCreator.addTreeModifications(
+        ImmutableList.of(
+            new RenameFileModification("oldFileName", "newFileName"),
+            new ChangeFileContentModification(
+                "newFileName", RawInputUtil.create("Different content"))));
+    IllegalStateException exception =
+        assertThrows(
+            IllegalStateException.class, () -> treeCreator.createNewTreeAndGetId(repository));
+
+    assertThat(exception).hasMessageThat().contains("oldFileName");
+    assertThat(exception).hasMessageThat().contains("newFileName");
+  }
+
+  @Test
+  public void fileContentModificationRefersToModifiedFile() {
+    ChangeFileContentModification contentModification =
+        new ChangeFileContentModification("myFileName", RawInputUtil.create("Some content"));
+    assertThat(contentModification.getFilePaths()).containsExactly("myFileName");
+  }
+
+  @Test
+  public void renameFileModificationRefersToOldAndNewFilePath() {
+    RenameFileModification fileModification =
+        new RenameFileModification("oldFileName", "newFileName");
+    assertThat(fileModification.getFilePaths()).containsExactly("oldFileName", "newFileName");
+  }
+
+  @Test
+  public void deleteFileModificationRefersToDeletedFile() {
+    DeleteFileModification fileModification = new DeleteFileModification("myFileName");
+    assertThat(fileModification.getFilePaths()).containsExactly("myFileName");
+  }
+
+  @Test
+  public void restoreFileModificationRefersToRestoredFile() {
+    RestoreFileModification fileModification = new RestoreFileModification("myFileName");
+    assertThat(fileModification.getFilePaths()).containsExactly("myFileName");
+  }
+
   private String getFileContent(ObjectId treeId, String filePath) throws Exception {
     try (RevWalk revWalk = new RevWalk(repository);
         ObjectReader reader = revWalk.getObjectReader()) {

javatests/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.fixes;
 
 import static com.google.gerrit.server.edit.tree.TreeModificationSubject.assertThatList;
 import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static java.util.Comparator.comparing;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -28,7 +29,6 @@ import com.google.gerrit.server.change.FileContentUtil;
 import com.google.gerrit.server.edit.tree.TreeModification;
 import com.google.gerrit.server.project.ProjectState;
 import java.util.ArrayList;
-import java.util.Comparator;
 import java.util.List;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
@@ -73,8 +73,8 @@ public class FixReplacementInterpreterTest {
     assertThatList(sortedTreeModifications)
         .element(0)
         .asChangeFileContentModification()
-        .filePath()
-        .isEqualTo(filePath1);
+        .filePaths()
+        .containsExactly(filePath1);
     assertThatList(sortedTreeModifications)
         .element(0)
         .asChangeFileContentModification()
@@ -83,8 +83,8 @@ public class FixReplacementInterpreterTest {
     assertThatList(sortedTreeModifications)
         .element(1)
         .asChangeFileContentModification()
-        .filePath()
-        .isEqualTo(filePath2);
+        .filePaths()
+        .containsExactly(filePath2);
     assertThatList(sortedTreeModifications)
         .element(1)
         .asChangeFileContentModification()
@@ -340,7 +340,10 @@ public class FixReplacementInterpreterTest {
 
   private static List<TreeModification> getSortedCopy(List<TreeModification> treeModifications) {
     List<TreeModification> sortedTreeModifications = new ArrayList<>(treeModifications);
-    sortedTreeModifications.sort(Comparator.comparing(TreeModification::getFilePath));
+    // The sorting is only necessary to get a deterministic order. The exact order doesn't matter.
+    sortedTreeModifications.sort(
+        comparing(
+            treeModification -> treeModification.getFilePaths().stream().findFirst().orElse("")));
     return sortedTreeModifications;
   }
 }