Add 'Parent $x' options to diff for merge commits

Add 'Parent $x' options to the diff base drop down for merge commits.
This way users are able to see changes brought by merge commit in
addition to the merge conflicts resolution.

Two main challenges of this change are:
* server: enable commenting on a Parent-N of a merge commit
* client: pass around the diff-base info which for merge commits can be:
  Auto-Merge, Parent-1, Parent-2, ... , Parent-N

Currently the patch_comments.side field uses two values:
1 = REVISION
0 = PARENT

For non-merge commits, side == 0 means comment of the Base (the parent)
of this patch-set.  For a merge commit, side == 0 means comment on the
Auto-Merge of this (merge) patch-set.

This change uses side == -N to store comment on the Parent-N of this (merge)
patch-set. For Parent-1 the side is -1, for Parent-2 the side is -2, etc..
This avoids need for a schema migration.

In NoteDb the parent number is stored in a new field: "Parent-number: 1".

CommentInfo was extended with the new "parent" field which is 1-based
parent number.  Some REST API endpoints expose a new --parent option to
enable referencing a specific parent of a merge commit.

On the client side we typically pass around two PatchSet.Id's: base and
revision to represent the state of the UI. The base had two meanings:
* when it was non-null it was representing another patch-set
* when it was null it represented the parent for a non-merge commit and
  the auto-merge for a merge commit.

For a merge commit we need to also pass around Parent-N as (diff) base for a
merge commit. To keep the number of changes minimal this change proposes
(re)using the base patch-set for that where its patch-set number is negative.
Therefore, when base is not null but its patch-set number is negative (-N) it
represents Parent-N of the patch-set represented by the revision patch-set.
This is also expressed in the client URL token which uses negative numbers
for parent(s) of a merge commit. For example: -1..2 represents comparison
of the second patch-set against its first parent. -2..2 would represent
comparison of the patch-set 2 against its second parent.

I experimented with introducing a DiffBase class which would be a combination
of a base patch-set plus a parent number:

  class DiffBase {
    PatchSet.Id base;
    int parent;
  }

which would avoid the ugliness of using a base with negative patch-set number.
However, this produced neither a smaller nor a more elegant change. The number
of changed files actually increased and, still, all places where base is used
had to handle the case where base is null and parent is greater than zero.

Bug: Issue 106
Change-Id: If0d7b13fad9051ec2943f6d51c99e84f7d2af708
Also-by: Dariusz Luksza <dariusz@luksza.org>
Also-by: Saša Živkov <zivkov@gmail.com>

gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java

@@ -28,7 +28,7 @@ public interface PatchListCache {
   PatchList get(Change change, PatchSet patchSet)
       throws PatchListNotAvailableException;
 
-  ObjectId getOldId(Change change, PatchSet patchSet)
+  ObjectId getOldId(Change change, PatchSet patchSet, Integer parentNum)
       throws PatchListNotAvailableException;
 
   IntraLineDiff getIntraLineDiff(IntraLineDiffKey key,

gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java

@@ -103,6 +103,17 @@ public class PatchListCacheImpl implements PatchListCache {
   @Override
   public PatchList get(Change change, PatchSet patchSet)
       throws PatchListNotAvailableException {
+    return get(change, patchSet, null);
+  }
+
+  @Override
+  public ObjectId getOldId(Change change, PatchSet patchSet, Integer parentNum)
+      throws PatchListNotAvailableException {
+    return get(change, patchSet, parentNum).getOldId();
+  }
+
+  private PatchList get(Change change, PatchSet patchSet, Integer parentNum)
+      throws PatchListNotAvailableException {
     Project.NameKey project = change.getProject();
     if (patchSet.getRevision() == null) {
       throw new PatchListNotAvailableException(
@@ -110,13 +121,10 @@ public class PatchListCacheImpl implements PatchListCache {
     }
     ObjectId b = ObjectId.fromString(patchSet.getRevision().get());
     Whitespace ws = Whitespace.IGNORE_NONE;
-    return get(new PatchListKey(null, b, ws), project);
-  }
-
-  @Override
-  public ObjectId getOldId(Change change, PatchSet patchSet)
-      throws PatchListNotAvailableException {
-    return get(change, patchSet).getOldId();
+    if (parentNum != null) {
+      return get(PatchListKey.againstParentNum(parentNum, b, ws), project);
+    }
+    return get(PatchListKey.againstDefaultBase(b, ws), project);
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java

@@ -32,9 +32,10 @@ import java.io.IOException;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
 import java.io.Serializable;
+import java.util.Objects;
 
 public class PatchListKey implements Serializable {
-  static final long serialVersionUID = 20L;
+  static final long serialVersionUID = 21L;
 
   public static final BiMap<Whitespace, Character> WHITESPACE_TYPES = ImmutableBiMap.of(
       Whitespace.IGNORE_NONE, 'N',
@@ -46,7 +47,36 @@ public class PatchListKey implements Serializable {
     checkState(WHITESPACE_TYPES.size() == Whitespace.values().length);
   }
 
+  public static PatchListKey againstDefaultBase(AnyObjectId newId,
+      Whitespace ws) {
+    return new PatchListKey(null, newId, ws);
+  }
+
+  public static PatchListKey againstParentNum(int parentNum, AnyObjectId newId,
+      Whitespace ws) {
+    return new PatchListKey(parentNum, newId, ws);
+  }
+
+  /**
+   * Old patch-set ID
+   * <p>
+   * When null, it represents the Base of the newId for a non-merge commit.
+   * <p>
+   * When newId is a merge commit, null value of the oldId represents either
+   * the auto-merge commit of the newId or a parent commit of the newId.
+   * These two cases are distinguished by the parentNum.
+   */
   private transient ObjectId oldId;
+
+  /**
+   * 1-based parent number when newId is a merge commit
+   * <p>
+   * For the auto-merge case this field is null.
+   * <p>
+   * Used only when oldId is null and newId is a merge commit
+   */
+  private transient Integer parentNum;
+
   private transient ObjectId newId;
   private transient Whitespace whitespace;
 
@@ -56,12 +86,24 @@ public class PatchListKey implements Serializable {
     whitespace = ws;
   }
 
+  private PatchListKey(int parentNum, AnyObjectId b, Whitespace ws) {
+    this.parentNum = Integer.valueOf(parentNum);
+    newId = b.copy();
+    whitespace = ws;
+  }
+
   /** Old side commit, or null to assume ancestor or combined merge. */
   @Nullable
   public ObjectId getOldId() {
     return oldId;
   }
 
+  /** Parent number (old side) of the new side (merge) commit */
+  @Nullable
+  public Integer getParentNum() {
+    return parentNum;
+  }
+
   /** New side commit name. */
   public ObjectId getNewId() {
     return newId;
@@ -73,24 +115,16 @@ public class PatchListKey implements Serializable {
 
   @Override
   public int hashCode() {
-    int h = 0;
-
-    if (oldId != null) {
-      h = h * 31 + oldId.hashCode();
-    }
-
-    h = h * 31 + newId.hashCode();
-    h = h * 31 + whitespace.name().hashCode();
-
-    return h;
+    return Objects.hash(oldId, parentNum, newId, whitespace);
   }
 
   @Override
   public boolean equals(final Object o) {
     if (o instanceof PatchListKey) {
-      final PatchListKey k = (PatchListKey) o;
-      return eq(oldId, k.oldId) //
-          && eq(newId, k.newId) //
+      PatchListKey k = (PatchListKey) o;
+      return Objects.equals(oldId, k.oldId)
+          && Objects.equals(parentNum, k.parentNum)
+          && Objects.equals(newId, k.newId)
           && whitespace == k.whitespace;
     }
     return false;
@@ -109,15 +143,9 @@ public class PatchListKey implements Serializable {
     return n.toString();
   }
 
-  private static boolean eq(final ObjectId a, final ObjectId b) {
-    if (a == null && b == null) {
-      return true;
-    }
-    return a != null && b != null && AnyObjectId.equals(a, b);
-  }
-
   private void writeObject(final ObjectOutputStream out) throws IOException {
     writeCanBeNull(out, oldId);
+    out.writeInt(parentNum == null ? 0 : parentNum);
     writeNotNull(out, newId);
     Character c = WHITESPACE_TYPES.get(whitespace);
     if (c == null) {
@@ -128,6 +156,8 @@ public class PatchListKey implements Serializable {
 
   private void readObject(final ObjectInputStream in) throws IOException {
     oldId = readCanBeNull(in);
+    int n = in.readInt();
+    parentNum = n == 0 ? null : Integer.valueOf(n);
     newId = readNotNull(in);
     char t = in.readChar();
     whitespace = WHITESPACE_TYPES.inverse().get(t);

gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java

@@ -163,11 +163,11 @@ public class PatchListLoader implements Callable<PatchList> {
       List<DiffEntry> diffEntries = df.scan(aTree, bTree);
 
       Set<String> paths = null;
-      if (key.getOldId() != null) {
-        PatchListKey newKey =
-            new PatchListKey(null, key.getNewId(), key.getWhitespace());
-        PatchListKey oldKey =
-            new PatchListKey(null, key.getOldId(), key.getWhitespace());
+      if (key.getOldId() != null && b.getParentCount() == 1) {
+        PatchListKey newKey = PatchListKey.againstDefaultBase(
+            key.getNewId(), key.getWhitespace());
+        PatchListKey oldKey = PatchListKey.againstDefaultBase(
+            key.getOldId(), key.getWhitespace());
         paths = FluentIterable
             .from(patchListCache.get(newKey, project).getPatches())
             .append(patchListCache.get(oldKey, project).getPatches())
@@ -331,6 +331,11 @@ public class PatchListLoader implements Callable<PatchList> {
         return r;
       }
       case 2:
+        if (key.getParentNum() != null) {
+          RevCommit r = b.getParent(key.getParentNum() - 1);
+          rw.parseBody(r);
+          return r;
+        }
         return autoMerger.merge(repo, rw, ins, b, mergeStrategy);
       default:
         // TODO(sop) handle an octopus merge.

gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.patch;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.gerrit.server.util.GitUtil.getParent;
 
 import com.google.common.base.Optional;
 import com.google.gerrit.common.Nullable;
@@ -44,9 +45,9 @@ import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.InvalidChangeOperationException;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
-import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.assistedinject.Assisted;
+import com.google.inject.assistedinject.AssistedInject;
 
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.lib.ObjectId;
@@ -70,6 +71,13 @@ public class PatchScriptFactory implements Callable<PatchScript> {
         @Assisted("patchSetA") PatchSet.Id patchSetA,
         @Assisted("patchSetB") PatchSet.Id patchSetB,
         DiffPreferencesInfo diffPrefs);
+
+    PatchScriptFactory create(
+        ChangeControl control,
+        String fileName,
+        int parentNum,
+        PatchSet.Id patchSetB,
+        DiffPreferencesInfo diffPrefs);
   }
 
   private static final Logger log =
@@ -86,6 +94,8 @@ public class PatchScriptFactory implements Callable<PatchScript> {
   private final String fileName;
   @Nullable
   private final PatchSet.Id psa;
+  @Nullable
+  private final int parentNum;
   private final PatchSet.Id psb;
   private final DiffPreferencesInfo diffPrefs;
   private final ChangeEditUtil editReader;
@@ -103,7 +113,7 @@ public class PatchScriptFactory implements Callable<PatchScript> {
   private List<Patch> history;
   private CommentDetail comments;
 
-  @Inject
+  @AssistedInject
   PatchScriptFactory(GitRepositoryManager grm,
       PatchSetUtil psUtil,
       Provider<PatchScriptBuilder> builderFactory,
@@ -129,14 +139,45 @@ public class PatchScriptFactory implements Callable<PatchScript> {
 
     this.fileName = fileName;
     this.psa = patchSetA;
+    this.parentNum = -1;
     this.psb = patchSetB;
     this.diffPrefs = diffPrefs;
 
     changeId = patchSetB.getParentKey();
-    checkArgument(
-        patchSetA == null || patchSetA.getParentKey().equals(changeId),
-        "cannot compare PatchSets from different changes: %s and %s",
-        patchSetA, patchSetB);
+  }
+
+  @AssistedInject
+  PatchScriptFactory(GitRepositoryManager grm,
+      PatchSetUtil psUtil,
+      Provider<PatchScriptBuilder> builderFactory,
+      PatchListCache patchListCache,
+      ReviewDb db,
+      AccountInfoCacheFactory.Factory aicFactory,
+      PatchLineCommentsUtil plcUtil,
+      ChangeEditUtil editReader,
+      @Assisted ChangeControl control,
+      @Assisted String fileName,
+      @Assisted int parentNum,
+      @Assisted PatchSet.Id patchSetB,
+      @Assisted DiffPreferencesInfo diffPrefs) {
+    this.repoManager = grm;
+    this.psUtil = psUtil;
+    this.builderFactory = builderFactory;
+    this.patchListCache = patchListCache;
+    this.db = db;
+    this.control = control;
+    this.aicFactory = aicFactory;
+    this.plcUtil = plcUtil;
+    this.editReader = editReader;
+
+    this.fileName = fileName;
+    this.psa = null;
+    this.parentNum = parentNum;
+    this.psb = patchSetB;
+    this.diffPrefs = diffPrefs;
+
+    changeId = patchSetB.getParentKey();
+    checkArgument(parentNum >= 0, "parentNum must be >= 0");
   }
 
   public void setLoadHistory(boolean load) {
@@ -151,7 +192,9 @@ public class PatchScriptFactory implements Callable<PatchScript> {
   public PatchScript call() throws OrmException, NoSuchChangeException,
       LargeObjectException, AuthException,
       InvalidChangeOperationException, IOException {
-    validatePatchSetId(psa);
+    if (parentNum < 0) {
+      validatePatchSetId(psa);
+    }
     validatePatchSetId(psb);
 
     change = control.getChange();
@@ -163,15 +206,19 @@ public class PatchScriptFactory implements Callable<PatchScript> {
         ? new PatchSet(psb)
         : psUtil.get(db, control.getNotes(), psb);
 
-    aId = psEntityA != null ? toObjectId(psEntityA) : null;
-    bId = toObjectId(psEntityB);
-
     if ((psEntityA != null && !control.isPatchVisible(psEntityA, db)) ||
         (psEntityB != null && !control.isPatchVisible(psEntityB, db))) {
       throw new NoSuchChangeException(changeId);
     }
 
     try (Repository git = repoManager.openRepository(project)) {
+      bId = toObjectId(psEntityB);
+      if (parentNum < 0) {
+        aId = psEntityA != null ? toObjectId(psEntityA) : null;
+      } else {
+        aId = getParent(git, bId, parentNum);
+      }
+
       try {
         final PatchList list = listFor(keyFor(diffPrefs.ignoreWhitespace));
         final PatchScriptBuilder b = newBuilder(list, git);