opendev/gerrit
Code Issues Proposed changes

Unify inline comment comparators

Browse Source 
Use the same implementation for the PatchLineComment and CommentInfo
versions, which takes into account more fields. Some differences arose
because they were used in different places under different assumptions
about which fields would vary. Since these are now used in a variety
of places, be more general. Move both to the same location, so it's
easier to see side-by-side that the implementation matches. Use
Ordering so callers can use convenience methods where convenient.

Change-Id: Id855ed5b09b38c45b192459551a3d2c1f4c58b43
This commit is contained in:
Dave Borowitz
2015-05-01 12:59:57 -07:00
parent 23fec2b441
commit b0ef2cd435
5 changed files with 50 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files

47
gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java
View File

@@ -14,12 +14,18 @@
package com.google.gerrit.server;
import static com.google.common.base.MoreObjects.firstNonNull;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Optional;
import com.google.common.base.Predicate;
import com.google.common.collect.ComparisonChain;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Ordering;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchLineComment;
@@ -62,10 +68,47 @@ import java.util.Set;
*/
@Singleton
public class PatchLineCommentsUtil {
public static Ordering<PatchLineComment> PLC_ORDER =
new Ordering<PatchLineComment>() {
@Override
public int compare(PatchLineComment c1, PatchLineComment c2) {
String filename1 = c1.getKey().getParentKey().get();
String filename2 = c2.getKey().getParentKey().get();
return ComparisonChain.start()
.compare(filename1, filename2)
.compare(getCommentPsId(c1).get(), getCommentPsId(c2).get())
.compare(c1.getSide(), c2.getSide())
.compare(c1.getLine(), c2.getLine())
.compare(c1.getWrittenOn(), c2.getWrittenOn())
.result();
}
};
public static final Ordering<CommentInfo> COMMENT_INFO_ORDER =
new Ordering<CommentInfo>() {
@Override
public int compare(CommentInfo a, CommentInfo b) {
return ComparisonChain.start()
.compare(a.path, b.path, NULLS_FIRST)
.compare(a.patchSet, b.patchSet, NULLS_FIRST)
.compare(side(a), side(b))
.compare(a.line, b.line, NULLS_FIRST)
.compare(a.id, b.id)
.result();
}
private int side(CommentInfo c) {
return firstNonNull(c.side, Side.REVISION).ordinal();
}
};
public static PatchSet.Id getCommentPsId(PatchLineComment plc) {
return plc.getKey().getParentKey().getParentKey();
}
private static final Ordering<Comparable<?>> NULLS_FIRST =
Ordering.natural().nullsFirst();
private final GitRepositoryManager repoManager;
private final AllUsersName allUsers;
private final DraftCommentNotes.Factory draftFactory;
@@ -219,7 +262,7 @@ public class PatchLineCommentsUtil {
in.getKey().getParentKey().getParentKey().getParentKey();
return changeId.equals(matchId);
}
}).toSortedList(ChangeNotes.PLC_ORDER);
}).toSortedList(PLC_ORDER);
}
List<PatchLineComment> comments = Lists.newArrayList();
comments.addAll(notes.getDraftComments(author).values());
@@ -350,7 +393,7 @@ public class PatchLineCommentsUtil {
}
private static List<PatchLineComment> sort(List<PatchLineComment> comments) {
Collections.sort(comments, ChangeNotes.PLC_ORDER);
Collections.sort(comments, PLC_ORDER);
return comments;
}
}

27
gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java
View File

@@ -14,11 +14,9 @@
package com.google.gerrit.server.change;
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.gerrit.server.PatchLineCommentsUtil.COMMENT_INFO_ORDER;
import com.google.common.base.Strings;
import com.google.common.collect.ComparisonChain;
import com.google.common.collect.Ordering;
import com.google.gerrit.extensions.client.Comment.Range;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.CommentInfo;
@@ -31,7 +29,6 @@ import com.google.inject.Inject;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.List;
import java.util.Map;
import java.util.TreeMap;
@@ -89,7 +86,7 @@ class CommentJson {
}
for (List<CommentInfo> list : out.values()) {
Collections.sort(list, COMPARATOR);
Collections.sort(list, COMMENT_INFO_ORDER);
}
if (accountLoader != null) {
@@ -99,26 +96,6 @@ class CommentJson {
return out;
}
private static final Comparator<CommentInfo> COMPARATOR =
new Comparator<CommentInfo>() {
private final Comparator<Comparable<?>> ORDER =
Ordering.natural().nullsFirst();
@Override
public int compare(CommentInfo a, CommentInfo b) {
return ComparisonChain.start()
.compare(a.patchSet, b.patchSet, ORDER)
.compare(side(a), side(b))
.compare(a.line, b.line, ORDER)
.compare(a.id, b.id)
.result();
}
private int side(CommentInfo c) {
return firstNonNull(c.side, Side.REVISION).ordinal();
}
};
private CommentInfo toCommentInfo(PatchLineComment c, AccountLoader loader) {
CommentInfo r = new CommentInfo();
if (fillPatchSet) {

16
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@@ -18,7 +18,6 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Function;
import com.google.common.collect.ComparisonChain;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSet;
@@ -51,7 +50,6 @@ import org.eclipse.jgit.revwalk.RevWalk;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.Comparator;
import java.util.Map;
/** View of a single {@link Change} based on the log of its notes branch. */
@@ -74,20 +72,6 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
}
});
public static Comparator<PatchLineComment> PLC_ORDER =
new Comparator<PatchLineComment>() {
@Override
public int compare(PatchLineComment c1, PatchLineComment c2) {
String filename1 = c1.getKey().getParentKey().get();
String filename2 = c2.getKey().getParentKey().get();
return ComparisonChain.start()
.compare(filename1, filename2)
.compare(c1.getLine(), c2.getLine())
.compare(c1.getWrittenOn(), c2.getWrittenOn())
.result();
}
};
public static ConfigInvalidException parseException(Change.Id changeId,
String fmt, Object... args) {
return new ConfigInvalidException("Change " + changeId + ": "

3
gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.gerrit.server.PatchLineCommentsUtil.PLC_ORDER;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_HOST;
import static com.google.gerrit.server.notedb.ChangeNotes.parseException;
import static java.nio.charset.StandardCharsets.UTF_8;
@@ -541,7 +542,7 @@ public class CommentsInNotesUtil {
noteMap.remove(commit);
continue;
}
Collections.sort(comments, ChangeNotes.PLC_ORDER);
Collections.sort(comments, PLC_ORDER);
// We allow comments for multiple commits to be written in the same
// update, even though the rest of the metadata update is associated with
// a single patch set.

2
gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java
View File

@@ -371,7 +371,7 @@ public class CommentsTest {
@Test
public void testPatchLineCommentsUtilByCommentStatus() throws OrmException {
assertThat(plcUtil.publishedByChange(db, revRes2.getNotes()))
.containsExactly(plc1, plc2, plc3).inOrder();
.containsExactly(plc3, plc1, plc2).inOrder();
assertThat(plcUtil.draftByChange(db, revRes2.getNotes()))
.containsExactly(plc4, plc5).inOrder();
}