Add a "groups" field to PatchSet
This field is intended to be used as an explicit replacement for the current "Related Changes" heuristic, which may involve walking arbitrary amounts of history and making lots of database lookups in the case of some merge topologies. With this field in the index, we can implement GetRelated using a single secondary index lookup once we know the patch set's group identifiers. Combined with the stored PatchSet field, this gives us the entire set of commits that need to be considered for rendering the Related Changes tab, and the only remaining work is to do a simple walk to determine the relative topo ordering. Any idea of automatically grouping changes together requires some amount of heuristics, so we use opaque string values in this field in case we decide to change heuristics later. If we change heuristics, old groups will still be valid as long as the relevant changes stay the same. (We might also decide to add a way to manually group changes together; today, this use case is largely handled by other features such as topics, but this implementation does not rule that out.) The field is actually multi-valued, as the heuristics we implemented in GroupCollector multiple groups if it is the merge commit of two distinct branches of open changes. However, since we don't need to look up changes in the database by group, it's not worth the effort of an extra SQL table, so we just concatenate groups together. Another useful feature of this implementation is that group information is preserved when changes are closed, so submitting a change will no longer immediately cause the Related Changes tab to disappear. Use GroupCollector to assign groups in both the main ReceiveCommits case and the schema upgrade (for all open changes). See the documentation of GroupCollector for how this is intended to work. For code paths other than ReceiveCommits, the rules are much simpler, because there is only one change involved in the operation. Depending on the context, we simply either copy the existing group of the latest patch set, or we create a new group. Change-Id: I7cef275772882b045be14fd9bbbe7c52c73da2a8
This commit is contained in:
@@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkState;
|
||||
import static org.eclipse.jgit.revwalk.RevFlag.UNINTERESTING;
|
||||
|
||||
import com.google.common.annotations.VisibleForTesting;
|
||||
import com.google.common.base.Function;
|
||||
import com.google.common.collect.ArrayListMultimap;
|
||||
import com.google.common.collect.HashMultimap;
|
||||
import com.google.common.collect.ImmutableList;
|
||||
@@ -26,12 +27,17 @@ import com.google.common.collect.Iterables;
|
||||
import com.google.common.collect.ListMultimap;
|
||||
import com.google.common.collect.Multimap;
|
||||
import com.google.common.collect.MultimapBuilder;
|
||||
import com.google.common.collect.Multimaps;
|
||||
import com.google.common.collect.SetMultimap;
|
||||
import com.google.common.collect.Sets;
|
||||
import com.google.gerrit.reviewdb.client.Change;
|
||||
import com.google.gerrit.reviewdb.client.PatchSet;
|
||||
import com.google.gerrit.reviewdb.server.ReviewDb;
|
||||
import com.google.gerrit.server.change.RevisionResource;
|
||||
import com.google.gwtorm.server.OrmException;
|
||||
|
||||
import org.eclipse.jgit.lib.ObjectId;
|
||||
import org.eclipse.jgit.lib.Ref;
|
||||
import org.eclipse.jgit.revwalk.RevCommit;
|
||||
import org.slf4j.Logger;
|
||||
import org.slf4j.LoggerFactory;
|
||||
@@ -76,6 +82,25 @@ public class GroupCollector {
|
||||
private static final Logger log =
|
||||
LoggerFactory.getLogger(GroupCollector.class);
|
||||
|
||||
public static List<String> getCurrentGroups(ReviewDb db, Change c)
|
||||
throws OrmException {
|
||||
PatchSet ps = db.patchSets().get(c.currentPatchSetId());
|
||||
return ps != null ? ps.getGroups() : null;
|
||||
}
|
||||
|
||||
public static List<String> getDefaultGroups(PatchSet ps) {
|
||||
return ImmutableList.of(ps.getRevision().get());
|
||||
}
|
||||
|
||||
public static List<String> getGroups(RevisionResource rsrc) {
|
||||
if (rsrc.getEdit().isPresent()) {
|
||||
// Groups for an edit are just the base revision's groups, since they have
|
||||
// the same parent.
|
||||
return rsrc.getEdit().get().getBasePatchSet().getGroups();
|
||||
}
|
||||
return rsrc.getPatchSet().getGroups();
|
||||
}
|
||||
|
||||
private static interface Lookup {
|
||||
List<String> lookup(PatchSet.Id psId) throws OrmException;
|
||||
}
|
||||
@@ -96,6 +121,27 @@ public class GroupCollector {
|
||||
groupAliases = HashMultimap.create();
|
||||
}
|
||||
|
||||
public GroupCollector(
|
||||
Multimap<ObjectId, Ref> changeRefsById,
|
||||
final ReviewDb db) {
|
||||
this(
|
||||
Multimaps.transformValues(
|
||||
changeRefsById,
|
||||
new Function<Ref, PatchSet.Id>() {
|
||||
@Override
|
||||
public PatchSet.Id apply(Ref in) {
|
||||
return PatchSet.Id.fromRef(in.getName());
|
||||
}
|
||||
}),
|
||||
new Lookup() {
|
||||
@Override
|
||||
public List<String> lookup(PatchSet.Id psId) throws OrmException {
|
||||
PatchSet ps = db.patchSets().get(psId);
|
||||
return ps != null ? ps.getGroups() : null;
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
@VisibleForTesting
|
||||
GroupCollector(
|
||||
Multimap<ObjectId, PatchSet.Id> patchSetsBySha,
|
||||
@@ -111,7 +157,7 @@ public class GroupCollector {
|
||||
});
|
||||
}
|
||||
|
||||
void visit(RevCommit c) {
|
||||
public void visit(RevCommit c) {
|
||||
checkState(!done, "visit() called after getGroups()");
|
||||
Set<RevCommit> interestingParents = getInterestingParents(c);
|
||||
|
||||
@@ -171,7 +217,7 @@ public class GroupCollector {
|
||||
}
|
||||
}
|
||||
|
||||
SetMultimap<ObjectId, String> getGroups() throws OrmException {
|
||||
public SetMultimap<ObjectId, String> getGroups() throws OrmException {
|
||||
done = true;
|
||||
SetMultimap<ObjectId, String> result = MultimapBuilder
|
||||
.hashKeys(groups.keySet().size())
|
||||
|
||||
Reference in New Issue
Block a user