opendev/gerrit
Code Issues Proposed changes

ChangeNotesState: Remove changeMessagesByPatchSet field

Browse Source 
The only caller outside of tests that used this field was
Submit#getConflictMessage, which no longer exists.

Rename the remaining field from allChangeMessages to changeMessages,
since there is no longer overlap with another name.

Change-Id: I8f2846d7b21b41a6e0c934fd73ff87f6dc8e58d9
This commit is contained in:
Dave Borowitz
2018-05-02 08:23:27 -04:00
parent cb67893742
commit 17625530b6
6 changed files with 15 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
java/com/google/gerrit/server/ChangeMessagesUtil.java
View File

@@ -116,14 +116,6 @@ public class ChangeMessagesUtil {
return notes.load().getChangeMessages();
}
public Iterable<ChangeMessage> byPatchSet(ReviewDb db, ChangeNotes notes, PatchSet.Id psId)
throws OrmException {
if (!migration.readChanges()) {
return db.changeMessages().byPatchSet(psId);
}
return notes.load().getChangeMessagesByPatchSet().get(psId);
}
public void addChangeMessage(ReviewDb db, ChangeUpdate update, ChangeMessage changeMessage)
throws OrmException {
checkState(

7
java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@@ -564,12 +564,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
/** @return all change messages, in chronological order, oldest first. */
public ImmutableList<ChangeMessage> getChangeMessages() {
return state.allChangeMessages();
}
/** @return change messages by patch set, in chronological order, oldest first. */
public ImmutableListMultimap<PatchSet.Id, ChangeMessage> getChangeMessagesByPatchSet() {
return state.changeMessagesByPatchSet();
return state.changeMessages();
}
/** @return inline comments on each revision. */

5
java/com/google/gerrit/server/notedb/ChangeNotesCache.java
View File

@@ -128,10 +128,7 @@ public class ChangeNotesCache {
+ P
+ list(state.submitRecords(), P + list(2, str(4) + P + K) + P)
+ P
+ list(state.allChangeMessages(), changeMessage())
// Just key overhead for map, already counted messages in previous.
+ P
+ map(state.changeMessagesByPatchSet().asMap(), patchSetId())
+ list(state.changeMessages(), changeMessage())
+ P
+ map(state.publishedComments().asMap(), comment())
+ T // readOnlyUntil

14
java/com/google/gerrit/server/notedb/ChangeNotesParser.java
View File

@@ -45,7 +45,6 @@ import com.google.common.base.Splitter;
import com.google.common.collect.HashBasedTable;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableTable;
import com.google.common.collect.LinkedListMultimap;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
import com.google.common.collect.MultimapBuilder;
@@ -146,7 +145,6 @@ class ChangeNotesParser {
private final Map<ApprovalKey, PatchSetApproval> approvals;
private final List<PatchSetApproval> bufferedApprovals;
private final List<ChangeMessage> allChangeMessages;
private final ListMultimap<PatchSet.Id, ChangeMessage> changeMessagesByPatchSet;
// Non-final private members filled in during the parsing process.
private String branch;
@@ -194,7 +192,6 @@ class ChangeNotesParser {
reviewerUpdates = new ArrayList<>();
submitRecords = Lists.newArrayListWithExpectedSize(1);
allChangeMessages = new ArrayList<>();
changeMessagesByPatchSet = LinkedListMultimap.create();
comments = MultimapBuilder.hashKeys().arrayListValues().build();
patchSets = new HashMap<>();
deletedPatchSets = new HashSet<>();
@@ -265,7 +262,6 @@ class ChangeNotesParser {
buildReviewerUpdates(),
submitRecords,
buildAllMessages(),
buildMessagesByPatchSet(),
comments,
readOnlyUntil,
firstNonNull(isPrivate, false),
@@ -319,13 +315,6 @@ class ChangeNotesParser {
return Lists.reverse(allChangeMessages);
}
private ListMultimap<PatchSet.Id, ChangeMessage> buildMessagesByPatchSet() {
for (Collection<ChangeMessage> v : changeMessagesByPatchSet.asMap().values()) {
Collections.reverse((List<ChangeMessage>) v);
}
return changeMessagesByPatchSet;
}
private void parse(ChangeNotesCommit commit) throws ConfigInvalidException {
Timestamp ts = new Timestamp(commit.getCommitterIdent().getWhen().getTime());
@@ -752,7 +741,6 @@ class ChangeNotesParser {
changeMessage.setMessage(changeMsgString);
changeMessage.setTag(tag);
changeMessage.setRealAuthor(realAccountId);
changeMessagesByPatchSet.put(psId, changeMessage);
allChangeMessages.add(changeMessage);
}
@@ -1089,8 +1077,6 @@ class ChangeNotesParser {
// (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.
changeMessagesByPatchSet.keys().retainAll(patchSets.keySet());
int pruned =
pruneEntitiesForMissingPatchSets(allChangeMessages, ChangeMessage::getPatchSetId, missing);
pruned +=

11
java/com/google/gerrit/server/notedb/ChangeNotesState.java
View File

@@ -74,7 +74,6 @@ public abstract class ChangeNotesState {
ImmutableList.of(),
ImmutableList.of(),
ImmutableListMultimap.of(),
ImmutableListMultimap.of(),
null);
}
@@ -104,8 +103,7 @@ public abstract class ChangeNotesState {
List<Account.Id> allPastReviewers,
List<ReviewerStatusUpdate> reviewerUpdates,
List<SubmitRecord> submitRecords,
List<ChangeMessage> allChangeMessages,
ListMultimap<PatchSet.Id, ChangeMessage> changeMessagesByPatchSet,
List<ChangeMessage> changeMessages,
ListMultimap<RevId, Comment> publishedComments,
@Nullable Timestamp readOnlyUntil,
boolean isPrivate,
@@ -143,8 +141,7 @@ public abstract class ChangeNotesState {
ImmutableList.copyOf(allPastReviewers),
ImmutableList.copyOf(reviewerUpdates),
ImmutableList.copyOf(submitRecords),
ImmutableList.copyOf(allChangeMessages),
ImmutableListMultimap.copyOf(changeMessagesByPatchSet),
ImmutableList.copyOf(changeMessages),
ImmutableListMultimap.copyOf(publishedComments),
readOnlyUntil);
}
@@ -233,9 +230,7 @@ public abstract class ChangeNotesState {
abstract ImmutableList<SubmitRecord> submitRecords();
abstract ImmutableList<ChangeMessage> allChangeMessages();
abstract ImmutableListMultimap<PatchSet.Id, ChangeMessage> changeMessagesByPatchSet();
abstract ImmutableList<ChangeMessage> changeMessages();
abstract ImmutableListMultimap<RevId, Comment> publishedComments();

36
javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
View File

@@ -1078,7 +1078,6 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
ChangeNotes notes = newNotes(c);
assertThat(notes.getPatchSets().keySet()).containsExactly(psId1, psId2);
assertThat(notes.getApprovals()).isNotEmpty();
assertThat(notes.getChangeMessagesByPatchSet()).isNotEmpty();
assertThat(notes.getChangeMessages()).isNotEmpty();
assertThat(notes.getComments()).isNotEmpty();
@@ -1095,7 +1094,6 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
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();
}
@@ -1349,16 +1347,12 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
update.putReviewer(changeOwner.getAccount().getId(), REVIEWER);
update.setChangeMessage("Just a little code change.\n");
update.commit();
PatchSet.Id ps1 = c.currentPatchSetId();
ChangeNotes notes = newNotes(c);
ListMultimap<PatchSet.Id, ChangeMessage> changeMessages = notes.getChangeMessagesByPatchSet();
assertThat(changeMessages.keySet()).containsExactly(ps1);
ChangeMessage cm = Iterables.getOnlyElement(changeMessages.get(ps1));
ChangeMessage cm = Iterables.getOnlyElement(notes.getChangeMessages());
assertThat(cm.getMessage()).isEqualTo("Just a little code change.\n");
assertThat(cm.getAuthor()).isEqualTo(changeOwner.getAccount().getId());
assertThat(cm.getPatchSetId()).isEqualTo(ps1);
assertThat(cm.getPatchSetId()).isEqualTo(c.currentPatchSetId());
}
@Test
@@ -1378,13 +1372,9 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
ChangeUpdate update = newUpdate(c, changeOwner);
update.setChangeMessage("Testing trailing double newline\n\n");
update.commit();
PatchSet.Id ps1 = c.currentPatchSetId();
ChangeNotes notes = newNotes(c);
ListMultimap<PatchSet.Id, ChangeMessage> changeMessages = notes.getChangeMessagesByPatchSet();
assertThat(changeMessages).hasSize(1);
ChangeMessage cm1 = Iterables.getOnlyElement(changeMessages.get(ps1));
ChangeMessage cm1 = Iterables.getOnlyElement(notes.getChangeMessages());
assertThat(cm1.getMessage()).isEqualTo("Testing trailing double newline\n\n");
assertThat(cm1.getAuthor()).isEqualTo(changeOwner.getAccount().getId());
}
@@ -1395,13 +1385,9 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
ChangeUpdate update = newUpdate(c, changeOwner);
update.setChangeMessage("Testing paragraph 1\n\nTesting paragraph 2\n\nTesting paragraph 3");
update.commit();
PatchSet.Id ps1 = c.currentPatchSetId();
ChangeNotes notes = newNotes(c);
ListMultimap<PatchSet.Id, ChangeMessage> changeMessages = notes.getChangeMessagesByPatchSet();
assertThat(changeMessages).hasSize(1);
ChangeMessage cm1 = Iterables.getOnlyElement(changeMessages.get(ps1));
ChangeMessage cm1 = Iterables.getOnlyElement(notes.getChangeMessages());
assertThat(cm1.getMessage())
.isEqualTo(
"Testing paragraph 1\n"
@@ -1429,15 +1415,15 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
PatchSet.Id ps2 = c.currentPatchSetId();
ChangeNotes notes = newNotes(c);
ListMultimap<PatchSet.Id, ChangeMessage> changeMessages = notes.getChangeMessagesByPatchSet();
assertThat(changeMessages).hasSize(2);
assertThat(notes.getChangeMessages()).hasSize(2);
ChangeMessage cm1 = Iterables.getOnlyElement(changeMessages.get(ps1));
ChangeMessage cm1 = notes.getChangeMessages().get(0);
assertThat(cm1.getPatchSetId()).isEqualTo(ps1);
assertThat(cm1.getMessage()).isEqualTo("This is the change message for the first PS.");
assertThat(cm1.getAuthor()).isEqualTo(changeOwner.getAccount().getId());
ChangeMessage cm2 = Iterables.getOnlyElement(changeMessages.get(ps2));
assertThat(cm1.getPatchSetId()).isEqualTo(ps1);
ChangeMessage cm2 = notes.getChangeMessages().get(1);
assertThat(cm2.getPatchSetId()).isEqualTo(ps2);
assertThat(cm2.getMessage()).isEqualTo("This is the change message for the second PS.");
assertThat(cm2.getAuthor()).isEqualTo(changeOwner.getAccount().getId());
assertThat(cm2.getPatchSetId()).isEqualTo(ps2);
@@ -1459,10 +1445,8 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
update.commit();
ChangeNotes notes = newNotes(c);
ListMultimap<PatchSet.Id, ChangeMessage> changeMessages = notes.getChangeMessagesByPatchSet();
assertThat(changeMessages.keySet()).hasSize(1);
List<ChangeMessage> cm = changeMessages.get(ps1);
List<ChangeMessage> cm = notes.getChangeMessages();
assertThat(cm).hasSize(2);
assertThat(cm.get(0).getMessage()).isEqualTo("First change message.\n");
assertThat(cm.get(0).getAuthor()).isEqualTo(changeOwner.getAccount().getId());