Map ported comments to patchset-level comments on file deletion

Comments on files which are deleted in the target patchset should not
be simply dropped when we try to port them. Instead, we want to show
them in the next best location.

Before we had patchset-level comments, we intended to use the magic
/COMMIT_MSG file as fallback. With patchset-level comments, those seem
better suited. The only downside is that patchset-level comments don't
appear in the "Files" section (yet).

This change adapts GitPositionTransformer to explicitly handle positions
without file paths and to also support file deletions. The actual
mapping of a no-file position to a patchset-level comment is covered
by the previous change Idd4fbe961.

In theory, this change also fixes the handling of file deletions in the
context of edits due to rebase. It won't have any user-visible impact,
though, as there's no code path relating to edits due to rebase which
will have a different end result now. The reason for this is that we
filter the "mappings" early on with respect to touched files (outside of
GitPositionTransformer and EditTransformer) and hence only
non-deletion/non-addition mappings are given to GitPositionTransformer.

This change also adds a way to express a file addition as mapping. For
GitPositionTransformer, file additions are meaningless and simply
ignored. The API still allows to create such mappings as code paths
using GitPositionTransformer shouldn't need to care about such details
of the transformation algorithm. It's also simpler code wise to
translate all potential diff outcomes to a mapping instead of having to
add additional code for filtering of file additions outside of
GitPositionTransformer.

Change-Id: If8ed1e511a91651f228fc9515311bfd3c4df048d

java/com/google/gerrit/server/patch/DiffMappings.java

@@ -16,7 +16,6 @@ package com.google.gerrit.server.patch;
 
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 
-import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.server.patch.GitPositionTransformer.FileMapping;
 import com.google.gerrit.server.patch.GitPositionTransformer.Mapping;
@@ -29,24 +28,36 @@ public class DiffMappings {
   private DiffMappings() {}
 
   public static Mapping toMapping(PatchListEntry patchListEntry) {
-    // This is just a direct translation of the former logic in EditTransformer. It doesn't
+    FileMapping fileMapping = toFileMapping(patchListEntry);
-    // work for file deletions, though. As file deletions aren't relevant for 'edits due to rebase'
+    ImmutableSet<RangeMapping> rangeMappings = toRangeMappings(patchListEntry);
-    // situations, we didn't notice this in the past.
+    return Mapping.create(fileMapping, rangeMappings);
-    // TODO(aliceks): Fix for file deletions in another change.
+  }
-    FileMapping fileMapping =
+
-        FileMapping.create(getOldFilePath(patchListEntry), patchListEntry.getNewName());
+  private static FileMapping toFileMapping(PatchListEntry patchListEntry) {
-    ImmutableSet<RangeMapping> rangeMappings =
+    switch (patchListEntry.getChangeType()) {
-        patchListEntry.getEdits().stream()
+      case ADDED:
+        return FileMapping.forAddedFile(patchListEntry.getNewName());
+      case MODIFIED:
+      case REWRITE:
+        return FileMapping.forModifiedFile(patchListEntry.getNewName());
+      case DELETED:
+        // Name of deleted file is mentioned as newName.
+        return FileMapping.forDeletedFile(patchListEntry.getNewName());
+      case RENAMED:
+      case COPIED:
+        return FileMapping.forRenamedFile(patchListEntry.getOldName(), patchListEntry.getNewName());
+      default:
+        throw new IllegalStateException("Unmapped diff type: " + patchListEntry.getChangeType());
+    }
+  }
+
+  private static ImmutableSet<RangeMapping> toRangeMappings(PatchListEntry patchListEntry) {
+    return patchListEntry.getEdits().stream()
         .map(
             edit ->
                 RangeMapping.create(
                     Range.create(edit.getBeginA(), edit.getEndA()),
                     Range.create(edit.getBeginB(), edit.getEndB())))
         .collect(toImmutableSet());
-    return Mapping.create(fileMapping, rangeMappings);
-  }
-
-  private static String getOldFilePath(PatchListEntry patchListEntry) {
-    return MoreObjects.firstNonNull(patchListEntry.getOldName(), patchListEntry.getNewName());
   }
 }

java/com/google/gerrit/server/patch/GitPositionTransformer.java

@@ -25,6 +25,7 @@ import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
+import com.google.common.collect.Streams;
 import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
@@ -52,7 +53,8 @@ import java.util.stream.Stream;
  *   <li>Go over all positions and replace the file path for each of them with the corresponding one
  *       in the target tree. If a file path maps to two file paths in the target tree (copied file),
  *       duplicate the position entry and use each of the new file paths with it. If a file path
- *       maps to no file in the target tree (deleted file), drop the position.
+ *       maps to no file in the target tree (deleted file), apply the specified conflict strategy
+ *       (e.g. drop position completely or map to next best guess).
  *   <li>Per file path, go through the file from top to bottom and keep track of how the range
  *       mappings for that file shift the lines. Derive the shifted amount by comparing the number
  *       of lines between source and target in the range mapping. While going through the file,
@@ -97,7 +99,7 @@ public class GitPositionTransformer {
     return shiftRanges(filePathUpdatedEntities, mappings);
   }
 
-  private static <T> ImmutableList<PositionedEntity<T>> updateFilePaths(
+  private <T> ImmutableList<PositionedEntity<T>> updateFilePaths(
       Collection<PositionedEntity<T>> entities, Set<Mapping> mappings) {
     Map<String, ImmutableSet<String>> newFilesPerOldFile = groupNewFilesByOldFiles(mappings);
     return entities.stream()
@@ -108,12 +110,22 @@ public class GitPositionTransformer {
   private static Map<String, ImmutableSet<String>> groupNewFilesByOldFiles(Set<Mapping> mappings) {
     return mappings.stream()
         .map(Mapping::file)
+        // Ignore file additions (irrelevant for mappings).
+        .filter(mapping -> mapping.oldPath().isPresent())
         .collect(
             groupingBy(
-                FileMapping::oldPath, Collectors.mapping(FileMapping::newPath, toImmutableSet())));
+                mapping -> mapping.oldPath().orElse(""),
+                collectingAndThen(
+                    Collectors.mapping(FileMapping::newPath, toImmutableSet()),
+                    // File deletion (empty Optional) -> empty set.
+                    GitPositionTransformer::unwrapOptionals)));
   }
 
-  private static <T> Stream<PositionedEntity<T>> mapToNewFileIfChanged(
+  private static ImmutableSet<String> unwrapOptionals(ImmutableSet<Optional<String>> optionals) {
+    return optionals.stream().flatMap(Streams::stream).collect(toImmutableSet());
+  }
+
+  private <T> Stream<PositionedEntity<T>> mapToNewFileIfChanged(
       Map<String, ? extends Set<String>> newFilesPerOldFile, PositionedEntity<T> entity) {
     if (!entity.position().filePath().isPresent()) {
       // No mapping of file paths necessary if no file path is set. -> Keep existing entry.
@@ -125,6 +137,11 @@ public class GitPositionTransformer {
       return Stream.of(entity);
     }
     Set<String> newFiles = newFilesPerOldFile.get(oldFilePath);
+    if (newFiles.isEmpty()) {
+      // File was deleted.
+      return Streams.stream(
+          positionConflictStrategy.getOnFileConflict(entity.position()).map(entity::withPosition));
+    }
     return newFiles.stream().map(entity::withFilePath);
   }
 
@@ -151,9 +168,11 @@ public class GitPositionTransformer {
   private static Map<String, ImmutableSet<RangeMapping>> groupRangeMappingsByNewFilePath(
       Set<Mapping> mappings) {
     return mappings.stream()
+        // Ignore range mappings of deleted files.
+        .filter(mapping -> mapping.file().newPath().isPresent())
         .collect(
             groupingBy(
-                mapping -> mapping.file().newPath(),
+                mapping -> mapping.file().newPath().orElse(""),
                 collectingAndThen(
                     Collectors.<Mapping, Set<RangeMapping>>reducing(
                         new HashSet<>(), Mapping::ranges, Sets::union),
@@ -271,20 +290,45 @@ public class GitPositionTransformer {
   @AutoValue
   public abstract static class FileMapping {
 
-    /** File path in the source tree. */
+    /** File path in the source tree. For file additions, this is an empty {@link Optional}. */
-    public abstract String oldPath();
+    public abstract Optional<String> oldPath();
-
-    /** File path in the target tree. Can be the same as {@link #oldPath()}. */
-    public abstract String newPath();
 
     /**
-     * Creates a new {@code FileMapping}.
+     * File path in the target tree. Can be the same as {@link #oldPath()} if unchanged. For file
-     *
+     * deletions, this is an empty {@link Optional}.
-     * @param oldPath see {@link #oldPath()}
-     * @param newPath see {@link #newPath()}
      */
-    public static FileMapping create(String oldPath, String newPath) {
+    public abstract Optional<String> newPath();
-      return new AutoValue_GitPositionTransformer_FileMapping(oldPath, newPath);
+
+    /**
+     * Creates a {@link FileMapping} for a file addition.
+     *
+     * <p>In the context of {@link GitPositionTransformer}, file additions are irrelevant as no
+     * given position in the source tree can refer to such a new file in the target tree. We still
+     * provide this factory method so that code outside of {@link GitPositionTransformer} doesn't
+     * have to care about such details and can simply create {@link FileMapping}s for any
+     * modifications between the trees.
+     */
+    public static FileMapping forAddedFile(String filePath) {
+      return new AutoValue_GitPositionTransformer_FileMapping(
+          Optional.empty(), Optional.of(filePath));
+    }
+
+    /** Creates a {@link FileMapping} for a file deletion. */
+    public static FileMapping forDeletedFile(String filePath) {
+      return new AutoValue_GitPositionTransformer_FileMapping(
+          Optional.of(filePath), Optional.empty());
+    }
+
+    /** Creates a {@link FileMapping} for a file modification. */
+    public static FileMapping forModifiedFile(String filePath) {
+      return new AutoValue_GitPositionTransformer_FileMapping(
+          Optional.of(filePath), Optional.of(filePath));
+    }
+
+    /** Creates a {@link FileMapping} for a file renaming. */
+    public static FileMapping forRenamedFile(String oldPath, String newPath) {
+      return new AutoValue_GitPositionTransformer_FileMapping(
+          Optional.of(oldPath), Optional.of(newPath));
     }
   }
 
@@ -536,6 +580,16 @@ public class GitPositionTransformer {
      *     dropped
      */
     Optional<Position> getOnRangeConflict(Position oldPosition);
+
+    /**
+     * Determines an alternate {@link Position} when there is no file for the position (= file
+     * deletion) in the target tree.
+     *
+     * @param oldPosition position in the source tree
+     * @return the new {@link Position} or an empty {@link Optional} if the position should be *
+     *     dropped
+     */
+    Optional<Position> getOnFileConflict(Position oldPosition);
   }
 
   /**
@@ -552,6 +606,11 @@ public class GitPositionTransformer {
     public Optional<Position> getOnRangeConflict(Position oldPosition) {
       return Optional.empty();
     }
+
+    @Override
+    public Optional<Position> getOnFileConflict(Position oldPosition) {
+      return Optional.empty();
+    }
   }
 
   /**
@@ -574,5 +633,11 @@ public class GitPositionTransformer {
     public Optional<Position> getOnRangeConflict(Position oldPosition) {
       return Optional.of(oldPosition.withoutLineRange());
     }
+
+    @Override
+    public Optional<Position> getOnFileConflict(Position oldPosition) {
+      // If there isn't a target file, we can also drop any ranges.
+      return Optional.of(Position.builder().build());
+    }
   }
 }

javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java

@@ -970,8 +970,6 @@ public class PortedCommentsIT extends AbstractDaemonTest {
   }
 
   @Test
-  // TODO(aliceks): Correct handling of deleted files.
-  @Ignore
   public void portedRangeCommentBecomesPatchsetLevelCommentOnFileDeletion() throws Exception {
     // Set up change and patchsets.
     Change.Id changeId =
@@ -1295,8 +1293,6 @@ public class PortedCommentsIT extends AbstractDaemonTest {
   }
 
   @Test
-  // TODO(aliceks): Correct handling of deleted files.
-  @Ignore
   public void portedLineCommentBecomesPatchsetLevelCommentOnFileDeletion() throws Exception {
     // Set up change and patchsets.
     Change.Id changeId =
@@ -1398,8 +1394,6 @@ public class PortedCommentsIT extends AbstractDaemonTest {
   }
 
   @Test
-  // TODO(aliceks): Correct handling of deleted files.
-  @Ignore
   public void portedFileCommentBecomesPatchsetLevelCommentOnFileDeletion() throws Exception {
     // Set up change and patchsets.
     Change.Id changeId =