Display post-image lines in side-by-side view when ignoring spaces

If we are ignoring whitespace, but a block of lines has had its
indentation changed, we should show the post-image accurately by
using the lines from the post-image buffer rather than using the
lines from the pre-image buffer.

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

5 files changed, 82 insertions, 28 deletions

diff --git a/src/main/java/com/google/gerrit/client/data/PatchScript.java b/src/main/java/com/google/gerrit/client/data/PatchScript.java
index 656668cea3..e3d118346c 100644
--- a/src/main/java/com/google/gerrit/client/data/PatchScript.java
+++ b/src/main/java/com/google/gerrit/client/data/PatchScript.java
@@ -16,6 +16,8 @@ package com.google.gerrit.client.data;
 
 
 
+import com.google.gerrit.client.data.PatchScriptSettings.Whitespace;
+
 import org.spearce.jgit.diff.Edit;
 
 import java.util.Iterator;
@@ -48,6 +50,10 @@ public class PatchScript {
     return settings.getContext();
   }
 
+  public boolean isIgnoreWhitespace() {
+    return settings.getWhitespace() != Whitespace.IGNORE_NONE;
+  }
+
   public SparseFileContent getA() {
     return a;
   }
diff --git a/src/main/java/com/google/gerrit/client/data/SparseFileContent.java b/src/main/java/com/google/gerrit/client/data/SparseFileContent.java
index 7318bb787c..947f8572bd 100644
--- a/src/main/java/com/google/gerrit/client/data/SparseFileContent.java
+++ b/src/main/java/com/google/gerrit/client/data/SparseFileContent.java
@@ -44,6 +44,18 @@ public class SparseFileContent {
   }
 
   public String get(final int idx) {
+    final String line = getLine(idx);
+    if (line == null) {
+      throw new ArrayIndexOutOfBoundsException(idx);
+    }
+    return line;
+  }
+
+  public boolean contains(final int idx) {
+    return getLine(idx) != null;
+  }
+
+  private String getLine(final int idx) {
     for (int i = lastGetRange; i < ranges.size(); i++) {
       final Range r = ranges.get(i);
       if (r.contains(idx)) {
@@ -53,9 +65,9 @@ public class SparseFileContent {
     }
     if (lastGetRange != 0) {
       lastGetRange = 0;
-      return get(idx);
+      return getLine(idx);
     }
-    throw new ArrayIndexOutOfBoundsException(idx);
+    return null;
   }
 
   public void addLine(final int i, final String content) {
diff --git a/src/main/java/com/google/gerrit/client/patches/SideBySideTable.java b/src/main/java/com/google/gerrit/client/patches/SideBySideTable.java
index 9a77424ef7..e41b9a8da6 100644
--- a/src/main/java/com/google/gerrit/client/patches/SideBySideTable.java
+++ b/src/main/java/com/google/gerrit/client/patches/SideBySideTable.java
@@ -68,6 +68,7 @@ public class SideBySideTable extends AbstractPatchContentTable {
     lines.add(null);
 
     int lastB = 0;
+    final boolean ignoreWS = script.isIgnoreWhitespace();
     for (final PatchScript.Hunk hunk : script.getHunks()) {
       if (!hunk.isStartOfFile()) {
         appendSkipLine(nc, hunk.getCurB() - lastB);
@@ -78,7 +79,11 @@ public class SideBySideTable extends AbstractPatchContentTable {
         if (hunk.isContextLine()) {
           openLine(nc);
           appendLineText(nc, hunk.getCurA(), CONTEXT, a, hunk.getCurA());
-          appendLineText(nc, hunk.getCurB(), CONTEXT, a, hunk.getCurA());
+          if (ignoreWS && b.contains(hunk.getCurB())) {
+            appendLineText(nc, hunk.getCurB(), CONTEXT, b, hunk.getCurB());
+          } else {
+            appendLineText(nc, hunk.getCurB(), CONTEXT, a, hunk.getCurA());
+          }
           closeLine(nc);
           hunk.incBoth();
           lines.add(new PatchLine(CONTEXT, hunk.getCurA(), hunk.getCurB()));
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 927f511f95..b7d570989e 100644
--- a/src/main/java/com/google/gerrit/server/patch/PatchScriptAction.java
+++ b/src/main/java/com/google/gerrit/server/patch/PatchScriptAction.java
@@ -18,6 +18,7 @@ import static com.google.gerrit.client.rpc.BaseServiceImplementation.canRead;
 
 import com.google.gerrit.client.data.PatchScript;
 import com.google.gerrit.client.data.PatchScriptSettings;
+import com.google.gerrit.client.data.PatchScriptSettings.Whitespace;
 import com.google.gerrit.client.patches.CommentDetail;
 import com.google.gerrit.client.reviewdb.Account;
 import com.google.gerrit.client.reviewdb.AccountGeneralPreferences;
@@ -107,10 +108,41 @@ class PatchScriptAction implements Action<PatchScript> {
     final PatchScriptBuilder b = newBuilder();
     final ObjectId bId = toObjectId(db, psb);
     final ObjectId aId = psa == null ? ancestor(bId) : toObjectId(db, psa);
-    final DiffCacheKey key;
-    final Element cacheElem;
 
-    key = new DiffCacheKey(projectKey, aId, bId, patch, settings);
+    final DiffCacheKey key = keyFor(bId, aId);
+    final DiffCacheContent contentWS = get(key);
+    final CommentDetail comments = allComments(db);
+    if (contentWS.isNoDifference() && comments.isEmpty()) {
+      throw new Failure(new NoDifferencesException());
+    }
+
+    final DiffCacheContent contentActual;
+    if (settings.getWhitespace() != Whitespace.IGNORE_NONE) {
+      // If we are ignoring whitespace in some form, we still need to know
+      // where the post-image differs so we can ensure the post-image lines
+      // are still packed for the client to display.
+      //
+      final PatchScriptSettings s = new PatchScriptSettings(settings);
+      s.setWhitespace(Whitespace.IGNORE_NONE);
+      contentActual = get(new DiffCacheKey(projectKey, aId, bId, patch, s));
+    } else {
+      contentActual = contentWS;
+    }
+
+    try {
+      return b.toPatchScript(contentWS, comments, contentActual);
+    } catch (CorruptEntityException e) {
+      log.error("File content for " + key + " unavailable", e);
+      throw new Failure(new NoSuchEntityException());
+    }
+  }
+
+  private DiffCacheKey keyFor(final ObjectId bId, final ObjectId aId) {
+    return new DiffCacheKey(projectKey, aId, bId, patch, settings);
+  }
+
+  private DiffCacheContent get(final DiffCacheKey key) throws Failure {
+    final Element cacheElem;
     try {
       cacheElem = server.getDiffCache().get(key);
     } catch (IllegalStateException e) {
@@ -120,24 +152,11 @@ class PatchScriptAction implements Action<PatchScript> {
       log.error("Cache get failed for " + key, e);
       throw new Failure(new NoSuchEntityException());
     }
-
     if (cacheElem == null || cacheElem.getObjectValue() == null) {
       log.error("Cache get failed for " + key);
       throw new Failure(new NoSuchEntityException());
     }
-
-    final DiffCacheContent dcc = (DiffCacheContent) cacheElem.getObjectValue();
-    final CommentDetail comments = allComments(db);
-    if (dcc.isNoDifference() && comments.isEmpty()) {
-      throw new Failure(new NoDifferencesException());
-    }
-
-    try {
-      return b.toPatchScript(dcc, comments);
-    } catch (CorruptEntityException e) {
-      log.error("File content for " + key + " unavailable", e);
-      throw new Failure(new NoSuchEntityException());
-    }
+    return (DiffCacheContent) cacheElem.getObjectValue();
   }
 
   private PatchScriptBuilder newBuilder() throws Failure {
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 38cc369caa..1f1b360258 100644
--- a/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
+++ b/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
@@ -90,9 +90,10 @@ class PatchScriptBuilder {
     return settings.getContext();
   }
 
-  PatchScript toPatchScript(final DiffCacheContent content,
-      final CommentDetail comments) throws CorruptEntityException {
-    final FileHeader fh = content.getFileHeader();
+  PatchScript toPatchScript(final DiffCacheContent contentWS,
+      final CommentDetail comments, final DiffCacheContent contentAct)
+      throws CorruptEntityException {
+    final FileHeader fh = contentAct.getFileHeader();
     if (fh instanceof CombinedFileHeader) {
       // For a diff --cc format we don't support converting it into
       // a patch script. Instead treat everything as a file header.
@@ -102,13 +103,13 @@ class PatchScriptBuilder {
       return new PatchScript(header, settings, dstA, dstB, edits);
     }
 
-    srcA = open(content.getOldId());
-    if (eq(content.getOldId(), content.getNewId())) {
+    srcA = open(contentAct.getOldId());
+    if (eq(contentAct.getOldId(), contentAct.getNewId())) {
       srcB = srcA;
     } else {
-      srcB = open(content.getNewId());
+      srcB = open(contentAct.getNewId());
     }
-    edits = content.getEdits();
+    edits = contentAct.getEdits();
     ensureCommentsVisible(comments);
 
     dstA.setMissingNewlineAtEnd(srcA.isMissingNewlineAtEnd());
@@ -120,7 +121,8 @@ class PatchScriptBuilder {
     if (fh != null) {
       packHeader(fh);
     }
-    if (srcA == srcB && srcA.size() <= context() && edits.isEmpty()) {
+    if (srcA == srcB && srcA.size() <= context()
+        && contentAct.getEdits().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.
       // Send them the entire file, with an empty edit after the last line.
@@ -135,6 +137,16 @@ class PatchScriptBuilder {
       }
       packContent();
     }
+
+    if (contentWS != contentAct) {
+      // The edit list we used to pack the file contents doesn't honor the
+      // whitespace settings requested. Instead we must rebuild our edit
+      // list around the whitespace edit list.
+      //
+      edits = contentWS.getEdits();
+      ensureCommentsVisible(comments);
+    }
+
     return new PatchScript(header, settings, dstA, dstB, edits);
   }