diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java index 3bca5d59f3..81e07b83e1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java @@ -27,7 +27,6 @@ import com.google.common.base.Optional; import com.google.common.base.Splitter; import com.google.common.base.Supplier; import com.google.common.collect.ArrayListMultimap; -import com.google.common.collect.HashMultimap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableSet; @@ -87,9 +86,8 @@ class ChangeNotesParser implements AutoCloseable { private final ObjectId tip; private final RevWalk walk; private final Repository repo; - private final Map> - approvals; - private final Map> removedApprovals; + private final Map>> approvals; private final List allChangeMessages; private final Multimap changeMessagesByPatchSet; @@ -102,7 +100,6 @@ class ChangeNotesParser implements AutoCloseable { this.repo = repoManager.openMetadataRepository(ChangeNotes.getProjectName(change)); approvals = Maps.newHashMap(); - removedApprovals = Maps.newHashMap(); reviewers = Maps.newLinkedHashMap(); allPastReviewers = Lists.newArrayList(); submitRecords = Lists.newArrayListWithExpectedSize(1); @@ -130,9 +127,11 @@ class ChangeNotesParser implements AutoCloseable { buildApprovals() { Multimap result = ArrayListMultimap.create(approvals.keySet().size(), 3); - for (Table curr : approvals.values()) { - for (PatchSetApproval psa : curr.values()) { - result.put(psa.getPatchSetId(), psa); + for (Table> curr : approvals.values()) { + for (Optional psa : curr.values()) { + if (psa.isPresent()) { + result.put(psa.get().getPatchSetId(), psa.get()); + } } } for (Collection v : result.asMap().values()) { @@ -315,92 +314,98 @@ class ChangeNotesParser implements AutoCloseable { } } - private void parseAddApproval(PatchSet.Id psId, Account.Id accountId, + private void parseAddApproval(PatchSet.Id psId, Account.Id committerId, RevCommit commit, String line) throws ConfigInvalidException { + Account.Id accountId; + String labelVoteStr; + int s = line.indexOf(' '); + if (s > 0) { + labelVoteStr = line.substring(0, s); + PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(s + 1)); + checkFooter(ident != null, FOOTER_LABEL, line); + accountId = parseIdent(ident); + } else { + labelVoteStr = line; + accountId = committerId; + } + LabelVote l; try { - l = LabelVote.parseWithEquals(line); + l = LabelVote.parseWithEquals(labelVoteStr); } catch (IllegalArgumentException e) { ConfigInvalidException pe = parseException("invalid %s: %s", FOOTER_LABEL, line); pe.initCause(e); throw pe; } - if (isApprovalRemoved(psId, accountId, l.label())) { - return; - } - Table curr = approvals.get(psId); + Table> curr = + getApprovalsTableIfNoVotePresent(psId, accountId, l.label()); if (curr != null) { - if (curr.contains(accountId, l.label())) { - return; - } - } else { - curr = newApprovalsTable(); - approvals.put(psId, curr); + curr.put(accountId, l.label(), Optional.of(new PatchSetApproval( + new PatchSetApproval.Key( + psId, + accountId, + new LabelId(l.label())), + l.value(), + new Timestamp(commit.getCommitterIdent().getWhen().getTime())))); } - curr.put(accountId, l.label(), new PatchSetApproval( - new PatchSetApproval.Key( - psId, - accountId, - new LabelId(l.label())), - l.value(), - new Timestamp(commit.getCommitterIdent().getWhen().getTime()))); } - private static Table - newApprovalsTable() { - return Tables.newCustomTable( - Maps.> + private void parseRemoveApproval(PatchSet.Id psId, Account.Id committerId, + String line) throws ConfigInvalidException { + Account.Id accountId; + String label; + int s = line.indexOf(' '); + if (s > 0) { + label = line.substring(1, s); + PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(s + 1)); + checkFooter(ident != null, FOOTER_LABEL, line); + accountId = parseIdent(ident); + } else { + label = line.substring(1); + accountId = committerId; + } + + try { + LabelType.checkNameInternal(label); + } catch (IllegalArgumentException e) { + ConfigInvalidException pe = + parseException("invalid %s: %s", FOOTER_LABEL, line); + pe.initCause(e); + throw pe; + } + + Table> curr = + getApprovalsTableIfNoVotePresent(psId, accountId, label); + if (curr != null) { + curr.put(accountId, label, Optional. absent()); + } + } + + private Table> + getApprovalsTableIfNoVotePresent(PatchSet.Id psId, Account.Id accountId, + String label) { + + Table> curr = + approvals.get(psId); + if (curr != null) { + if (curr.contains(accountId, label)) { + return null; + } + } else { + curr = Tables.newCustomTable( + Maps.>> newHashMapWithExpectedSize(2), - new Supplier>() { + new Supplier>>() { @Override - public Map get() { + public Map> get() { return Maps.newLinkedHashMap(); } }); - } - - private void parseRemoveApproval(PatchSet.Id psId, Account.Id accountId, - String line) throws ConfigInvalidException { - Multimap curr = removedApprovals.get(psId); - if (curr == null) { - curr = HashMultimap.create(1, 1); - removedApprovals.put(psId, curr); + approvals.put(psId, curr); } - String label; - Account.Id removedAccountId; - line = line.substring(1); - int s = line.indexOf(' '); - if (s > 0) { - label = line.substring(0, s); - PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(s + 1)); - checkFooter(ident != null, FOOTER_LABEL, line); - removedAccountId = parseIdent(ident); - } else { - label = line; - removedAccountId = accountId; - } - - Table added = approvals.get(psId); - if (added != null && added.contains(accountId, label)) { - return; - } - - try { - curr.put(removedAccountId, LabelType.checkNameInternal(label)); - } catch (IllegalArgumentException e) { - ConfigInvalidException pe = - parseException("invalid %s: %s", FOOTER_LABEL, line); - pe.initCause(e); - throw pe; - } - } - - private boolean isApprovalRemoved(PatchSet.Id psId, Account.Id accountId, - String label) { - Multimap curr = removedApprovals.get(psId); - return curr != null && curr.containsEntry(accountId, label); + return curr; } private void parseSubmitRecords(List lines) 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 f1843bd7ff..f896d89e89 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 @@ -24,13 +24,16 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TOPIC; import static com.google.gerrit.server.notedb.CommentsInNotesUtil.addCommentToMap; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Function; import com.google.common.base.Joiner; +import com.google.common.base.Optional; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import com.google.common.collect.Multimap; -import com.google.common.collect.MultimapBuilder; +import com.google.common.collect.Ordering; +import com.google.common.collect.Table; +import com.google.common.collect.TreeBasedTable; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; @@ -88,9 +91,8 @@ public class ChangeUpdate extends AbstractChangeUpdate { } private final AccountCache accountCache; - private final Map approvals; + private final Table> approvals; private final Map reviewers; - private final Multimap removedApprovals; private Change.Status status; private String subject; private List submitRecords; @@ -163,10 +165,15 @@ public class ChangeUpdate extends AbstractChangeUpdate { this.draftUpdateFactory = draftUpdateFactory; this.accountCache = accountCache; this.commentsUtil = commentsUtil; - this.approvals = Maps.newTreeMap(labelNameComparator); + this.approvals = TreeBasedTable.create( + labelNameComparator, + Ordering.natural().onResultOf(new Function() { + @Override + public Integer apply(Account.Id in) { + return in.get(); + } + })); this.reviewers = Maps.newLinkedHashMap(); - this.removedApprovals = - MultimapBuilder.linkedHashKeys(1).arrayListValues(1).build(); this.comments = Lists.newArrayList(); } @@ -181,7 +188,11 @@ public class ChangeUpdate extends AbstractChangeUpdate { } public void putApproval(String label, short value) { - approvals.put(label, value); + putApprovalFor(getUser().getAccountId(), label, value); + } + + public void putApprovalFor(Account.Id reviewer, String label, short value) { + approvals.put(label, reviewer, Optional.of(value)); } public void removeApproval(String label) { @@ -189,7 +200,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { } public void removeApprovalFor(Account.Id reviewer, String label) { - removedApprovals.put(reviewer, label); + approvals.put(label, reviewer, Optional. absent()); } public void merge(Iterable submitRecords) { @@ -446,14 +457,16 @@ public class ChangeUpdate extends AbstractChangeUpdate { addIdent(msg, e.getKey()).append('\n'); } - for (Map.Entry e : approvals.entrySet()) { - addFooter(msg, FOOTER_LABEL, LabelVote.create( - e.getKey(), e.getValue()).formatWithEquals()); - } - - for (Map.Entry e : removedApprovals.entries()) { - addFooter(msg, FOOTER_LABEL).append('-').append(e.getValue()); - Account.Id id = e.getKey(); + for (Table.Cell> c + : approvals.cellSet()) { + addFooter(msg, FOOTER_LABEL); + if (!c.getValue().isPresent()) { + msg.append('-').append(c.getRowKey()); + } else { + msg.append(LabelVote.create( + c.getRowKey(), c.getValue().get()).formatWithEquals()); + } + Account.Id id = c.getColumnKey(); if (!id.equals(ctl.getUser().getAccountId())) { addIdent(msg.append(' '), id); } @@ -496,7 +509,6 @@ public class ChangeUpdate extends AbstractChangeUpdate { private boolean isEmpty() { return approvals.isEmpty() - && removedApprovals.isEmpty() && changeMessage == null && comments.isEmpty() && reviewers.isEmpty() 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 611054d7c3..a57294dfc6 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 @@ -20,6 +20,7 @@ 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 com.google.common.base.Function; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMultimap; @@ -27,6 +28,7 @@ import com.google.common.collect.ImmutableSetMultimap; import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; +import com.google.common.collect.Ordering; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.reviewdb.client.Account; @@ -228,7 +230,35 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { assertThat(psa.getAccountId()).isEqualTo(otherUserId); assertThat(psa.getLabel()).isEqualTo("Not-For-Long"); assertThat(psa.getValue()).isEqualTo((short) 2); + } + @Test + public void putOtherUsersApprovals() throws Exception { + Change c = newChange(); + ChangeUpdate update = newUpdate(c, changeOwner); + update.putApproval("Code-Review", (short) 1); + update.putApprovalFor(otherUser.getAccountId(), "Code-Review", (short) -1); + update.commit(); + + ChangeNotes notes = newNotes(c); + List approvals = Ordering.natural().onResultOf( + new Function() { + @Override + public Integer apply(PatchSetApproval in) { + return in.getAccountId().get(); + } + }).sortedCopy(notes.getApprovals().get(c.currentPatchSetId())); + assertThat(approvals).hasSize(2); + + assertThat(approvals.get(0).getAccountId()) + .isEqualTo(changeOwner.getAccountId()); + assertThat(approvals.get(0).getLabel()).isEqualTo("Code-Review"); + assertThat(approvals.get(0).getValue()).isEqualTo((short) 1); + + assertThat(approvals.get(1).getAccountId()) + .isEqualTo(otherUser.getAccountId()); + assertThat(approvals.get(1).getLabel()).isEqualTo("Code-Review"); + assertThat(approvals.get(1).getValue()).isEqualTo((short) -1); } @Test