Merge "Keep track of deleted PS to avoid using stale meta data in NoteDB"

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

@@ -40,6 +40,8 @@ import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.inject.Inject;
 
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
 import org.junit.Test;
@@ -190,6 +192,62 @@ public class DeleteDraftPatchSetIT extends AbstractDaemonTest {
     }
   }
 
+  @Test
+  public void deleteDraftPatchSetAndPushNewDraftPatchSet() throws Exception {
+    String ref = "refs/drafts/master";
+
+    // Clone repository
+    TestRepository<InMemoryRepository> testRepo =
+        cloneProject(project, admin);
+
+    // Create change
+    PushOneCommit push = pushFactory.create(
+        db, admin.getIdent(), testRepo);
+    PushOneCommit.Result r1 = push.to(ref);
+    r1.assertOkStatus();
+    String revPs1 = r1.getChange().currentPatchSet().getRevision().get();
+
+    // Push draft patch set
+    PushOneCommit.Result r2 = amendChange(
+        r1.getChangeId(), ref, admin, testRepo);
+    r2.assertOkStatus();
+    String revPs2 = r2.getChange().currentPatchSet().getRevision().get();
+
+    assertThat(
+        gApi.changes()
+            .id(r1.getChange().getId().get()).get()
+            .currentRevision)
+        .isEqualTo(revPs2);
+
+    // Remove draft patch set
+    gApi.changes()
+        .id(r1.getChange().getId().get())
+        .revision(revPs2)
+        .delete();
+
+    assertThat(
+        gApi.changes()
+            .id(r1.getChange().getId().get()).get()
+            .currentRevision)
+        .isEqualTo(revPs1);
+
+    // Push new draft patch set
+    PushOneCommit.Result r3 = amendChange(
+        r1.getChangeId(), ref, admin, testRepo);
+    r3.assertOkStatus();
+    String revPs3 = r2.getChange().currentPatchSet().getRevision().get();
+
+    assertThat(
+        gApi.changes()
+            .id(r1.getChange().getId().get()).get()
+            .currentRevision)
+        .isEqualTo(revPs3);
+
+    // Check that all patch sets have different SHA1s
+    assertThat(revPs1).doesNotMatch(revPs2);
+    assertThat(revPs2).doesNotMatch(revPs3);
+  }
+
   private Ref getDraftRef(TestAccount account, Change.Id changeId)
       throws Exception {
     try (Repository repo = repoManager.openRepository(allUsers)) {

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

@@ -79,6 +79,7 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -108,6 +109,7 @@ class ChangeNotesParser {
   private final List<SubmitRecord> submitRecords;
   private final Multimap<RevId, PatchLineComment> comments;
   private final TreeMap<PatchSet.Id, PatchSet> patchSets;
+  private final Set<PatchSet.Id> deletedPatchSets;
   private final Map<PatchSet.Id, PatchSetState> patchSetStates;
   private final Map<PatchSet.Id,
       Table<Account.Id, Entry<String, String>, Optional<PatchSetApproval>>> approvals;
@@ -145,6 +147,7 @@ class ChangeNotesParser {
     changeMessagesByPatchSet = LinkedListMultimap.create();
     comments = ArrayListMultimap.create();
     patchSets = Maps.newTreeMap(ReviewDbUtil.intKeyOrdering());
+    deletedPatchSets = new HashSet<>();
     patchSetStates = new HashMap<>();
   }
 
@@ -249,8 +252,13 @@ class ChangeNotesParser {
     }
 
     PatchSetState psState = parsePatchSetState(commit);
-    if (psState != null && !patchSetStates.containsKey(psId)) {
-      patchSetStates.put(psId, psState);
+    if (psState != null) {
+      if (!patchSetStates.containsKey(psId)) {
+        patchSetStates.put(psId, psState);
+      }
+      if (psState == PatchSetState.DELETED) {
+        deletedPatchSets.add(psId);
+      }
     }
 
     Account.Id accountId = parseIdent(commit);
@@ -382,6 +390,11 @@ class ChangeNotesParser {
       ps = new PatchSet(psId);
       patchSets.put(psId, ps);
     } else if (ps.getRevision() != PARTIAL_PATCH_SET) {
+      if (deletedPatchSets.contains(psId)) {
+        // Do not update PS details as PS was deleted and this meta data is of
+        // no relevance
+        return;
+      }
       throw new ConfigInvalidException(
           String.format(
               "Multiple revisions parsed for patch set %s: %s and %s",