Support setting current patch set in NoteDb

Until now, NoteDb was not explicitly recording the current patch set, and instead just assuming that the highest-numbered non-deleted patch set was current. This is usually fine, but there is at least one corner case where it's not. Specifically, when pushing an old patch set of a change directly to a branch: * The change moves to state MERGED. * The currentPatchSetId field gets set to the corresponding patch set ID of the merged commit, even if this is less than the maximum patch set in the database. Teach NoteDb to handle this case with a footer "Current: true" indicating the patch set mentioned in the "Patch-set" footer is the current patch set. Creating a new patch set also implicitly sets it to current, matching existing data. Various parts of the code in ChangeRebuilderImpl and ChangeBundle were assuming that any entities referring to patch sets greater than the current patch set were ignorable. Remove this assumption, and ensure we're handling these entities properly. Add a test for the above case. Change-Id: I099fd703458ffd63a31009ceaaa1c1d2c59d4669
2016-12-12 12:58:32 -05:00
parent 81f334a77f
commit e5ee0d3865
13 changed files with 252 additions and 111 deletions
--- 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
@@ -769,39 +769,6 @@ public class ChangeBundleTest extends GerritBaseTests {
            + "Only in B:\n  " + cm1);
  }

-  @Test
-  public void diffChangeMessagesIgnoresMessagesOnPatchSetGreaterThanCurrent()
-      throws Exception {
-    Change c = TestChanges.newChange(project, accountId);
-
-    PatchSet ps1 = new PatchSet(new PatchSet.Id(c.getId(), 1));
-    ps1.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
-    ps1.setUploader(accountId);
-    ps1.setCreatedOn(TimeUtil.nowTs());
-    PatchSet ps2 = new PatchSet(new PatchSet.Id(c.getId(), 2));
-    ps2.setRevision(new RevId("badc0feebadc0feebadc0feebadc0feebadc0fee"));
-    ps2.setUploader(accountId);
-    ps2.setCreatedOn(TimeUtil.nowTs());
-
-    assertThat(c.currentPatchSetId()).isEqualTo(ps1.getId());
-
-    ChangeMessage cm1 = new ChangeMessage(
-        new ChangeMessage.Key(c.getId(), "uuid1"),
-        accountId, TimeUtil.nowTs(), ps1.getId());
-    cm1.setMessage("a message");
-    ChangeMessage cm2 = new ChangeMessage(
-        new ChangeMessage.Key(c.getId(), "uuid2"),
-        accountId, TimeUtil.nowTs(), ps2.getId());
-    cm2.setMessage("other message");
-
-    ChangeBundle b1 = new ChangeBundle(c, messages(cm1, cm2),
-        patchSets(ps1, ps2), approvals(), comments(), reviewers(), REVIEW_DB);
-    ChangeBundle b2 = new ChangeBundle(c, messages(cm1), patchSets(ps1),
-        approvals(), comments(), reviewers(), NOTE_DB);
-    assertNoDiffs(b1, b2);
-    assertNoDiffs(b2, b1);
-  }
-
  @Test
  public void diffPatchSetIdSets() throws Exception {
    Change c = TestChanges.newChange(project, accountId);
@@ -919,7 +886,7 @@ public class ChangeBundleTest extends GerritBaseTests {
  }

  @Test
-  public void diffIgnoresPatchSetsGreaterThanCurrent() throws Exception {
+  public void diffPatchSetsGreaterThanCurrent() throws Exception {
    Change c = TestChanges.newChange(project, accountId);

    PatchSet ps1 = new PatchSet(new PatchSet.Id(c.getId(), 1));
@@ -932,6 +899,13 @@ public class ChangeBundleTest extends GerritBaseTests {
    ps2.setCreatedOn(TimeUtil.nowTs());
    assertThat(ps2.getId().get()).isGreaterThan(c.currentPatchSetId().get());

+    ChangeMessage cm1 = new ChangeMessage(
+        new ChangeMessage.Key(c.getId(), "uuid1"),
+        accountId, TimeUtil.nowTs(), c.currentPatchSetId());
+    ChangeMessage cm2 = new ChangeMessage(
+        new ChangeMessage.Key(c.getId(), "uuid2"),
+        accountId, TimeUtil.nowTs(), c.currentPatchSetId());
+
    PatchSetApproval a1 = new PatchSetApproval(
        new PatchSetApproval.Key(
            ps1.getId(), accountId, new LabelId("Code-Review")),
@@ -944,26 +918,44 @@ public class ChangeBundleTest extends GerritBaseTests {
        TimeUtil.nowTs());

    // Both ReviewDb.
-    ChangeBundle b1 = new ChangeBundle(c, messages(), patchSets(ps1),
+    ChangeBundle b1 = new ChangeBundle(c, messages(cm1), patchSets(ps1),
        approvals(a1), comments(), reviewers(), REVIEW_DB);
-    ChangeBundle b2 = new ChangeBundle(c, messages(), patchSets(ps1, ps2),
-        approvals(a1, a2), comments(), reviewers(), REVIEW_DB);
-    assertNoDiffs(b1, b2);
+    ChangeBundle b2 = new ChangeBundle(c, messages(cm1, cm2),
+        patchSets(ps1, ps2), approvals(a1, a2), comments(), reviewers(),
+        REVIEW_DB);
+    assertDiffs(b1, b2,
+        "ChangeMessage.Key sets differ: [] only in A; [" + cm2.getKey()
+            + "] only in B",
+        "PatchSet.Id sets differ:"
+            + " [] only in A; [" + ps2.getId() + "] only in B",
+        "PatchSetApproval.Key sets differ:"
+            + " [] only in A; [" + a2.getKey() + "] only in B");

    // One NoteDb.
-    b1 = new ChangeBundle(c, messages(), patchSets(ps1), approvals(a1),
+    b1 = new ChangeBundle(c, messages(cm1), patchSets(ps1), approvals(a1),
        comments(), reviewers(), NOTE_DB);
-    b2 = new ChangeBundle(c, messages(), patchSets(ps1, ps2), approvals(a1, a2),
+    b2 = new ChangeBundle(c, messages(cm1, cm2), patchSets(ps1, ps2), approvals(a1, a2),
        comments(), reviewers(), REVIEW_DB);
-    assertNoDiffs(b1, b2);
-    assertNoDiffs(b2, b1);
+    assertDiffs(b1, b2,
+        "ChangeMessages differ for Change.Id " + c.getId() + "\n"
+            + "Only in B:\n  " + cm2,
+        "PatchSet.Id sets differ:"
+            + " [] only in A; [" + ps2.getId() + "] only in B",
+        "PatchSetApproval.Key sets differ:"
+            + " [] only in A; [" + a2.getKey() + "] only in B");

    // Both NoteDb.
-    b1 = new ChangeBundle(c, messages(), patchSets(ps1), approvals(a1),
+    b1 = new ChangeBundle(c, messages(cm1), patchSets(ps1), approvals(a1),
        comments(), reviewers(), NOTE_DB);
-    b2 = new ChangeBundle(c, messages(), patchSets(ps1, ps2), approvals(a1, a2),
+    b2 = new ChangeBundle(c, messages(cm1, cm2), patchSets(ps1, ps2), approvals(a1, a2),
        comments(), reviewers(), NOTE_DB);
-    assertNoDiffs(b1, b2);
+    assertDiffs(b1, b2,
+        "ChangeMessages differ for Change.Id " + c.getId() + "\n"
+            + "Only in B:\n  " + cm2,
+        "PatchSet.Id sets differ:"
+            + " [] only in A; [" + ps2.getId() + "] only in B",
+        "PatchSetApproval.Key sets differ:"
+            + " [] only in A; [" + a2.getKey() + "] only in B");
  }

  @Test
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
@@ -448,6 +448,26 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
        + "subject: This is a test change\n");
  }

+  @Test
+  public void currentPatchSet() throws Exception {
+    assertParseSucceeds("Update change\n"
+        + "\n"
+        + "Patch-set: 1\n"
+        + "Current: true");
+    assertParseSucceeds("Update change\n"
+        + "\n"
+        + "Patch-set: 1\n"
+        + "Current: tRUe");
+    assertParseFails("Update change\n"
+        + "\n"
+        + "Patch-set: 1\n"
+        + "Current: false");
+    assertParseFails("Update change\n"
+        + "\n"
+        + "Patch-set: 1\n"
+        + "Current: blah");
+  }
+
  private RevCommit writeCommit(String body) throws Exception {
    ChangeNoteUtil noteUtil = injector.getInstance(ChangeNoteUtil.class);
    return writeCommit(body, noteUtil.newIdent(
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -2631,6 +2631,38 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
    assertThat(notes.getComments()).hasSize(numComments);
  }

+  @Test
+  public void currentPatchSet() throws Exception {
+    Change c = newChange();
+    assertThat(newNotes(c).getChange().currentPatchSetId().get()).isEqualTo(1);
+
+    incrementPatchSet(c);
+    assertThat(newNotes(c).getChange().currentPatchSetId().get()).isEqualTo(2);
+
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.setPatchSetId(new PatchSet.Id(c.getId(), 1));
+    update.setCurrentPatchSet();
+    update.commit();
+    assertThat(newNotes(c).getChange().currentPatchSetId().get()).isEqualTo(1);
+
+    incrementPatchSet(c);
+    assertThat(newNotes(c).getChange().currentPatchSetId().get()).isEqualTo(3);
+
+    // Delete PS3, PS1 becomes current, as the most recent event explicitly set
+    // it to current.
+    update = newUpdate(c, changeOwner);
+    update.setPatchSetState(PatchSetState.DELETED);
+    update.commit();
+    assertThat(newNotes(c).getChange().currentPatchSetId().get()).isEqualTo(1);
+
+    // Delete PS1, PS2 becomes current.
+    update = newUpdate(c, changeOwner);
+    update.setPatchSetId(new PatchSet.Id(c.getId(), 1));
+    update.setPatchSetState(PatchSetState.DELETED);
+    update.commit();
+    assertThat(newNotes(c).getChange().currentPatchSetId().get()).isEqualTo(2);
+  }
+
  private boolean testJson() {
    return noteUtil.getWriteJson();
  }
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
@@ -356,6 +356,20 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
        commit);
  }

+  @Test
+  public void currentPatchSet() throws Exception {
+    Change c = newChange();
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.setCurrentPatchSet();
+    update.commit();
+
+    assertBodyEquals("Update patch set 1\n"
+        + "\n"
+        + "Patch-set: 1\n"
+        + "Current: true\n",
+        update.getResult());
+  }
+
  private RevCommit parseCommit(ObjectId id) throws Exception {
    if (id instanceof RevCommit) {
      return (RevCommit) id;