opendev/gerrit
Code Issues Proposed changes

Merge "Sort GetRelated more like the PatchSetAncestor ordering"

Browse Source
This commit is contained in:
Dave Borowitz
2015-10-26 15:42:41 +00:00
committed by Gerrit Code Review
parent 334f95d75e 39d1a499a7
commit b54d38dcbe
4 changed files with 577 additions and 97 deletions
Download Patch File Download Diff File Expand all files Collapse all files

355
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
View File

@@ -20,7 +20,6 @@ import static com.google.gerrit.acceptance.GitUtil.pushHead;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestSession;
import com.google.gerrit.extensions.common.CommitInfo;
@@ -75,38 +74,41 @@ public class GetRelatedIT extends AbstractDaemonTest {
@Test
public void getRelatedLinear() throws Exception {
// 1,1---2,1
RevCommit c1_1 = commitBuilder()
.add("a.txt", "1")
.message("subject: 1")
.create();
String id1 = getChangeId(c1_1);
RevCommit c2_2 = commitBuilder()
RevCommit c2_1 = commitBuilder()
.add("b.txt", "2")
.message("subject: 2")
.create();
String id2 = getChangeId(c2_2);
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps1_1 = getPatchSetId(c1_1);
PatchSet.Id ps2_1 = getPatchSetId(c2_1);
for (RevCommit c : ImmutableList.of(c2_2, c1_1)) {
assertRelated(getPatchSetId(c),
changeAndCommit(id2, c2_2, 1, 1),
changeAndCommit(id1, c1_1, 1, 1));
for (PatchSet.Id ps : ImmutableList.of(ps2_1, ps1_1)) {
assertRelated(ps,
changeAndCommit(ps2_1, c2_1, 1),
changeAndCommit(ps1_1, c1_1, 1));
}
}
@Test
public void getRelatedReorder() throws Exception {
// 1,1---2,1
//
// 2,2---1,2
// Create two commits and push.
RevCommit c1_1 = commitBuilder()
.add("a.txt", "1")
.message("subject: 1")
.create();
String id1 = getChangeId(c1_1);
RevCommit c2_1 = commitBuilder()
.add("b.txt", "2")
.message("subject: 2")
.create();
String id2 = getChangeId(c2_1);
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps1_1 = getPatchSetId(c1_1);
PatchSet.Id ps2_1 = getPatchSetId(c2_1);
@@ -121,31 +123,71 @@ public class GetRelatedIT extends AbstractDaemonTest {
for (PatchSet.Id ps : ImmutableList.of(ps2_2, ps1_2)) {
assertRelated(ps,
changeAndCommit(id1, c1_2, 2, 2),
changeAndCommit(id2, c2_2, 2, 2));
changeAndCommit(ps1_2, c1_2, 2),
changeAndCommit(ps2_2, c2_2, 2));
}
for (PatchSet.Id ps : ImmutableList.of(ps2_1, ps1_1)) {
assertRelated(ps,
changeAndCommit(id2, c2_1, 1, 2),
changeAndCommit(id1, c1_1, 1, 2));
changeAndCommit(ps2_1, c2_1, 2),
changeAndCommit(ps1_1, c1_1, 2));
}
}
@Test
public void getRelatedAmendParentChange() throws Exception {
// 1,1---2,1
//
// 1,2
// Create two commits and push.
RevCommit c1_1 = commitBuilder()
.add("a.txt", "1")
.message("subject: 1")
.create();
RevCommit c2_1 = commitBuilder()
.add("b.txt", "2")
.message("subject: 2")
.create();
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps1_1 = getPatchSetId(c1_1);
PatchSet.Id ps2_1 = getPatchSetId(c2_1);
// Amend parent change and push.
testRepo.reset("HEAD~1");
RevCommit c1_2 = amendBuilder()
.add("c.txt", "2")
.create();
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps1_2 = getPatchSetId(c1_2);
for (PatchSet.Id ps : ImmutableList.of(ps2_1, ps1_1)) {
assertRelated(ps,
changeAndCommit(ps2_1, c2_1, 1),
changeAndCommit(ps1_1, c1_1, 2));
}
assertRelated(ps1_2,
changeAndCommit(ps2_1, c2_1, 1),
changeAndCommit(ps1_2, c1_2, 2));
}
@Test
public void getRelatedReorderAndExtend() throws Exception {
// 1,1---2,1
//
// 2,2---1,2---3,1
// Create two commits and push.
ObjectId initial = repo().getRef("HEAD").getObjectId();
RevCommit c1_1 = commitBuilder()
.add("a.txt", "1")
.message("subject: 1")
.create();
String id1 = getChangeId(c1_1);
RevCommit c2_1 = commitBuilder()
.add("b.txt", "2")
.message("subject: 2")
.create();
String id2 = getChangeId(c2_1);
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps1_1 = getPatchSetId(c1_1);
PatchSet.Id ps2_1 = getPatchSetId(c2_1);
@@ -158,7 +200,6 @@ public class GetRelatedIT extends AbstractDaemonTest {
.add("c.txt", "3")
.message("subject: 3")
.create();
String id3 = getChangeId(c3_1);
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps1_2 = getPatchSetId(c1_1);
PatchSet.Id ps2_2 = getPatchSetId(c2_1);
@@ -166,36 +207,248 @@ public class GetRelatedIT extends AbstractDaemonTest {
for (PatchSet.Id ps : ImmutableList.of(ps3_1, ps2_2, ps1_2)) {
assertRelated(ps,
changeAndCommit(id3, c3_1, 1, 1),
changeAndCommit(id1, c1_2, 2, 2),
changeAndCommit(id2, c2_2, 2, 2));
changeAndCommit(ps3_1, c3_1, 1),
changeAndCommit(ps1_2, c1_2, 2),
changeAndCommit(ps2_2, c2_2, 2));
}
for (PatchSet.Id ps : ImmutableList.of(ps2_1, ps1_1)) {
assertRelated(ps,
changeAndCommit(id3, c3_1, 1, 1),
changeAndCommit(id2, c2_1, 1, 2),
changeAndCommit(id1, c1_1, 1, 2));
changeAndCommit(ps3_1, c3_1, 1),
changeAndCommit(ps2_1, c2_1, 2),
changeAndCommit(ps1_1, c1_1, 2));
}
}
@Test
public void getRelatedReworkSeries() throws Exception {
// 1,1---2,1---3,1
//
// 1,2---2,2---3,2
// Create three commits and push.
RevCommit c1_1 = commitBuilder()
.add("a.txt", "1")
.message("subject: 1")
.create();
RevCommit c2_1 = commitBuilder()
.add("b.txt", "1")
.message("subject: 2")
.create();
RevCommit c3_1 = commitBuilder()
.add("b.txt", "1")
.message("subject: 3")
.create();
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps1_1 = getPatchSetId(c1_1);
PatchSet.Id ps2_1 = getPatchSetId(c2_1);
PatchSet.Id ps3_1 = getPatchSetId(c3_1);
// Amend all changes change and push.
testRepo.reset(c1_1);
RevCommit c1_2 = amendBuilder()
.add("a.txt", "2")
.create();
RevCommit c2_2 = commitBuilder()
.add("b.txt", "2")
.message(parseBody(c2_1).getFullMessage())
.create();
RevCommit c3_2 = commitBuilder()
.add("b.txt", "3")
.message(parseBody(c3_1).getFullMessage())
.create();
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps1_2 = getPatchSetId(c1_2);
PatchSet.Id ps2_2 = getPatchSetId(c2_2);
PatchSet.Id ps3_2 = getPatchSetId(c3_2);
for (PatchSet.Id ps : ImmutableList.of(ps1_1, ps2_1, ps3_1)) {
assertRelated(ps,
changeAndCommit(ps3_1, c3_1, 2),
changeAndCommit(ps2_1, c2_1, 2),
changeAndCommit(ps1_1, c1_1, 2));
}
for (PatchSet.Id ps : ImmutableList.of(ps1_2, ps2_2, ps3_2)) {
assertRelated(ps,
changeAndCommit(ps3_2, c3_2, 2),
changeAndCommit(ps2_2, c2_2, 2),
changeAndCommit(ps1_2, c1_2, 2));
}
}
@Test
public void getRelatedReworkThenExtendInTheMiddleOfSeries() throws Exception {
// 1,1---2,1---3,1
//
// 1,2---2,2---3,2
// \---4,1
// Create three commits and push.
RevCommit c1_1 = commitBuilder()
.add("a.txt", "1")
.message("subject: 1")
.create();
RevCommit c2_1 = commitBuilder()
.add("b.txt", "1")
.message("subject: 2")
.create();
RevCommit c3_1 = commitBuilder()
.add("b.txt", "1")
.message("subject: 3")
.create();
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps1_1 = getPatchSetId(c1_1);
PatchSet.Id ps2_1 = getPatchSetId(c2_1);
PatchSet.Id ps3_1 = getPatchSetId(c3_1);
// Amend all changes change and push.
testRepo.reset(c1_1);
RevCommit c1_2 = amendBuilder()
.add("a.txt", "2")
.create();
RevCommit c2_2 = commitBuilder()
.add("b.txt", "2")
.message(parseBody(c2_1).getFullMessage())
.create();
RevCommit c3_2 = commitBuilder()
.add("b.txt", "3")
.message(parseBody(c3_1).getFullMessage())
.create();
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps1_2 = getPatchSetId(c1_2);
PatchSet.Id ps2_2 = getPatchSetId(c2_2);
PatchSet.Id ps3_2 = getPatchSetId(c3_2);
// Add one more commit 4,1 based on 1,2.
testRepo.reset(c1_2);
RevCommit c4_1 = commitBuilder()
.add("d.txt", "4")
.message("subject: 4")
.create();
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps4_1 = getPatchSetId(c4_1);
// 1,1 is related indirectly to 4,1.
assertRelated(ps1_1,
changeAndCommit(ps4_1, c4_1, 1),
changeAndCommit(ps3_1, c3_1, 2),
changeAndCommit(ps2_1, c2_1, 2),
changeAndCommit(ps1_1, c1_1, 2));
// 2,1 and 3,1 don't include 4,1 since we don't walk forward after walking
// backward.
for (PatchSet.Id ps : ImmutableList.of(ps2_1, ps3_1)) {
assertRelated(ps,
changeAndCommit(ps3_1, c3_1, 2),
changeAndCommit(ps2_1, c2_1, 2),
changeAndCommit(ps1_1, c1_1, 2));
}
// 1,2 is related directly to 4,1, and the 2-3 parallel branch stays intact.
assertRelated(ps1_2,
changeAndCommit(ps3_2, c3_2, 2),
changeAndCommit(ps4_1, c4_1, 1),
changeAndCommit(ps2_2, c2_2, 2),
changeAndCommit(ps1_2, c1_2, 2));
// 4,1 is only related to 1,2, since we don't walk forward after walking
// backward.
assertRelated(ps4_1,
changeAndCommit(ps4_1, c4_1, 1),
changeAndCommit(ps1_2, c1_2, 2));
// 2,2 and 3,2 don't include 4,1 since we don't walk forward after walking
// backward.
for (PatchSet.Id ps : ImmutableList.of(ps2_2, ps3_2)) {
assertRelated(ps,
changeAndCommit(ps3_2, c3_2, 2),
changeAndCommit(ps2_2, c2_2, 2),
changeAndCommit(ps1_2, c1_2, 2));
}
}
@Test
public void getRelatedCrissCrossDependency() throws Exception {
// 1,1---2,1---3,2
//
// 1,2---2,2---3,1
// Create two commits and push.
RevCommit c1_1 = commitBuilder()
.add("a.txt", "1")
.message("subject: 1")
.create();
RevCommit c2_1 = commitBuilder()
.add("b.txt", "2")
.message("subject: 2")
.create();
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps1_1 = getPatchSetId(c1_1);
PatchSet.Id ps2_1 = getPatchSetId(c2_1);
// Amend both changes change and push.
testRepo.reset(c1_1);
RevCommit c1_2 = amendBuilder()
.add("a.txt", "2")
.create();
RevCommit c2_2 = commitBuilder()
.add("b.txt", "2")
.message(parseBody(c2_1).getFullMessage())
.create();
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps1_2 = getPatchSetId(c1_2);
PatchSet.Id ps2_2 = getPatchSetId(c2_2);
// PS 3,1 depends on 2,2.
RevCommit c3_1 = commitBuilder()
.add("c.txt", "1")
.message("subject: 3")
.create();
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps3_1 = getPatchSetId(c3_1);
// PS 3,2 depends on 2,1.
testRepo.reset(c2_1);
RevCommit c3_2 = commitBuilder()
.add("c.txt", "2")
.message(parseBody(c3_1).getFullMessage())
.create();
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps3_2 = getPatchSetId(c3_2);
for (PatchSet.Id ps : ImmutableList.of(ps1_1, ps2_1, ps3_2)) {
assertRelated(ps,
changeAndCommit(ps3_2, c3_2, 2),
changeAndCommit(ps2_1, c2_1, 2),
changeAndCommit(ps1_1, c1_1, 2));
}
for (PatchSet.Id ps : ImmutableList.of(ps1_2, ps2_2, ps3_1)) {
assertRelated(ps,
changeAndCommit(ps3_1, c3_1, 2),
changeAndCommit(ps2_2, c2_2, 2),
changeAndCommit(ps1_2, c1_2, 2));
}
}
@Test
public void getRelatedEdit() throws Exception {
// 1,1---2,1---3,1
// \---2,E---/
RevCommit c1_1 = commitBuilder()
.add("a.txt", "1")
.message("subject: 1")
.create();
String id1 = getChangeId(c1_1);
RevCommit c2_1 = commitBuilder()
.add("b.txt", "2")
.message("subject: 2")
.create();
String id2 = getChangeId(c2_1);
RevCommit c3_1 = commitBuilder()
.add("c.txt", "3")
.message("subject: 3")
.create();
String id3 = getChangeId(c3_1);
pushHead(testRepo, "refs/for/master", false);
Change ch2 = getChange(c2_1).change();
@@ -212,37 +465,38 @@ public class GetRelatedIT extends AbstractDaemonTest {
for (PatchSet.Id ps : ImmutableList.of(ps1_1, ps2_1, ps3_1)) {
assertRelated(ps,
changeAndCommit(id3, c3_1, 1, 1),
changeAndCommit(id2, c2_1, 1, 1),
changeAndCommit(id1, c1_1, 1, 1));
changeAndCommit(ps3_1, c3_1, 1),
changeAndCommit(ps2_1, c2_1, 1),
changeAndCommit(ps1_1, c1_1, 1));
}
assertRelated(ps2_edit,
changeAndCommit(id3, c3_1, 1, 1),
changeAndCommit(id2, editRev, 0, 1),
changeAndCommit(id1, c1_1, 1, 1));
changeAndCommit(ps3_1, c3_1, 1),
changeAndCommit(new PatchSet.Id(ch2.getId(), 0), editRev, 1),
changeAndCommit(ps1_1, c1_1, 1));
}
@Test
public void pushNewPatchSetWhenParentHasNullGroup() throws Exception {
// 1,1---2,1
// \---2,2
RevCommit c1_1 = commitBuilder()
.add("a.txt", "1")
.message("subject: 1")
.create();
String id1 = getChangeId(c1_1);
RevCommit c2_1 = commitBuilder()
.add("b.txt", "2")
.message("subject: 2")
.create();
String id2 = getChangeId(c2_1);
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id psId1_1 = getPatchSetId(c1_1);
PatchSet.Id psId2_1 = getPatchSetId(c2_1);
for (PatchSet.Id psId : ImmutableList.of(psId1_1, psId2_1)) {
assertRelated(psId,
changeAndCommit(id2, c2_1, 1, 1),
changeAndCommit(id1, c1_1, 1, 1));
changeAndCommit(psId2_1, c2_1, 1),
changeAndCommit(psId1_1, c1_1, 1));
}
// Pretend PS1,1 was pushed before the groups field was added.
@@ -266,8 +520,8 @@ public class GetRelatedIT extends AbstractDaemonTest {
// Push updated the group for PS1,1, so it shows up in related changes even
// though a new patch set was not pushed.
assertRelated(psId2_2,
changeAndCommit(id2, c2_2, 2, 2),
changeAndCommit(id1, c1_1, 1, 1));
changeAndCommit(psId2_2, c2_2, 2),
changeAndCommit(psId1_1, c1_1, 1));
}
private List<ChangeAndCommit> getRelated(PatchSet.Id ps) throws IOException {
@@ -282,8 +536,9 @@ public class GetRelatedIT extends AbstractDaemonTest {
RelatedInfo.class).changes;
}
private String getChangeId(RevCommit c) throws Exception {
return GitUtil.getChangeId(testRepo, c).get();
private RevCommit parseBody(RevCommit c) throws IOException {
testRepo.getRevWalk().parseBody(c);
return c;
}
private PatchSet.Id getPatchSetId(ObjectId c) throws OrmException {
@@ -298,13 +553,13 @@ public class GetRelatedIT extends AbstractDaemonTest {
return Iterables.getOnlyElement(queryProvider.get().byCommit(c));
}
private static ChangeAndCommit changeAndCommit(String changeId,
ObjectId commitId, int revisionNum, int currentRevisionNum) {
private static ChangeAndCommit changeAndCommit(
PatchSet.Id psId, ObjectId commitId, int currentRevisionNum) {
ChangeAndCommit result = new ChangeAndCommit();
result.changeId = changeId;
result._changeNumber = psId.getParentKey().get();
result.commit = new CommitInfo();
result.commit.commit = commitId.name();
result._revisionNumber = revisionNum;
result._revisionNumber = psId.get();
result._currentRevisionNumber = currentRevisionNum;
result.status = "NEW";
return result;
@@ -313,18 +568,18 @@ public class GetRelatedIT extends AbstractDaemonTest {
private void assertRelated(PatchSet.Id psId, ChangeAndCommit... expected)
throws Exception {
List<ChangeAndCommit> actual = getRelated(psId);
assertThat(actual).hasSize(expected.length);
assertThat(actual).named("related to " + psId).hasSize(expected.length);
for (int i = 0; i < actual.size(); i++) {
String name = "index " + i + " related to " + psId;
ChangeAndCommit a = actual.get(i);
ChangeAndCommit e = expected[i];
assertThat(a.changeId).named("Change-Id of " + name)
.isEqualTo(e.changeId);
assertThat(a.commit.commit).named("commit of " + name)
.isEqualTo(e.commit.commit);
// Don't bother checking _changeNumber; assume changeId is sufficient.
assertThat(a._changeNumber).named("change ID of " + name)
.isEqualTo(e._changeNumber);
// Don't bother checking changeId; assume _changeNumber is sufficient.
assertThat(a._revisionNumber).named("revision of " + name)
.isEqualTo(e._revisionNumber);
assertThat(a.commit.commit).named("commit of " + name)
.isEqualTo(e.commit.commit);
assertThat(a._currentRevisionNumber).named("current revision of " + name)
.isEqualTo(e._currentRevisionNumber);
}

56
gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java
View File

@@ -24,9 +24,8 @@ import com.google.gerrit.extensions.restapi.RestReadView;
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.ChangeUtil;
import com.google.gerrit.server.CommonConverters;
import com.google.gerrit.server.change.WalkSorter.PatchSetData;
import com.google.gerrit.server.change.PatchSetAncestorSorter.PatchSetData;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.GroupCollector;
import com.google.gerrit.server.index.IndexCollection;
@@ -43,7 +42,6 @@ import org.eclipse.jgit.revwalk.RevCommit;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
@@ -54,7 +52,7 @@ public class GetRelated implements RestReadView<RevisionResource> {
private final Provider<ReviewDb> db;
private final GetRelatedByAncestors byAncestors;
private final Provider<InternalChangeQuery> queryProvider;
private final Provider<WalkSorter> sorter;
private final PatchSetAncestorSorter sorter;
private final IndexCollection indexes;
private final boolean byAncestorsOnly;
@@ -63,7 +61,7 @@ public class GetRelated implements RestReadView<RevisionResource> {
@GerritServerConfig Config cfg,
GetRelatedByAncestors byAncestors,
Provider<InternalChangeQuery> queryProvider,
Provider<WalkSorter> sorter,
PatchSetAncestorSorter sorter,
IndexCollection indexes) {
this.db = db;
this.byAncestors = byAncestors;
@@ -107,16 +105,14 @@ public class GetRelated implements RestReadView<RevisionResource> {
}
List<ChangeAndCommit> result = new ArrayList<>(cds.size());
PatchSet.Id editBaseId = rsrc.getEdit().isPresent()
? rsrc.getEdit().get().getBasePatchSet().getId()
: null;
for (PatchSetData d : sorter.get()
.includePatchSets(choosePatchSets(thisPatchSetGroups, cds))
.setRetainBody(true)
.sort(cds)) {
boolean isEdit = rsrc.getEdit().isPresent();
PatchSet basePs = isEdit
? rsrc.getEdit().get().getBasePatchSet()
: rsrc.getPatchSet();
for (PatchSetData d : sorter.sort(cds, basePs)) {
PatchSet ps = d.patchSet();
RevCommit commit;
if (ps.getId().equals(editBaseId)) {
if (isEdit && ps.getId().equals(basePs.getId())) {
// Replace base of an edit with the edit itself.
ps = rsrc.getPatchSet();
commit = rsrc.getEdit().get().getEditCommit();
@@ -147,40 +143,6 @@ public class GetRelated implements RestReadView<RevisionResource> {
return result;
}
private static Set<PatchSet.Id> choosePatchSets(List<String> groups,
List<ChangeData> cds) throws OrmException {
// Prefer the latest patch set matching at least one group from this
// revision; otherwise, just use the latest patch set overall.
Set<PatchSet.Id> result = new HashSet<>();
for (ChangeData cd : cds) {
Collection<PatchSet> patchSets = cd.patchSets();
List<PatchSet> sameGroup = new ArrayList<>(patchSets.size());
for (PatchSet ps : patchSets) {
if (hasAnyGroup(ps, groups)) {
sameGroup.add(ps);
}
}
result.add(ChangeUtil.PS_ID_ORDER.max(
!sameGroup.isEmpty() ? sameGroup : patchSets).getId());
}
return result;
}
private static boolean hasAnyGroup(PatchSet ps, List<String> groups) {
if (ps.getGroups() == null) {
return false;
}
// Expected size of each list is 1, so nested linear search is fine.
for (String g1 : ps.getGroups()) {
for (String g2 : groups) {
if (g1.equals(g2)) {
return true;
}
}
}
return false;
}
public static class RelatedInfo {
public List<ChangeAndCommit> changes;
}

250
gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetAncestorSorter.java Normal file
View File

@@ -0,0 +1,250 @@
// Copyright (C) 2015 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.change;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import java.io.IOException;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Deque;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
@Singleton
class PatchSetAncestorSorter {
private final GitRepositoryManager repoManager;
@Inject
PatchSetAncestorSorter(GitRepositoryManager repoManager) {
this.repoManager = repoManager;
}
public List<PatchSetData> sort(List<ChangeData> in, PatchSet startPs)
throws OrmException, IOException {
checkArgument(!in.isEmpty(), "Input may not be empty");
// Map of all patch sets, keyed by commit SHA-1.
Map<String, PatchSetData> byId = collectById(in);
PatchSetData start = byId.get(startPs.getRevision().get());
checkArgument(start != null, "%s not found in %s", startPs, in);
ProjectControl ctl = start.data().changeControl().getProjectControl();
// Map of patch set -> immediate parent.
ListMultimap<PatchSetData, PatchSetData> parents =
ArrayListMultimap.create(in.size(), 3);
// Map of patch set -> immediate children.
ListMultimap<PatchSetData, PatchSetData> children =
ArrayListMultimap.create(in.size(), 3);
// All other patch sets of the same change as startPs.
List<PatchSetData> otherPatchSetsOfStart = new ArrayList<>();
for (ChangeData cd : in) {
for (PatchSet ps : cd.patchSets()) {
PatchSetData thisPsd = checkNotNull(byId.get(ps.getRevision().get()));
if (cd.getId().equals(start.id()) && !ps.getId().equals(start.psId())) {
otherPatchSetsOfStart.add(thisPsd);
}
for (RevCommit p : thisPsd.commit().getParents()) {
PatchSetData parentPsd = byId.get(p.name());
if (parentPsd != null) {
parents.put(thisPsd, parentPsd);
children.put(parentPsd, thisPsd);
}
}
}
}
List<PatchSetData> ancestors = walkAncestors(ctl, parents, start);
List<PatchSetData> descendants =
walkDescendants(ctl, children, start, otherPatchSetsOfStart, ancestors);
List<PatchSetData> result =
new ArrayList<>(ancestors.size() + descendants.size() - 1);
result.addAll(Lists.reverse(descendants));
result.addAll(ancestors);
return result;
}
private Map<String, PatchSetData> collectById(List<ChangeData> in)
throws OrmException, IOException {
Project.NameKey project = in.get(0).change().getProject();
Map<String, PatchSetData> result =
Maps.newHashMapWithExpectedSize(in.size() * 3);
try (Repository repo = repoManager.openRepository(project);
RevWalk rw = new RevWalk(repo)) {
rw.setRetainBody(true);
for (ChangeData cd : in) {
checkArgument(cd.change().getProject().equals(project),
"Expected change %s in project %s, found %s",
cd.getId(), project, cd.change().getProject());
for (PatchSet ps : cd.patchSets()) {
String id = ps.getRevision().get();
RevCommit c = rw.parseCommit(ObjectId.fromString(id));
PatchSetData psd = PatchSetData.create(cd, ps, c);
result.put(id, psd);
}
}
}
return result;
}
private static List<PatchSetData> walkAncestors(ProjectControl ctl,
ListMultimap<PatchSetData, PatchSetData> parents, PatchSetData start)
throws OrmException {
List<PatchSetData> result = new ArrayList<>();
Deque<PatchSetData> pending = new ArrayDeque<>();
pending.add(start);
while (!pending.isEmpty()) {
PatchSetData psd = pending.remove();
if (!isVisible(psd, ctl)) {
continue;
}
result.add(psd);
pending.addAll(Lists.reverse(parents.get(psd)));
}
return result;
}
private static List<PatchSetData> walkDescendants(ProjectControl ctl,
ListMultimap<PatchSetData, PatchSetData> children,
PatchSetData start, List<PatchSetData> otherPatchSetsOfStart,
List<PatchSetData> ancestors)
throws OrmException {
Set<Change.Id> alreadyEmittedChanges = new HashSet<>();
addAllChangeIds(alreadyEmittedChanges, ancestors);
// Prefer descendants found by following the original patch set passed in.
List<PatchSetData> result = walkDescendentsImpl(
ctl, alreadyEmittedChanges, children, ImmutableList.of(start));
addAllChangeIds(alreadyEmittedChanges, result);
// Then, go back and add new indirect descendants found by following any
// other patch sets of start. These show up after all direct descendants,
// because we wouldn't know where in the walk to insert them.
result.addAll(walkDescendentsImpl(
ctl, alreadyEmittedChanges, children, otherPatchSetsOfStart));
return result;
}
private static void addAllChangeIds(Collection<Change.Id> changeIds,
Iterable<PatchSetData> psds) {
for (PatchSetData psd : psds) {
changeIds.add(psd.id());
}
}
private static List<PatchSetData> walkDescendentsImpl(ProjectControl ctl,
Set<Change.Id> alreadyEmittedChanges,
ListMultimap<PatchSetData, PatchSetData> children,
List<PatchSetData> start) throws OrmException {
if (start.isEmpty()) {
return ImmutableList.of();
}
Map<Change.Id, PatchSet.Id> maxPatchSetIds = new HashMap<>();
List<PatchSetData> allPatchSets = new ArrayList<>();
Deque<PatchSetData> pending = new ArrayDeque<>();
pending.addAll(start);
while (!pending.isEmpty()) {
PatchSetData psd = pending.remove();
if (!isVisible(psd, ctl)) {
continue;
}
if (!alreadyEmittedChanges.contains(psd.id())) {
// Don't emit anything for changes that were previously emitted, even
// though different patch sets might show up later. However, do
// continue walking through them for the purposes of finding indirect
// descendants.
PatchSet.Id oldMax = maxPatchSetIds.get(psd.id());
if (oldMax == null || psd.psId().get() > oldMax.get()) {
maxPatchSetIds.put(psd.id(), psd.psId());
}
allPatchSets.add(psd);
}
// Breadth-first search with oldest children first.
// TODO(dborowitz): After killing PatchSetAncestors, consider DFS to keep
// parallel history together.
pending.addAll(Lists.reverse(children.get(psd)));
}
// If we saw the same change multiple times, prefer the latest patch set.
List<PatchSetData> result = new ArrayList<>(allPatchSets.size());
for (PatchSetData psd : allPatchSets) {
if (checkNotNull(maxPatchSetIds.get(psd.id())).equals(psd.psId())) {
result.add(psd);
}
}
return result;
}
private static boolean isVisible(PatchSetData psd, ProjectControl ctl)
throws OrmException {
// Reuse existing project control rather than lazily creating a new one for
// each ChangeData.
return ctl.controlFor(psd.data().change())
.isPatchVisible(psd.patchSet(), psd.data());
}
@AutoValue
abstract static class PatchSetData {
@VisibleForTesting
static PatchSetData create(ChangeData cd, PatchSet ps, RevCommit commit) {
return new AutoValue_PatchSetAncestorSorter_PatchSetData(cd, ps, commit);
}
abstract ChangeData data();
abstract PatchSet patchSet();
abstract RevCommit commit();
PatchSet.Id psId() {
return patchSet().getId();
}
Change.Id id() {
return psId().getParentKey();
}
@Override
public int hashCode() {
return Objects.hash(patchSet().getId(), commit());
}
}
}

13
gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
View File

@@ -14,6 +14,8 @@
package com.google.gerrit.server.project;
import static com.google.common.base.Preconditions.checkArgument;
import com.google.common.collect.Lists;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.LabelType;
@@ -176,6 +178,17 @@ public class ChangeControl {
return isVisible(db);
}
/** Can this user see the given patchset? */
public boolean isPatchVisible(PatchSet ps, ChangeData cd)
throws OrmException {
checkArgument(cd.getId().equals(ps.getId().getParentKey()),
"%s not for change %s", ps, cd.getId());
if (ps.isDraft() && !isDraftVisible(cd.db(), cd)) {
return false;
}
return isVisible(cd.db());
}
/** Can this user abandon this change? */
public boolean canAbandon() {
return isOwner() // owner (aka creator) of the change can abandon