diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteDraftPatchSetIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteDraftPatchSetIT.java index 3f2107c296..13729ce510 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteDraftPatchSetIT.java +++ b/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(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java index 6d4dd91c05..8ad9cf86d3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java +++ b/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 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); + } + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java index b041112a8c..7bc836fcf9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java +++ b/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, @@ -138,10 +137,11 @@ public class DeleteDraftPatchSet implements RestModifyView reviewers; final List allPastReviewers; final List submitRecords; final Multimap comments; - final Map patchSets; + final TreeMap patchSets; + final Map 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. 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 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 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 deleted = + Sets.newHashSetWithExpectedSize(patchSetStates.size()); + for (Map.Entry 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 it = allChangeMessages.iterator(); + it.hasNext();) { + if (deleted.contains(it.next().getPatchSetId())) { + it.remove(); + } + } + for (Iterator 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 missing = new ArrayList<>(); if (branch == null) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java index e8edb17866..44a3a82539 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -111,6 +111,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { private Set 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) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/PatchSetState.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/PatchSetState.java new file mode 100644 index 0000000000..7fa7cffd7a --- /dev/null +++ b/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. + *

+ * Used internally as a tombstone; patch sets exposed by public notedb + * interfaces never have this state. + */ + DELETED; +} diff --git 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 index 32a5f6ba1c..b79cfa4f89 100644 --- 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 @@ -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, diff --git 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 index 38ee1ce91e..1ed0ae5a00 100644 --- 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 @@ -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);