Support diff between patch sets

Pass through arguments for diff between patch sets

Modify internal APIs to pass through the old, new and preferences
needed to compute the differences between two patch sets for the
UI.

Bug: issue 194
Change-Id: I98827bf88227e912860769f22cd90f5c35b784b0

6 files changed, 92 insertions, 22 deletions

diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/ChangeDetailService.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/ChangeDetailService.java
index 4d362fe0b8..621f52df3a 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/ChangeDetailService.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/ChangeDetailService.java
@@ -15,6 +15,7 @@
 package com.google.gerrit.common.data;
 
 import com.google.gerrit.common.auth.SignInRequired;
+import com.google.gerrit.reviewdb.AccountDiffPreference;
 import com.google.gerrit.reviewdb.Change;
 import com.google.gerrit.reviewdb.PatchSet;
 import com.google.gwt.user.client.rpc.AsyncCallback;
@@ -28,7 +29,8 @@ public interface ChangeDetailService extends RemoteJsonService {
 
   void includedInDetail(Change.Id id, AsyncCallback<IncludedInDetail> callback);
 
-  void patchSetDetail(PatchSet.Id key, AsyncCallback<PatchSetDetail> callback);
+  void patchSetDetail(PatchSet.Id keyA, PatchSet.Id keyB,
+      AccountDiffPreference diffPrefs, AsyncCallback<PatchSetDetail> callback);
 
   @SignInRequired
   void patchSetPublishDetail(PatchSet.Id key,
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java
index d7fd13f723..834a9f492d 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java
@@ -503,7 +503,7 @@ class PatchSetComplexDisclosurePanel extends ComplexDisclosurePanel implements O
   @Override
   public void onOpen(final OpenEvent<DisclosurePanel> event) {
     if (infoTable == null) {
-      Util.DETAIL_SVC.patchSetDetail(patchSet.getId(),
+      Util.DETAIL_SVC.patchSetDetail(patchSet.getId(), null, null,
           new GerritCallback<PatchSetDetail>() {
             public void onSuccess(final PatchSetDetail result) {
               ensureLoaded(result);
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java
index 5d0b0d1267..035793f657 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java
@@ -324,7 +324,7 @@ public abstract class PatchScreen extends Screen implements
   protected void onLoad() {
     super.onLoad();
     if (patchSetDetail == null) {
-      Util.DETAIL_SVC.patchSetDetail(idSideB,
+      Util.DETAIL_SVC.patchSetDetail(idSideB, null, null,
           new GerritCallback<PatchSetDetail>() {
             @Override
             public void onSuccess(PatchSetDetail result) {
@@ -407,7 +407,7 @@ public abstract class PatchScreen extends Screen implements
       commitMessageBlock.display(patchSetDetail.getInfo().getMessage());
     } else {
       commitMessageBlock.setVisible(false);
-      Util.DETAIL_SVC.patchSetDetail(idSideB,
+      Util.DETAIL_SVC.patchSetDetail(idSideB, null, null,
           new GerritCallback<PatchSetDetail>() {
             @Override
             public void onSuccess(PatchSetDetail result) {
@@ -501,7 +501,7 @@ public abstract class PatchScreen extends Screen implements
         final PatchSet.Id psid = patchKey.getParentKey();
         fileList = new PatchTable(prefs);
         fileList.setSavePointerId("PatchTable " + psid);
-        Util.DETAIL_SVC.patchSetDetail(psid,
+        Util.DETAIL_SVC.patchSetDetail(psid, null, null,
             new GerritCallback<PatchSetDetail>() {
               public void onSuccess(final PatchSetDetail result) {
                 fileList.display(result);
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java
index 14cb1e15ee..bdd0864134 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java
@@ -190,7 +190,7 @@ public class ChangeDetailFactory extends Handler<ChangeDetail> {
       NoSuchEntityException, PatchSetInfoNotAvailableException,
       NoSuchChangeException {
     final PatchSet.Id psId = detail.getChange().currentPatchSetId();
-    final PatchSetDetailFactory loader = patchSetDetail.create(psId);
+    final PatchSetDetailFactory loader = patchSetDetail.create(psId, null, null);
     loader.patchSet = detail.getCurrentPatchSet();
     loader.control = control;
     detail.setCurrentPatchSetDetail(loader.call());
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailServiceImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailServiceImpl.java
index ebb4502933..f131123bab 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailServiceImpl.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailServiceImpl.java
@@ -19,6 +19,7 @@ import com.google.gerrit.common.data.ChangeDetailService;
 import com.google.gerrit.common.data.IncludedInDetail;
 import com.google.gerrit.common.data.PatchSetDetail;
 import com.google.gerrit.common.data.PatchSetPublishDetail;
+import com.google.gerrit.reviewdb.AccountDiffPreference;
 import com.google.gerrit.reviewdb.Change;
 import com.google.gerrit.reviewdb.PatchSet;
 import com.google.gwt.user.client.rpc.AsyncCallback;
@@ -51,9 +52,10 @@ class ChangeDetailServiceImpl implements ChangeDetailService {
     includedInDetail.create(id).to(callback);
   }
 
-  public void patchSetDetail(final PatchSet.Id id,
+  public void patchSetDetail(final PatchSet.Id idA, final PatchSet.Id idB,
+      final AccountDiffPreference diffPrefs,
       final AsyncCallback<PatchSetDetail> callback) {
-    patchSetDetail.create(id).to(callback);
+    patchSetDetail.create(idA, idB, diffPrefs).to(callback);
   }
 
   public void patchSetPublishDetail(final PatchSet.Id id,
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java
index 0fcdf40d33..d7c5fc747f 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java
@@ -18,15 +18,20 @@ import com.google.gerrit.common.data.PatchSetDetail;
 import com.google.gerrit.common.errors.NoSuchEntityException;
 import com.google.gerrit.httpd.rpc.Handler;
 import com.google.gerrit.reviewdb.Account;
+import com.google.gerrit.reviewdb.AccountDiffPreference;
 import com.google.gerrit.reviewdb.AccountPatchReview;
 import com.google.gerrit.reviewdb.Patch;
 import com.google.gerrit.reviewdb.PatchLineComment;
 import com.google.gerrit.reviewdb.PatchSet;
+import com.google.gerrit.reviewdb.Project;
 import com.google.gerrit.reviewdb.ReviewDb;
+import com.google.gerrit.reviewdb.AccountDiffPreference.Whitespace;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.patch.PatchList;
 import com.google.gerrit.server.patch.PatchListCache;
+import com.google.gerrit.server.patch.PatchListKey;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.project.ChangeControl;
@@ -35,54 +40,92 @@ import com.google.gwtorm.client.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
 
+import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
+import javax.annotation.Nullable;
+
 /** Creates a {@link PatchSetDetail} from a {@link PatchSet}. */
 class PatchSetDetailFactory extends Handler<PatchSetDetail> {
+
+  private static final Logger log =
+    LoggerFactory.getLogger(PatchSetDetailFactory.class);
+
   interface Factory {
-    PatchSetDetailFactory create(PatchSet.Id id);
+    PatchSetDetailFactory create(@Assisted("psIdNew") PatchSet.Id psIdA,
+        @Assisted("psIdOld") PatchSet.Id psIdB, AccountDiffPreference diffPrefs);
   }
 
+  private final GitRepositoryManager repoManager;
+
   private final PatchSetInfoFactory infoFactory;
   private final ReviewDb db;
   private final PatchListCache patchListCache;
   private final ChangeControl.Factory changeControlFactory;
 
-  private final PatchSet.Id psId;
+  private Project.NameKey projectKey;
+  private final PatchSet.Id psIdNew;
+  private final PatchSet.Id psIdOld;
+  private final AccountDiffPreference diffPrefs;
+  private ObjectId oldId;
+  private ObjectId newId;
 
   private PatchSetDetail detail;
   ChangeControl control;
   PatchSet patchSet;
 
   @Inject
-  PatchSetDetailFactory(final PatchSetInfoFactory psif, final ReviewDb db,
+  PatchSetDetailFactory(final GitRepositoryManager grm,
+      final PatchSetInfoFactory psif, final ReviewDb db,
       final PatchListCache patchListCache,
       final ChangeControl.Factory changeControlFactory,
-      @Assisted final PatchSet.Id id) {
+      @Assisted("psIdNew") final PatchSet.Id psIdNew,
+      @Assisted("psIdOld") @Nullable final PatchSet.Id psIdOld,
+      @Assisted @Nullable final AccountDiffPreference diffPrefs) {
+    this.repoManager = grm;
     this.infoFactory = psif;
     this.db = db;
     this.patchListCache = patchListCache;
     this.changeControlFactory = changeControlFactory;
 
-    this.psId = id;
+    this.psIdNew = psIdNew;
+    this.psIdOld = psIdOld;
+    this.diffPrefs = diffPrefs;
   }
 
+  @SuppressWarnings("deprecation")
   @Override
   public PatchSetDetail call() throws OrmException, NoSuchEntityException,
       PatchSetInfoNotAvailableException, NoSuchChangeException {
     if (control == null || patchSet == null) {
-      control = changeControlFactory.validateFor(psId.getParentKey());
-      patchSet = db.patchSets().get(psId);
+      control = changeControlFactory.validateFor(psIdNew.getParentKey());
+      patchSet = db.patchSets().get(psIdNew);
       if (patchSet == null) {
         throw new NoSuchEntityException();
       }
     }
 
-    final PatchList list = patchListCache.get(control.getChange(), patchSet);
-    if (list == null) {
-      throw new NoSuchEntityException();
+    final PatchList list;
+
+    if (psIdOld != null) {
+      newId = toObjectId(psIdNew);
+      oldId = psIdOld != null ? toObjectId(psIdOld) : null;
+
+      projectKey = control.getProject().getNameKey();
+
+      list = listFor(keyFor(diffPrefs.getIgnoreWhitespace()));
+    } else { // OK, means use base to compare
+      list = patchListCache.get(control.getChange(), patchSet);
+      if (list == null) {
+        throw new NoSuchEntityException();
+      }
     }
 
     final List<Patch> patches = list.toPatchList(patchSet.getId());
@@ -91,7 +134,7 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
       byKey.put(p.getKey(), p);
     }
 
-    for (final PatchLineComment c : db.patchComments().published(psId)) {
+    for (final PatchLineComment c : db.patchComments().published(psIdNew)) {
       final Patch p = byKey.get(c.getKey().getParentKey());
       if (p != null) {
         p.setCommentCount(p.getCommentCount() + 1);
@@ -101,7 +144,7 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
     detail = new PatchSetDetail();
     detail.setPatchSet(patchSet);
 
-    detail.setInfo(infoFactory.get(psId));
+    detail.setInfo(infoFactory.get(psIdNew));
     detail.setPatches(patches);
 
     final CurrentUser user = control.getCurrentUser();
@@ -111,14 +154,14 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
       // quickly locate where they have pending drafts, and review them.
       //
       final Account.Id me = ((IdentifiedUser) user).getAccountId();
-      for (final PatchLineComment c : db.patchComments().draft(psId, me)) {
+      for (final PatchLineComment c : db.patchComments().draft(psIdNew, me)) {
         final Patch p = byKey.get(c.getKey().getParentKey());
         if (p != null) {
           p.setDraftCount(p.getDraftCount() + 1);
         }
       }
 
-      for (AccountPatchReview r : db.accountPatchReviews().byReviewer(me, psId)) {
+      for (AccountPatchReview r : db.accountPatchReviews().byReviewer(me, psIdNew)) {
         final Patch p = byKey.get(r.getKey().getPatchKey());
         if (p != null) {
           p.setReviewedByCurrentUser(true);
@@ -128,4 +171,27 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
 
     return detail;
   }
+
+  private ObjectId toObjectId(final PatchSet.Id psId) throws OrmException,
+      NoSuchEntityException {
+    final PatchSet ps = db.patchSets().get(psId);
+    if (ps == null) {
+      throw new NoSuchEntityException();
+    }
+
+    try {
+      return ObjectId.fromString(ps.getRevision().get());
+    } catch (IllegalArgumentException e) {
+      log.error("Patch set " + psId + " has invalid revision");
+      throw new NoSuchEntityException();
+    }
+  }
+
+  private PatchListKey keyFor(final Whitespace whitespace) {
+    return new PatchListKey(projectKey, oldId, newId, whitespace);
+  }
+
+  private PatchList listFor(final PatchListKey key) {
+    return patchListCache.get(key);
+  }
 }