Always show comments in patch views, even if no edit exists

If a comment exists but is not within the context region of an edit,
we still want to ensure the comment shows to the reader, as it may
still be an important part of the review.

By creating a 0-length edit around a visible comment we ensure that
the packContent() method will format the necessary context lines to
the destination buffers, and we also ensure that the patch views in
the client UI will draw a hunk around the region.

Bug: GERRIT-66
Signed-off-by: Shawn O. Pearce <sop@google.com>

5 files changed, 186 insertions, 40 deletions

diff --git a/src/main/java/com/google/gerrit/server/patch/DiffCacheContent.java b/src/main/java/com/google/gerrit/server/patch/DiffCacheContent.java
index 0427882e69..80730e4293 100644
--- a/src/main/java/com/google/gerrit/server/patch/DiffCacheContent.java
+++ b/src/main/java/com/google/gerrit/server/patch/DiffCacheContent.java
@@ -32,12 +32,16 @@ import java.io.IOException;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
 import java.io.Serializable;
-import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 
 public final class DiffCacheContent implements Serializable {
-  private static final long serialVersionUID = 2L;
+  // Note: If we modify our version, also modify DiffCacheKey, so
+  // the on disk cache is fully destroyed and recreated when the
+  // schema has changed.
+  //
+  private static final long serialVersionUID = 3L;
 
   public static DiffCacheContent create(final Repository db,
       final DiffCacheKey key, final FileHeader file)
@@ -64,6 +68,16 @@ public final class DiffCacheContent implements Serializable {
     return new DiffCacheContent(file, o, n);
   }
 
+  public static DiffCacheContent createEmpty(final Repository db,
+      final ObjectId treeIsh, final String path) throws MissingObjectException,
+      IncorrectObjectTypeException, IOException {
+    final ObjectId blob = find(db, treeIsh, path);
+    if (blob == null) {
+      throw new IOException("path \"" + path + "\" not in " + treeIsh.name());
+    }
+    return new DiffCacheContent(blob);
+  }
+
   private static ObjectId toId(final AbbreviatedObjectId a) {
     final ObjectId o = a.toObjectId();
     return ObjectId.zeroId().equals(o) ? null : o;
@@ -80,32 +94,33 @@ public final class DiffCacheContent implements Serializable {
     return tw.getObjectId(0);
   }
 
-  private transient boolean noDifference;
   private transient ObjectId oldId;
   private transient ObjectId newId;
   private transient FileHeader header;
   private transient List<Edit> edits;
 
-  public DiffCacheContent() {
-    noDifference = true;
+  private DiffCacheContent(final ObjectId o) {
+    oldId = o;
+    newId = o;
+    header = null;
+    edits = Collections.emptyList();
   }
 
   private DiffCacheContent(final FileHeader h, final ObjectId o,
       final ObjectId n) {
-    noDifference = false;
     header = compact(h);
     oldId = o;
     newId = n;
 
-    if (h instanceof CombinedFileHeader || h.getHunks().isEmpty()) {
+    if (h == null || h instanceof CombinedFileHeader || h.getHunks().isEmpty()) {
       edits = Collections.emptyList();
     } else {
-      edits = h.toEditList();
+      edits = Collections.unmodifiableList(h.toEditList());
     }
   }
 
   public boolean isNoDifference() {
-    return noDifference;
+    return header == null;
   }
 
   public ObjectId getOldId() {
@@ -125,17 +140,16 @@ public final class DiffCacheContent implements Serializable {
   }
 
   private void writeObject(final ObjectOutputStream out) throws IOException {
-    out.writeBoolean(noDifference);
-    if (noDifference) {
-      return;
-    }
-
     ObjectIdSerialization.write(out, oldId);
     ObjectIdSerialization.write(out, newId);
 
-    final int len = end(header) - header.getStartOffset();
-    out.writeInt(len);
-    out.write(header.getBuffer(), header.getStartOffset(), len);
+    if (header != null) {
+      final int hdrLen = end(header) - header.getStartOffset();
+      out.writeInt(hdrLen);
+      out.write(header.getBuffer(), header.getStartOffset(), hdrLen);
+    } else {
+      out.writeInt(0);
+    }
 
     out.writeInt(edits.size());
     for (final Edit e : edits) {
@@ -147,26 +161,31 @@ public final class DiffCacheContent implements Serializable {
   }
 
   private void readObject(final ObjectInputStream in) throws IOException {
-    noDifference = in.readBoolean();
-    if (noDifference) {
-      return;
-    }
-
     oldId = ObjectIdSerialization.read(in);
     newId = ObjectIdSerialization.read(in);
 
-    final byte[] buf = new byte[in.readInt()];
-    in.readFully(buf);
-    header = parse(buf);
+    final int hdrLen = in.readInt();
+    if (hdrLen > 0) {
+      final byte[] buf = new byte[hdrLen];
+      in.readFully(buf);
+      header = parse(buf);
+    } else {
+      header = null;
+    }
 
     final int editCount = in.readInt();
-    edits = new ArrayList<Edit>(editCount);
-    for (int n = editCount - 1; 0 <= n; n--) {
-      final int beginA = in.readInt();
-      final int endA = in.readInt();
-      final int beginB = in.readInt();
-      final int endB = in.readInt();
-      edits.add(new Edit(beginA, endA, beginB, endB));
+    if (editCount > 0) {
+      final Edit[] editArray = new Edit[editCount];
+      for (int i = 0; i < editCount; i++) {
+        final int beginA = in.readInt();
+        final int endA = in.readInt();
+        final int beginB = in.readInt();
+        final int endB = in.readInt();
+        editArray[i] = new Edit(beginA, endA, beginB, endB);
+      }
+      edits = Collections.unmodifiableList(Arrays.asList(editArray));
+    } else {
+      edits = Collections.emptyList();
     }
   }
 
diff --git a/src/main/java/com/google/gerrit/server/patch/DiffCacheEntryFactory.java b/src/main/java/com/google/gerrit/server/patch/DiffCacheEntryFactory.java
index 43820d15b9..aab9f3631a 100644
--- a/src/main/java/com/google/gerrit/server/patch/DiffCacheEntryFactory.java
+++ b/src/main/java/com/google/gerrit/server/patch/DiffCacheEntryFactory.java
@@ -20,6 +20,7 @@ import com.google.gerrit.server.GerritServer;
 
 import net.sf.ehcache.constructs.blocking.CacheEntryFactory;
 
+import org.spearce.jgit.lib.ObjectId;
 import org.spearce.jgit.lib.Repository;
 import org.spearce.jgit.patch.FileHeader;
 
@@ -37,6 +38,7 @@ public class DiffCacheEntryFactory implements CacheEntryFactory {
   public Object createEntry(Object genericKey) throws Exception {
     final DiffCacheKey key = (DiffCacheKey) genericKey;
     final Repository db = open(key.getProjectKey());
+    final ObjectId newId = key.getNewId();
     final List<String> args = new ArrayList<String>();
     args.add("git");
     args.add("--git-dir=.");
@@ -52,7 +54,7 @@ public class DiffCacheEntryFactory implements CacheEntryFactory {
       args.add("--unified=1");
       args.add(key.getOldId().name());
     }
-    args.add(key.getNewId().name());
+    args.add(newId.name());
     args.add("--");
     args.add(key.getFileName());
     if (key.getSourceFileName() != null) {
@@ -71,7 +73,7 @@ public class DiffCacheEntryFactory implements CacheEntryFactory {
       proc.getInputStream().close();
 
       if (p.getFiles().isEmpty()) {
-        return new DiffCacheContent();
+        return DiffCacheContent.createEmpty(db, newId, key.getFileName());
 
       } else if (p.getFiles().size() != 1) {
         throw new IOException("unexpected file count: " + key);
diff --git a/src/main/java/com/google/gerrit/server/patch/DiffCacheKey.java b/src/main/java/com/google/gerrit/server/patch/DiffCacheKey.java
index 7c81e53da5..cc8eddcb30 100644
--- a/src/main/java/com/google/gerrit/server/patch/DiffCacheKey.java
+++ b/src/main/java/com/google/gerrit/server/patch/DiffCacheKey.java
@@ -27,7 +27,7 @@ import java.io.ObjectOutputStream;
 import java.io.Serializable;
 
 public final class DiffCacheKey implements Serializable {
-  private static final long serialVersionUID = 2L;
+  private static final long serialVersionUID = 3L;
 
   private transient Project.NameKey projectKey;
   private transient ObjectId oldId;
diff --git a/src/main/java/com/google/gerrit/server/patch/PatchScriptAction.java b/src/main/java/com/google/gerrit/server/patch/PatchScriptAction.java
index 56a85cfc96..bbddde7a2d 100644
--- a/src/main/java/com/google/gerrit/server/patch/PatchScriptAction.java
+++ b/src/main/java/com/google/gerrit/server/patch/PatchScriptAction.java
@@ -17,12 +17,16 @@ package com.google.gerrit.server.patch;
 import static com.google.gerrit.client.rpc.BaseServiceImplementation.canRead;
 
 import com.google.gerrit.client.data.PatchScript;
+import com.google.gerrit.client.patches.CommentDetail;
+import com.google.gerrit.client.reviewdb.Account;
 import com.google.gerrit.client.reviewdb.AccountGeneralPreferences;
 import com.google.gerrit.client.reviewdb.Change;
 import com.google.gerrit.client.reviewdb.Patch;
+import com.google.gerrit.client.reviewdb.PatchLineComment;
 import com.google.gerrit.client.reviewdb.PatchSet;
 import com.google.gerrit.client.reviewdb.Project;
 import com.google.gerrit.client.reviewdb.ReviewDb;
+import com.google.gerrit.client.rpc.Common;
 import com.google.gerrit.client.rpc.CorruptEntityException;
 import com.google.gerrit.client.rpc.NoDifferencesException;
 import com.google.gerrit.client.rpc.NoSuchEntityException;
@@ -120,12 +124,13 @@ class PatchScriptAction implements Action<PatchScript> {
     }
 
     final DiffCacheContent dcc = (DiffCacheContent) cacheElem.getObjectValue();
-    if (dcc.isNoDifference()) {
+    final CommentDetail comments = allComments(db);
+    if (dcc.isNoDifference() && comments.isEmpty()) {
       throw new Failure(new NoDifferencesException());
     }
 
     try {
-      return b.toPatchScript(dcc);
+      return b.toPatchScript(dcc, comments);
     } catch (CorruptEntityException e) {
       log.error("File content for " + key + " unavailable", e);
       throw new Failure(new NoSuchEntityException());
@@ -194,4 +199,20 @@ class PatchScriptAction implements Action<PatchScript> {
       throw new Failure(new NoSuchEntityException());
     }
   }
+
+  private CommentDetail allComments(final ReviewDb db) throws OrmException {
+    final CommentDetail r = new CommentDetail(psa, psb);
+    final String pn = patchKey.get();
+    for (PatchLineComment p : db.patchComments().published(changeId, pn)) {
+      r.include(p);
+    }
+
+    final Account.Id me = Common.getAccountId();
+    if (me != null) {
+      for (PatchLineComment p : db.patchComments().draft(changeId, pn, me)) {
+        r.include(p);
+      }
+    }
+    return r;
+  }
 }
diff --git a/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java b/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
index dd90e70644..27b57df8c5 100644
--- a/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
+++ b/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
@@ -16,7 +16,9 @@ package com.google.gerrit.server.patch;
 
 import com.google.gerrit.client.data.PatchScript;
 import com.google.gerrit.client.data.SparseFileContent;
+import com.google.gerrit.client.patches.CommentDetail;
 import com.google.gerrit.client.reviewdb.Patch;
+import com.google.gerrit.client.reviewdb.PatchLineComment;
 import com.google.gerrit.client.rpc.CorruptEntityException;
 
 import org.slf4j.Logger;
@@ -35,6 +37,7 @@ import org.spearce.jgit.util.RawParseUtils;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.List;
 
 class PatchScriptBuilder {
@@ -44,6 +47,13 @@ class PatchScriptBuilder {
   private static final Logger log =
       LoggerFactory.getLogger(PatchScriptBuilder.class);
 
+  private static final Comparator<Edit> EDIT_SORT = new Comparator<Edit>() {
+    @Override
+    public int compare(final Edit o1, final Edit o2) {
+      return o1.getBeginA() - o2.getBeginA();
+    }
+  };
+
   private final List<String> header;
   private final SparseFileContent dstA;
   private final SparseFileContent dstB;
@@ -75,8 +85,8 @@ class PatchScriptBuilder {
     context = c;
   }
 
-  PatchScript toPatchScript(final DiffCacheContent content)
-      throws CorruptEntityException {
+  PatchScript toPatchScript(final DiffCacheContent content,
+      final CommentDetail comments) throws CorruptEntityException {
     final FileHeader fh = content.getFileHeader();
     if (fh instanceof CombinedFileHeader) {
       // For a diff --cc format we don't support converting it into
@@ -94,6 +104,7 @@ class PatchScriptBuilder {
       srcB = open(content.getNewId());
     }
     edits = content.getEdits();
+    ensureCommentsVisible(comments);
 
     dstA.setMissingNewlineAtEnd(srcA.isMissingNewlineAtEnd());
     dstB.setMissingNewlineAtEnd(srcA.isMissingNewlineAtEnd());
@@ -101,7 +112,9 @@ class PatchScriptBuilder {
     dstA.setSize(srcA.size());
     dstB.setSize(srcB.size());
 
-    packHeader(fh);
+    if (fh != null) {
+      packHeader(fh);
+    }
     if (srcA == srcB && srcA.size() <= context && edits.isEmpty()) {
       // Odd special case; the files are identical (100% rename or copy)
       // and the user has asked for context that is larger than the file.
@@ -120,6 +133,97 @@ class PatchScriptBuilder {
     return new PatchScript(header, context, dstA, dstB, edits);
   }
 
+  private void ensureCommentsVisible(final CommentDetail comments)
+      throws CorruptEntityException {
+    if (comments.getCommentsA().isEmpty() && comments.getCommentsB().isEmpty()) {
+      // No comments, no additional dummy edits are required.
+      //
+      return;
+    }
+
+    // Construct empty Edit blocks around each location where a comment is.
+    // This will force the later packContent method to include the regions
+    // containing comments, potentially combining those regions together if
+    // they have overlapping contexts. UI renders will also be able to make
+    // correct hunks from this, but because the Edit is empty they will not
+    // style it specially.
+    //
+    final List<Edit> empty = new ArrayList<Edit>();
+    int lastLine;
+
+    lastLine = -1;
+    for (PatchLineComment plc : comments.getCommentsA()) {
+      final int a = plc.getLine();
+      if (lastLine != a) {
+        empty.add(new Edit(a - 1, mapA2B(a - 1)));
+        lastLine = a;
+      }
+    }
+
+    lastLine = -1;
+    for (PatchLineComment plc : comments.getCommentsB()) {
+      final int b = plc.getLine();
+      if (lastLine != b) {
+        empty.add(new Edit(mapB2A(b - 1), b - 1));
+        lastLine = b;
+      }
+    }
+
+    // Build the final list as a copy, as we cannot modify the cached
+    // edit list we started out with. Also sort the final list by the
+    // index in A, so packContent can combine them correctly later.
+    //
+    final List<Edit> n = new ArrayList<Edit>(edits.size() + empty.size());
+    n.addAll(edits);
+    n.addAll(empty);
+    Collections.sort(n, EDIT_SORT);
+    edits = n;
+  }
+
+  private int mapA2B(final int a) throws CorruptEntityException {
+    if (edits.isEmpty()) {
+      // Magic special case of an unmodified file.
+      //
+      return a;
+    }
+
+    if (a < edits.get(0).getBeginA()) {
+      // Special case of context at start of file.
+      //
+      return a;
+    }
+
+    for (Edit e : edits) {
+      if (e.getBeginA() <= a && a <= e.getEndA()) {
+        return e.getBeginB() + (a - e.getBeginA());
+      }
+    }
+    log.error("In " + patchKey + " cannot remap A " + a + " to B");
+    throw new CorruptEntityException(patchKey);
+  }
+
+  private int mapB2A(final int b) throws CorruptEntityException {
+    if (edits.isEmpty()) {
+      // Magic special case of an unmodified file.
+      //
+      return b;
+    }
+
+    if (b < edits.get(0).getBeginB()) {
+      // Special case of context at start of file.
+      //
+      return b;
+    }
+
+    for (Edit e : edits) {
+      if (e.getBeginB() <= b && b <= e.getEndB()) {
+        return e.getBeginA() + (b - e.getBeginB());
+      }
+    }
+    log.error("In " + patchKey + " cannot remap B " + b + " to A");
+    throw new CorruptEntityException(patchKey);
+  }
+
   private static boolean eq(final ObjectId a, final ObjectId b) {
     if (a == null && b == null) {
       return true;