From 713d2b2c366e1aea0ed642cc90598c11026e63a9 Mon Sep 17 00:00:00 2001
From: Dave Borowitz <dborowitz@google.com>
Date: Thu, 28 Apr 2016 13:25:36 -0400
Subject: [PATCH] ChangeBundle: Ignore null originalSubject in ReviewDb changes

Original subject is a nullable field added relatively recently, so
there are many changes that don't have it set. In NoteDb we
reconstruct this by looking at the patch set commit, so it is always
present. Don't consider this a difference between ReviewDb and NoteDb.

Change-Id: I143430fed2da531806f2b8dcc17465f827df80d7
---
 .../google/gerrit/reviewdb/client/Change.java |  4 ++
 .../gerrit/server/notedb/ChangeBundle.java    | 18 +++++++-
 .../server/notedb/ChangeBundleTest.java       | 45 +++++++++++++++++++
 3 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
index 26bab58bdd..863ed0870f 100644
--- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
+++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java
@@ -572,6 +572,10 @@ public final class Change {
     return originalSubject != null ? originalSubject : subject;
   }
 
+  public String getOriginalSubjectOrNull() {
+    return originalSubject;
+  }
+
   /** Get the id of the most current {@link PatchSet} in this change. */
   public PatchSet.Id currentPatchSetId() {
     if (currentPatchSetId > 0) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
index e6332ea0a4..8654834868 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
@@ -312,8 +312,24 @@ public class ChangeBundle {
     Change a = bundleA.change;
     Change b = bundleB.change;
     String desc = a.getId().equals(b.getId()) ? describe(a.getId()) : "Changes";
+
+    boolean excludeOrigSubj = false;
+    // Ignore null original subject on the ReviewDb side, as this field is
+    // always set in NoteDb.
+    if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB
+        && a.getOriginalSubjectOrNull() == null) {
+      excludeOrigSubj = true;
+    } else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB
+        && b.getOriginalSubjectOrNull() == null) {
+      excludeOrigSubj = true;
+    }
+
+    List<String> exclude = Lists.newArrayList("rowVersion", "noteDbState");
+    if (excludeOrigSubj) {
+      exclude.add("originalSubject");
+    }
     diffColumnsExcluding(diffs, Change.class, desc, bundleA, a, bundleB, b,
-        "rowVersion", "noteDbState");
+        exclude);
   }
 
   private static void diffChangeMessages(List<String> diffs,
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java
index ea12ecaffe..f3a6698046 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java
@@ -181,6 +181,51 @@ public class ChangeBundleTest {
     assertDiffs(b3, b1, msg);
   }
 
+  @Test
+  public void diffChangesIgnoresNullOriginalSubjectInReviewDb()
+      throws Exception {
+    Change c1 = TestChanges.newChange(
+        new Project.NameKey("project"), new Account.Id(100));
+    c1.setCurrentPatchSet(c1.currentPatchSetId(), "Subject", null);
+    Change c2 = clone(c1);
+    c2.setCurrentPatchSet(
+        c2.currentPatchSetId(), c1.getSubject(), "Original subject");
+
+    // Both ReviewDb, exact match required.
+    ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
+        comments(), REVIEW_DB);
+    ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
+        comments(), REVIEW_DB);
+    assertDiffs(b1, b2,
+        "originalSubject differs for Change.Id " + c1.getId() + ":"
+            + " {null} != {Original subject}");
+
+    // Both NoteDb, exact match required.
+    b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(), comments(),
+        NOTE_DB);
+    b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(), comments(),
+        NOTE_DB);
+    assertDiffs(b1, b2,
+        "originalSubject differs for Change.Id " + c1.getId() + ":"
+            + " {null} != {Original subject}");
+
+    // Original subject ignored if ReviewDb has null value.
+    b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(), comments(),
+        REVIEW_DB);
+    b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(), comments(),
+        NOTE_DB);
+    assertNoDiffs(b1, b2);
+
+    // Exact match still required if NoteDb has null value (not realistic).
+    b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(), comments(),
+        NOTE_DB);
+    b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(), comments(),
+        REVIEW_DB);
+    assertDiffs(b1, b2,
+        "originalSubject differs for Change.Id " + c1.getId() + ":"
+            + " {null} != {Original subject}");
+  }
+
   @Test
   public void diffChangeMessageKeySets() throws Exception {
     Change c = TestChanges.newChange(project, accountId);