Notedb: Implement draft patch sets

Store state in notedb as a state enum. Rather than rewriting history,
use a DELETED state for deletion. The state PUBLISHED need only appear
when a patch set is published; otherwise it is implied.

Change-Id: Ie55139286db9270e2d11ae920bfec2240778bd43

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteDraftPatchSetIT.java

@@ -24,6 +24,7 @@ import com.google.gerrit.acceptance.RestResponse;
 import com.google.gerrit.acceptance.RestSession;
 import com.google.gerrit.extensions.client.ChangeStatus;
 import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gwtorm.server.OrmException;
@@ -35,7 +36,7 @@ import java.io.IOException;
 public class DeleteDraftPatchSetIT extends AbstractDaemonTest {
 
   @Test
-  public void deletePatchSet() throws Exception {
+  public void deletePatchSetNotDraft() throws Exception {
     String changeId = createChange().getChangeId();
     PatchSet ps = getCurrentPatchSet(changeId);
     String triplet = project.get() + "~master~" + changeId;
@@ -64,12 +65,17 @@ public class DeleteDraftPatchSetIT extends AbstractDaemonTest {
   public void deleteDraftPatchSetAndChange() throws Exception {
     String changeId = createDraftChangeWith2PS();
     PatchSet ps = getCurrentPatchSet(changeId);
-    String triplet = project.get() + "~master~" + changeId;
-    ChangeInfo c = get(triplet);
-    assertThat(c.id).isEqualTo(triplet);
-    assertThat(c.status).isEqualTo(ChangeStatus.DRAFT);
+
+    ChangeData cd = getChange(changeId);
+    assertThat(cd.patchSets()).hasSize(2);
+    assertThat(cd.change().currentPatchSetId().get()).isEqualTo(2);
+    assertThat(cd.change().getStatus()).isEqualTo(Change.Status.DRAFT);
     deletePatchSet(changeId, ps, adminSession).assertNoContent();
-    assertThat(getChange(changeId).patchSets()).hasSize(1);
+
+    cd = getChange(changeId);
+    assertThat(cd.patchSets()).hasSize(1);
+    assertThat(cd.change().currentPatchSetId().get()).isEqualTo(1);
+
     ps = getCurrentPatchSet(changeId);
     deletePatchSet(changeId, ps, adminSession).assertNoContent();
     assertThat(queryProvider.get().byKeyPrefix(changeId)).isEmpty();

gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java

@@ -15,6 +15,8 @@
 package com.google.gerrit.server;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.gerrit.server.notedb.PatchSetState.DRAFT;
+import static com.google.gerrit.server.notedb.PatchSetState.PUBLISHED;
 
 import com.google.common.collect.ImmutableList;
 import com.google.gerrit.reviewdb.client.Change;
@@ -24,6 +26,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.gerrit.server.notedb.PatchSetState;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
@@ -67,16 +70,7 @@ public class PatchSetUtil {
       PatchSet.Id psId, ObjectId commit, boolean draft,
       Iterable<String> groups, String pushCertificate)
       throws OrmException, IOException {
-    Change.Id changeId = update.getChange().getId();
-    checkArgument(psId.getParentKey().equals(changeId),
-        "cannot insert patch set %s on change %s", psId, changeId);
-    if (update.getPatchSetId() != null) {
-      checkArgument(update.getPatchSetId().equals(psId),
-          "cannot insert patch set %s on update for %s",
-          psId, update.getPatchSetId());
-    } else {
-      update.setPatchSetId(psId);
-    }
+    ensurePatchSetMatches(psId, update);
 
     PatchSet ps = new PatchSet(psId);
     ps.setRevision(new RevId(commit.name()));
@@ -88,7 +82,40 @@ public class PatchSetUtil {
     db.patchSets().insert(Collections.singleton(ps));
 
     update.setCommit(rw, commit);
+    if (draft) {
+      update.setPatchSetState(DRAFT);
+    }
 
     return ps;
   }
+
+  public void publish(ReviewDb db, ChangeUpdate update, PatchSet ps)
+      throws OrmException {
+    ensurePatchSetMatches(ps.getId(), update);
+    ps.setDraft(false);
+    update.setPatchSetState(PUBLISHED);
+    db.patchSets().update(Collections.singleton(ps));
+  }
+
+  public void delete(ReviewDb db, ChangeUpdate update, PatchSet ps)
+      throws OrmException {
+    ensurePatchSetMatches(ps.getId(), update);
+    checkArgument(ps.isDraft(),
+        "cannot delete non-draft patch set %s", ps.getId());
+    update.setPatchSetState(PatchSetState.DELETED);
+    db.patchSets().delete(Collections.singleton(ps));
+  }
+
+  private void ensurePatchSetMatches(PatchSet.Id psId, ChangeUpdate update) {
+    Change.Id changeId = update.getChange().getId();
+    checkArgument(psId.getParentKey().equals(changeId),
+        "cannot modify patch set %s on update for change %s", psId, changeId);
+    if (update.getPatchSetId() != null) {
+      checkArgument(update.getPatchSetId().equals(psId),
+          "cannot modify patch set %s on update for %s",
+          psId, update.getPatchSetId());
+    } else {
+      update.setPatchSetId(psId);
+    }
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java

@@ -46,7 +46,6 @@ import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.transport.ReceiveCommand;
 
 import java.io.IOException;
-import java.util.Collections;
 
 @Singleton
 public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Input>,
@@ -138,10 +137,11 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
       ReviewDb db = ctx.getDb();
       db.accountPatchReviews().delete(db.accountPatchReviews().byPatchSet(psId));
       db.changeMessages().delete(db.changeMessages().byPatchSet(psId));
-      // No need to delete from notedb; draft patch sets will be filtered out.
+      // No need to delete from notedb; deleted patch sets are filtered out.
       db.patchComments().delete(db.patchComments().byPatchSet(psId));
       db.patchSetApprovals().delete(db.patchSetApprovals().byPatchSet(psId));
-      db.patchSets().delete(Collections.singleton(patchSet));
+
+      psUtil.delete(db, ctx.getUpdate(patchSet.getId()), patchSet);
     }
 
     private void deleteOrUpdateDraftChange(ChangeContext ctx)

gerrit-server/src/main/java/com/google/gerrit/server/change/PublishDraftPatchSet.java

@@ -62,7 +62,6 @@ import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.List;
 
 @Singleton
@@ -201,12 +200,11 @@ public class PublishDraftPatchSet implements RestModifyView<RevisionResource, In
       if (!patchSet.isDraft()) {
         throw new ResourceConflictException("Patch set is not a draft");
       }
-      patchSet.setDraft(false);
+      psUtil.publish(ctx.getDb(), ctx.getUpdate(psId), patchSet);
       // Force ETag invalidation if not done already
       if (!wasDraftChange) {
         ctx.saveChange();
       }
-      ctx.getDb().patchSets().update(Collections.singleton(patchSet));
     }
 
     private void addReviewers(ChangeContext ctx)

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java

@@ -81,13 +81,15 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.TreeMap;
 
 class ChangeNotesParser implements AutoCloseable {
   final Map<Account.Id, ReviewerStateInternal> reviewers;
   final List<Account.Id> allPastReviewers;
   final List<SubmitRecord> submitRecords;
   final Multimap<RevId, PatchLineComment> comments;
-  final Map<PatchSet.Id, PatchSet> patchSets;
+  final TreeMap<PatchSet.Id, PatchSet> patchSets;
+  final Map<PatchSet.Id, PatchSetState> patchSetStates;
   NoteMap commentNoteMap;
   String branch;
   Change.Status status;
@@ -125,6 +127,7 @@ class ChangeNotesParser implements AutoCloseable {
     changeMessagesByPatchSet = LinkedListMultimap.create();
     comments = ArrayListMultimap.create();
     patchSets = Maps.newTreeMap(ReviewDbUtil.intKeyOrdering());
+    patchSetStates = Maps.newHashMap();
   }
 
   @Override
@@ -140,6 +143,7 @@ class ChangeNotesParser implements AutoCloseable {
     parseComments();
     allPastReviewers.addAll(reviewers.keySet());
     pruneReviewers();
+    updatePatchSetStates();
     checkMandatoryFooters();
   }
 
@@ -192,6 +196,11 @@ class ChangeNotesParser implements AutoCloseable {
       currentPatchSetId = psId;
     }
 
+    PatchSetState psState = parsePatchSetState(commit);
+    if (psState != null && !patchSetStates.containsKey(psId)) {
+      patchSetStates.put(psId, psState);
+    }
+
     Account.Id accountId = parseIdent(commit);
     ownerId = accountId;
 
@@ -264,6 +273,15 @@ class ChangeNotesParser implements AutoCloseable {
     return footerLines.get(0);
   }
 
+  private String parseExactlyOneFooter(RevCommit commit, FooterKey footerKey)
+      throws ConfigInvalidException {
+    String line = parseOneFooter(commit, footerKey);
+    if (line == null) {
+      throw expectedOneFooter(footerKey, Collections.<String> emptyList());
+    }
+    return line;
+  }
+
   private ObjectId parseRevision(RevCommit commit)
       throws ConfigInvalidException {
     String sha = parseOneFooter(commit, FOOTER_COMMIT);
@@ -330,17 +348,34 @@ class ChangeNotesParser implements AutoCloseable {
 
   private PatchSet.Id parsePatchSetId(RevCommit commit)
       throws ConfigInvalidException {
-    List<String> psIdLines = commit.getFooterLines(FOOTER_PATCH_SET);
-    if (psIdLines.size() != 1) {
-      throw expectedOneFooter(FOOTER_PATCH_SET, psIdLines);
-    }
-    Integer psId = Ints.tryParse(psIdLines.get(0));
+    String psIdLine = parseExactlyOneFooter(commit, FOOTER_PATCH_SET);
+    int s = psIdLine.indexOf(' ');
+    String psIdStr = s < 0 ? psIdLine : psIdLine.substring(0, s);
+    Integer psId = Ints.tryParse(psIdStr);
     if (psId == null) {
-      throw invalidFooter(FOOTER_PATCH_SET, psIdLines.get(0));
+      throw invalidFooter(FOOTER_PATCH_SET, psIdStr);
     }
     return new PatchSet.Id(changeId, psId);
   }
 
+  private PatchSetState parsePatchSetState(RevCommit commit)
+      throws ConfigInvalidException {
+    String psIdLine = parseExactlyOneFooter(commit, FOOTER_PATCH_SET);
+    int s = psIdLine.indexOf(' ');
+    if (s < 0) {
+      return null;
+    }
+    String withParens = psIdLine.substring(s + 1);
+    if (withParens.startsWith("(") && withParens.endsWith(")")) {
+      Optional<PatchSetState> state = Enums.getIfPresent(PatchSetState.class,
+          withParens.substring(1, withParens.length() - 1).toUpperCase());
+      if (state.isPresent()) {
+        return state.get();
+      }
+    }
+    throw invalidFooter(FOOTER_PATCH_SET, psIdLine);
+  }
+
   private void parseChangeMessage(PatchSet.Id psId, Account.Id accountId,
       RevCommit commit, Timestamp ts) {
     byte[] raw = commit.getRawBuffer();
@@ -599,6 +634,58 @@ class ChangeNotesParser implements AutoCloseable {
     }
   }
 
+  private void updatePatchSetStates() {
+    Set<PatchSet.Id> deleted =
+        Sets.newHashSetWithExpectedSize(patchSetStates.size());
+    for (Map.Entry<PatchSet.Id, PatchSetState> e : patchSetStates.entrySet()) {
+      switch (e.getValue()) {
+        case PUBLISHED:
+        default:
+          break;
+
+        case DELETED:
+          deleted.add(e.getKey());
+          break;
+
+        case DRAFT:
+          PatchSet ps = patchSets.get(e.getKey());
+          if (ps != null) {
+            ps.setDraft(true);
+          }
+          break;
+      }
+    }
+    if (deleted.isEmpty()) {
+      return;
+    }
+
+    // Post-process other collections to remove items corresponding to deleted
+    // patch sets. This is safer than trying to prevent insertion, as it will
+    // also filter out items racily added after the patch set was deleted.
+    patchSets.keySet().removeAll(deleted);
+    if (!patchSets.keySet().isEmpty()) {
+      currentPatchSetId = patchSets.navigableKeySet().last();
+    } else {
+      currentPatchSetId = null;
+    }
+    approvals.keySet().removeAll(deleted);
+    changeMessagesByPatchSet.keys().removeAll(deleted);
+
+    for (Iterator<ChangeMessage> it = allChangeMessages.iterator();
+        it.hasNext();) {
+      if (deleted.contains(it.next().getPatchSetId())) {
+        it.remove();
+      }
+    }
+    for (Iterator<PatchLineComment> it = comments.values().iterator();
+        it.hasNext();) {
+      PatchSet.Id psId = it.next().getKey().getParentKey().getParentKey();
+      if (deleted.contains(psId)) {
+        it.remove();
+      }
+    }
+  }
+
   private void checkMandatoryFooters() throws ConfigInvalidException {
     List<FooterKey> missing = new ArrayList<>();
     if (branch == null) {

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java

@@ -111,6 +111,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   private Set<String> hashtags;
   private String changeMessage;
   private ChangeNotes notes;
+  private PatchSetState psState;
 
   private final ChangeDraftUpdate.Factory draftUpdateFactory;
   private ChangeDraftUpdate draftUpdate;
@@ -385,6 +386,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     reviewers.put(reviewer, ReviewerStateInternal.REMOVED);
   }
 
+  public void setPatchSetState(PatchSetState psState) {
+    this.psState = psState;
+  }
+
   /** @return the tree id for the updated tree */
   private ObjectId storeCommentsInNotes() throws OrmException, IOException {
     ChangeNotes notes = ctl.getNotes().load();
@@ -468,8 +473,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
       msg.append("\n\n");
     }
 
-
-    addFooter(msg, FOOTER_PATCH_SET, ps);
+    addPatchSetFooter(msg, ps);
 
     if (subject != null) {
       addFooter(msg, FOOTER_SUBJECT, subject);
@@ -549,6 +553,14 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     return true;
   }
 
+  private void addPatchSetFooter(StringBuilder sb, int ps) {
+    addFooter(sb, FOOTER_PATCH_SET).append(ps);
+    if (psState != null) {
+      sb.append(" (").append(psState.name().toLowerCase()).append(')');
+    }
+    sb.append('\n');
+  }
+
   @Override
   protected Project.NameKey getProjectName() {
     return getProjectName(ctl);
@@ -566,7 +578,8 @@ public class ChangeUpdate extends AbstractChangeUpdate {
         && submitRecords == null
         && hashtags == null
         && topic == null
-        && commit == null;
+        && commit == null
+        && psState == null;
   }
 
   private static StringBuilder addFooter(StringBuilder sb, FooterKey footer) {

gerrit-server/src/main/java/com/google/gerrit/server/notedb/PatchSetState.java

@@ -0,0 +1,31 @@
+// Copyright (C) 2016 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.notedb;
+
+public enum PatchSetState {
+  /** Published and visible to anyone who can see the change; the default.*/
+  PUBLISHED,
+
+  /** Draft patch set, only visible to certain users. */
+  DRAFT,
+
+  /**
+   * Deleted patch set.
+   * <p>
+   * Used internally as a tombstone; patch sets exposed by public notedb
+   * interfaces never have this state.
+   */
+  DELETED;
+}

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java

@@ -295,6 +295,30 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
         + "Commit: beef");
   }
 
+  @Test
+  public void parsePatchSetState() throws Exception {
+    assertParseSucceeds("Update change\n"
+        + "\n"
+        + "Patch-set: 1 (PUBLISHED)\n"
+        + "Branch: refs/heads/master\n"
+        + "Subject: Some subject of a change\n");
+    assertParseSucceeds("Update change\n"
+        + "\n"
+        + "Patch-set: 1 (DRAFT)\n"
+        + "Branch: refs/heads/master\n"
+        + "Subject: Some subject of a change\n");
+    assertParseSucceeds("Update change\n"
+        + "\n"
+        + "Patch-set: 1 (DELETED)\n"
+        + "Branch: refs/heads/master\n"
+        + "Subject: Some subject of a change\n");
+    assertParseFails("Update change\n"
+        + "\n"
+        + "Patch-set: 1 (NOT A STATUS)\n"
+        + "Branch: refs/heads/master\n"
+        + "Subject: Some subject of a change\n");
+  }
+
   private RevCommit writeCommit(String body) throws Exception {
     return writeCommit(body, ChangeNoteUtil.newIdent(
         changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent,

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java

@@ -610,6 +610,54 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertThat(ps2.getCreatedOn()).isEqualTo(update.getWhen());
   }
 
+  @Test
+  public void patchSetStates() throws Exception {
+    Change c = newChange();
+    PatchSet.Id psId1 = c.currentPatchSetId();
+
+    // ps2
+    incrementPatchSet(c);
+    PatchSet.Id psId2 = c.currentPatchSetId();
+    RevCommit commit = tr.commit().message("PS2").create();
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.setCommit(rw, commit);
+    update.setPatchSetState(PatchSetState.DRAFT);
+    update.putApproval("Code-Review", (short) 1);
+    update.setChangeMessage("This is a message");
+    update.insertComment(newPublishedComment(c.currentPatchSetId(), "a.txt",
+        "uuid1", new CommentRange(1, 2, 3, 4), 1, changeOwner, null,
+        TimeUtil.nowTs(), "Comment", (short) 1, commit.name()));
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getPatchSets().get(psId2).isDraft()).isTrue();
+    assertThat(notes.getPatchSets().keySet()).containsExactly(psId1, psId2);
+    assertThat(notes.getApprovals()).isNotEmpty();
+    assertThat(notes.getChangeMessagesByPatchSet()).isNotEmpty();
+    assertThat(notes.getChangeMessages()).isNotEmpty();
+    assertThat(notes.getComments()).isNotEmpty();
+
+    // publish ps2
+    update = newUpdate(c, changeOwner);
+    update.setPatchSetState(PatchSetState.PUBLISHED);
+    update.commit();
+
+    notes = newNotes(c);
+    assertThat(notes.getPatchSets().get(psId2).isDraft()).isFalse();
+
+    // delete ps2
+    update = newUpdate(c, changeOwner);
+    update.setPatchSetState(PatchSetState.DELETED);
+    update.commit();
+
+    notes = newNotes(c);
+    assertThat(notes.getPatchSets().keySet()).containsExactly(psId1);
+    assertThat(notes.getApprovals()).isEmpty();
+    assertThat(notes.getChangeMessagesByPatchSet()).isEmpty();
+    assertThat(notes.getChangeMessages()).isEmpty();
+    assertThat(notes.getComments()).isEmpty();
+  }
+
   @Test
   public void emptyExceptSubject() throws Exception {
     ChangeUpdate update = newUpdate(newChange(), changeOwner);