opendev/gerrit
Code Issues Proposed changes

Merge changes If1fccba0,I16d595a6,I0916ff07,I9a91fae2

Browse Source 
* changes:
  ReplaceOp: Delete dead code
  ChangeRebuilderImpl: Prune ChangeMessages for missing patch sets
  ChangeNotesParser: Filter out entities for missing patch sets
  ChangeNotestTest: Always add new patch set
This commit is contained in:
Dave Borowitz
2016-11-03 11:24:27 +00:00
committed by Gerrit Code Review
parent 76eac6f3fc ce60204753
commit 57b91a3a87
7 changed files with 158 additions and 79 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
View File

@@ -20,6 +20,7 @@ import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef;
import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.stream.Collectors.toList;
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
import static org.junit.Assert.fail;
@@ -1088,6 +1089,32 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
checker.rebuildAndCheckChanges(id);
}
@Test
public void ignoreChangeMessageBeyondCurrentPatchSet() throws Exception {
PushOneCommit.Result r = createChange();
PatchSet.Id psId1 = r.getPatchSetId();
Change.Id id = psId1.getParentKey();
gApi.changes().id(id.get()).current().review(ReviewInput.recommend());
r = amendChange(r.getChangeId());
PatchSet.Id psId2 = r.getPatchSetId();
assertThat(db.patchSets().byChange(id)).hasSize(2);
assertThat(db.changeMessages().byPatchSet(psId2)).hasSize(1);
db.patchSets().deleteKeys(Collections.singleton(psId2));
checker.rebuildAndCheckChanges(psId2.getParentKey());
setNotesMigration(true, true);
ChangeData cd = changeDataFactory.create(db, project, id);
assertThat(cd.change().currentPatchSetId()).isEqualTo(psId1);
assertThat(cd.patchSets().stream().map(ps -> ps.getId()).collect(toList()))
.containsExactly(psId1);
PatchSet ps = cd.currentPatchSet();
assertThat(ps).isNotNull();
assertThat(ps.getId()).isEqualTo(psId1);
}
private void assertChangesReadOnly(RestApiException e) throws Exception {
Throwable cause = e.getCause();
assertThat(cause).isInstanceOf(UpdateException.class);

13
gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
View File

@@ -69,7 +69,6 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@@ -289,7 +288,7 @@ public class ReplaceOp extends BatchUpdate.Op {
cmUtil.addChangeMessage(ctx.getDb(), update, msg);
if (mergedByPushOp == null) {
resetChange(ctx, msg);
resetChange(ctx);
} else {
mergedByPushOp.setPatchSetProvider(Providers.of(newPatchSet))
.updateChange(ctx);
@@ -333,16 +332,8 @@ public class ReplaceOp extends BatchUpdate.Op {
return current;
}
private void resetChange(ChangeContext ctx, ChangeMessage msg)
throws OrmException {
private void resetChange(ChangeContext ctx) {
Change change = ctx.getChange();
if (change.getStatus().isClosed()) {
ctx.getDb().patchSets().delete(Collections.singleton(newPatchSet));
ctx.getDb().changeMessages().delete(Collections.singleton(msg));
rejectMessage = CHANGE_IS_CLOSED;
return;
}
if (!change.currentPatchSetId().equals(priorPatchSetId)) {
return;
}

6
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
View File

@@ -327,16 +327,16 @@ public class ChangeBundle {
private Timestamp getLatestTimestamp() {
Ordering<Timestamp> o = Ordering.natural().nullsFirst();
Timestamp ts = null;
for (ChangeMessage cm : getChangeMessages()) {
for (ChangeMessage cm : filterChangeMessages()) {
ts = o.max(ts, cm.getWrittenOn());
}
for (PatchSet ps : getPatchSets()) {
ts = o.max(ts, ps.getCreatedOn());
}
for (PatchSetApproval psa : getPatchSetApprovals()) {
for (PatchSetApproval psa : filterPatchSetApprovals().values()) {
ts = o.max(ts, psa.getGranted());
}
for (PatchLineComment plc : getPatchLineComments()) {
for (PatchLineComment plc : filterPatchLineComments().values()) {
// Ignore draft comments, as they do not show up in the change meta graph.
if (plc.getStatus() != PatchLineComment.Status.DRAFT) {
ts = o.max(ts, plc.getWrittenOn());

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

@@ -60,6 +60,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.ReviewerStatusUpdate;
import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
@@ -73,6 +74,8 @@ import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.notes.NoteMap;
import org.eclipse.jgit.revwalk.FooterKey;
import org.eclipse.jgit.util.RawParseUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.nio.charset.Charset;
@@ -91,8 +94,13 @@ import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.TreeMap;
import java.util.TreeSet;
import java.util.function.Function;
class ChangeNotesParser {
private static final Logger log =
LoggerFactory.getLogger(ChangeNotesParser.class);
// Sentinel RevId indicating a mutable field on a patch set was parsed, but
// the parser does not yet know its commit SHA-1.
private static final RevId PARTIAL_PATCH_SET =
@@ -232,8 +240,8 @@ class ChangeNotesParser {
private Multimap<PatchSet.Id, PatchSetApproval> buildApprovals() {
Multimap<PatchSet.Id, PatchSetApproval> result = ArrayListMultimap.create();
for (PatchSetApproval a : approvals.values()) {
if (patchSetStates.get(a.getPatchSetId()) == PatchSetState.DELETED) {
continue; // Patch set was explicitly deleted.
if (!patchSets.containsKey(a.getPatchSetId())) {
continue; // Patch set deleted or missing.
} else if (allPastReviewers.contains(a.getAccountId())
&& !reviewers.containsRow(a.getAccountId())) {
continue; // Reviewer was explicitly removed.
@@ -283,10 +291,6 @@ class ChangeNotesParser {
}
PatchSet.Id psId = parsePatchSetId(commit);
if (currentPatchSetId == null || psId.get() > currentPatchSetId.get()) {
currentPatchSetId = psId;
}
PatchSetState psState = parsePatchSetState(commit);
if (psState != null) {
if (!patchSetStates.containsKey(psId)) {
@@ -874,18 +878,16 @@ class ChangeNotesParser {
}
}
private void updatePatchSetStates() throws ConfigInvalidException {
for (PatchSet ps : patchSets.values()) {
private void updatePatchSetStates() {
Set<PatchSet.Id> missing = new TreeSet<>(ReviewDbUtil.intKeyOrdering());
for (Iterator<PatchSet> it = patchSets.values().iterator();
it.hasNext();) {
PatchSet ps = it.next();
if (ps.getRevision().equals(PARTIAL_PATCH_SET)) {
throw parseException("No %s found for patch set %s",
FOOTER_COMMIT, ps.getPatchSetId());
missing.add(ps.getId());
it.remove();
}
}
if (patchSetStates.isEmpty()) {
return;
}
boolean deleted = false;
for (Map.Entry<PatchSet.Id, PatchSetState> e : patchSetStates.entrySet()) {
switch (e.getValue()) {
case PUBLISHED:
@@ -893,7 +895,6 @@ class ChangeNotesParser {
break;
case DELETED:
deleted = true;
patchSets.remove(e.getKey());
break;
@@ -905,15 +906,11 @@ class ChangeNotesParser {
break;
}
}
if (!deleted) {
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.
//
// Approvals are filtered in buildApprovals().
// (or otherwise missing) 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.
NavigableSet<PatchSet.Id> all = patchSets.navigableKeySet();
if (!all.isEmpty()) {
currentPatchSetId = all.last();
@@ -922,19 +919,35 @@ class ChangeNotesParser {
}
changeMessagesByPatchSet.keys().retainAll(all);
for (Iterator<ChangeMessage> it = allChangeMessages.iterator();
it.hasNext();) {
if (!all.contains(it.next().getPatchSetId())) {
it.remove();
}
}
for (Iterator<Comment> it = comments.values().iterator();
it.hasNext();) {
PatchSet.Id psId = new PatchSet.Id(id, it.next().key.patchSetId);
if (!all.contains(psId)) {
int pruned = pruneEntitiesForMissingPatchSets(
allChangeMessages, ChangeMessage::getPatchSetId, missing);
pruned += pruneEntitiesForMissingPatchSets(
comments.values(), c -> new PatchSet.Id(id, c.key.patchSetId), missing);
pruned += pruneEntitiesForMissingPatchSets(
approvals.values(), PatchSetApproval::getPatchSetId, missing);
if (!missing.isEmpty()) {
log.warn(
"ignoring {} additional entities due to missing patch sets: {}",
pruned, missing);
}
}
private <T> int pruneEntitiesForMissingPatchSets(
Iterable<T> ents, Function<T, PatchSet.Id> psIdFunc,
Set<PatchSet.Id> missing) {
int pruned = 0;
for (Iterator<T> it = ents.iterator(); it.hasNext();) {
PatchSet.Id psId = psIdFunc.apply(it.next());
if (!patchSets.containsKey(psId)) {
pruned++;
missing.add(psId);
it.remove();
} else if (deletedPatchSets.contains(psId)) {
it.remove(); // Not an error we need to report, don't increment pruned.
}
}
return pruned;
}
private void checkMandatoryFooters() throws ConfigInvalidException {

9
gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java
View File

@@ -319,10 +319,11 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
List<Event> msgEvents = parseChangeMessage(msg, change, noteDbChange);
if (msg.getPatchSetId() != null) {
PatchSetEvent pse = patchSetEvents.get(msg.getPatchSetId());
if (pse != null) {
for (Event e : msgEvents) {
e.addDep(pse);
}
if (pse == null) {
continue; // Ignore events for missing patch sets.
}
for (Event e : msgEvents) {
e.addDep(pse);
}
}
events.addAll(msgEvents);

22
gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java
View File

@@ -328,6 +328,10 @@ public class ChangeBundleTest {
throws Exception {
Change c1 = TestChanges.newChange(
new Project.NameKey("project"), new Account.Id(100));
PatchSet ps = new PatchSet(c1.currentPatchSetId());
ps.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
ps.setUploader(accountId);
ps.setCreatedOn(TimeUtil.nowTs());
PatchSetApproval a = new PatchSetApproval(
new PatchSetApproval.Key(
c1.currentPatchSetId(), accountId, new LabelId("Code-Review")),
@@ -338,16 +342,16 @@ public class ChangeBundleTest {
c2.setLastUpdatedOn(a.getGranted());
// Both ReviewDb, exact match required.
ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(),
ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(ps),
approvals(a), comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(),
ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(ps),
approvals(a), comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"effective last updated time differs for Change.Id " + c1.getId() + ":"
+ " {2009-09-30 17:00:00.0} != {2009-09-30 17:00:06.0}");
+ " {2009-09-30 17:00:00.0} != {2009-09-30 17:00:12.0}");
// NoteDb allows latest timestamp from all entities in bundle.
b2 = new ChangeBundle(c2, messages(), patchSets(),
b2 = new ChangeBundle(c2, messages(), patchSets(ps),
approvals(a), comments(), reviewers(), NOTE_DB);
assertNoDiffs(b1, b2);
}
@@ -356,6 +360,10 @@ public class ChangeBundleTest {
public void diffChangesIgnoresChangeTimestampIfAnyOtherEntitiesExist() {
Change c1 = TestChanges.newChange(
new Project.NameKey("project"), new Account.Id(100));
PatchSet ps = new PatchSet(c1.currentPatchSetId());
ps.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
ps.setUploader(accountId);
ps.setCreatedOn(TimeUtil.nowTs());
PatchSetApproval a = new PatchSetApproval(
new PatchSetApproval.Key(
c1.currentPatchSetId(), accountId, new LabelId("Code-Review")),
@@ -368,9 +376,9 @@ public class ChangeBundleTest {
// ReviewDb has later lastUpdatedOn timestamp than NoteDb, allowed since
// NoteDb matches the latest timestamp of a non-Change entity.
ChangeBundle b1 = new ChangeBundle(c2, messages(), patchSets(),
ChangeBundle b1 = new ChangeBundle(c2, messages(), patchSets(ps),
approvals(a), comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c1, messages(), patchSets(),
ChangeBundle b2 = new ChangeBundle(c1, messages(), patchSets(ps),
approvals(a), comments(), reviewers(), NOTE_DB);
assertThat(b1.getChange().getLastUpdatedOn())
.isGreaterThan(b2.getChange().getLastUpdatedOn());
@@ -384,7 +392,7 @@ public class ChangeBundleTest {
assertDiffs(b1, b2,
"effective last updated time differs for Change.Id " + c1.getId()
+ " in NoteDb vs. ReviewDb:"
+ " {2009-09-30 17:00:06.0} != {2009-09-30 17:00:12.0}");
+ " {2009-09-30 17:00:12.0} != {2009-09-30 17:00:18.0}");
}
@Test

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

@@ -19,7 +19,6 @@ import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef;
import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static com.google.gerrit.testutil.TestChanges.incrementPatchSet;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
import static org.junit.Assert.fail;
@@ -251,7 +250,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(psa2.getAccountId().get()).isEqualTo(1);
assertThat(psa2.getLabel()).isEqualTo("Code-Review");
assertThat(psa2.getValue()).isEqualTo((short) +1);
assertThat(psa2.getGranted()).isEqualTo(truncate(after(c, 3000)));
assertThat(psa2.getGranted()).isEqualTo(truncate(after(c, 4000)));
}
@Test
@@ -873,10 +872,6 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(ts4).isGreaterThan(ts3);
incrementPatchSet(c);
RevCommit commit = tr.commit().message("PS2").create();
update = newUpdate(c, changeOwner);
update.setCommit(rw, commit);
update.commit();
Timestamp ts5 = newNotes(c).getChange().getLastUpdatedOn();
assertThat(ts5).isGreaterThan(ts4);
@@ -983,11 +978,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(ps1.getUploader()).isEqualTo(changeOwner.getAccountId());
// ps2 by other user
incrementPatchSet(c);
RevCommit commit = tr.commit().message("PS2").create();
ChangeUpdate update = newUpdate(c, otherUser);
update.setCommit(rw, commit);
update.commit();
RevCommit commit = incrementPatchSet(c, otherUser);
notes = newNotes(c);
PatchSet ps2 = notes.getCurrentPatchSet();
assertThat(ps2.getId()).isEqualTo(new PatchSet.Id(c.getId(), 2));
@@ -998,10 +989,11 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(ps2.getRevision().get()).isNotEqualTo(ps1.getRevision());
assertThat(ps2.getRevision().get()).isEqualTo(commit.name());
assertThat(ps2.getUploader()).isEqualTo(otherUser.getAccountId());
assertThat(ps2.getCreatedOn()).isEqualTo(update.getWhen());
assertThat(ps2.getCreatedOn())
.isEqualTo(notes.getChange().getLastUpdatedOn());
// comment on ps1, current patch set is still ps2
update = newUpdate(c, changeOwner);
ChangeUpdate update = newUpdate(c, changeOwner);
update.setPatchSetId(ps1.getId());
update.setChangeMessage("Comment on old patch set.");
update.commit();
@@ -1014,8 +1006,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
Change c = newChange();
PatchSet.Id psId1 = c.currentPatchSetId();
// ps2
incrementPatchSet(c);
incrementCurrentPatchSetFieldOnly(c);
PatchSet.Id psId2 = c.currentPatchSetId();
RevCommit commit = tr.commit().message("PS2").create();
ChangeUpdate update = newUpdate(c, changeOwner);
@@ -1074,8 +1065,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(notes.getPatchSets().get(psId1).getGroups())
.containsExactly("a", "b").inOrder();
// ps2
incrementPatchSet(c);
incrementCurrentPatchSetFieldOnly(c);
PatchSet.Id psId2 = c.currentPatchSetId();
update = newUpdate(c, changeOwner);
update.setCommit(rw, tr.commit().message("PS2").create());
@@ -1101,7 +1091,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
// ps2 with push cert
Change c = newChange();
PatchSet.Id psId1 = c.currentPatchSetId();
incrementPatchSet(c);
incrementCurrentPatchSetFieldOnly(c);
PatchSet.Id psId2 = c.currentPatchSetId();
ChangeUpdate update = newUpdate(c, changeOwner);
update.setPatchSetId(psId2);
@@ -1656,6 +1646,9 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
public void patchLineCommentNotesFormatMultiplePatchSetsSameRevId()
throws Exception {
Change c = newChange();
PatchSet.Id psId1 = c.currentPatchSetId();
incrementPatchSet(c);
PatchSet.Id psId2 = c.currentPatchSetId();
String uuid1 = "uuid1";
String uuid2 = "uuid2";
String uuid3 = "uuid3";
@@ -1667,9 +1660,6 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
Timestamp time = TimeUtil.nowTs();
RevId revId = new RevId("abcd1234abcd1234abcd1234abcd1234abcd1234");
PatchSet.Id psId1 = c.currentPatchSetId();
PatchSet.Id psId2 = new PatchSet.Id(c.getId(), psId1.get() + 1);
Comment comment1 =
newComment(psId1, "file1", uuid1, range1, range1.getEndLine(),
otherUser, null, time, message1, (short) 0, revId.get());
@@ -2497,6 +2487,35 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(msg.getRealAuthor()).isEqualTo(changeOwner.getAccountId());
}
@Test
public void ignoreEntitiesBeyondCurrentPatchSet() throws Exception {
Change c = newChange();
ChangeNotes notes = newNotes(c);
int numMessages = notes.getChangeMessages().size();
int numPatchSets = notes.getPatchSets().size();
int numApprovals = notes.getApprovals().size();
int numComments = notes.getComments().size();
ChangeUpdate update = newUpdate(c, changeOwner);
update.setPatchSetId(
new PatchSet.Id(c.getId(), c.currentPatchSetId().get() + 1));
update.setChangeMessage("Should be ignored");
update.putApproval("Code-Review", (short) 2);
CommentRange range = new CommentRange(1, 1, 2, 1);
Comment comment = newComment(update.getPatchSetId(), "filename",
"uuid", range, range.getEndLine(), changeOwner, null,
new Timestamp(update.getWhen().getTime()), "comment", (short) 1,
"abcd1234abcd1234abcd1234abcd1234abcd1234");
update.putComment(Status.PUBLISHED, comment);
update.commit();
notes = newNotes(c);
assertThat(notes.getChangeMessages()).hasSize(numMessages);
assertThat(notes.getPatchSets()).hasSize(numPatchSets);
assertThat(notes.getApprovals()).hasSize(numApprovals);
assertThat(notes.getComments()).hasSize(numComments);
}
private boolean testJson() {
return noteUtil.getWriteJson();
}
@@ -2529,4 +2548,24 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
.isNotNull();
assertThat(cause.getMessage()).isEqualTo(expectedMsg);
}
private void incrementCurrentPatchSetFieldOnly(Change c) {
TestChanges.incrementPatchSet(c);
}
private RevCommit incrementPatchSet(Change c) throws Exception {
return incrementPatchSet(c, userFactory.create(c.getOwner()));
}
private RevCommit incrementPatchSet(Change c, IdentifiedUser user)
throws Exception {
incrementCurrentPatchSetFieldOnly(c);
RevCommit commit = tr.commit()
.message("PS" + c.currentPatchSetId().get())
.create();
ChangeUpdate update = newUpdate(c, user);
update.setCommit(rw, commit);
update.commit();
return tr.parseBody(commit);
}
}