opendev/gerrit
Code Issues Proposed changes

Merge "Add option to skip diff computations when requesting change details."

Browse Source
This commit is contained in:
Alice Kober-Sotzek
2019-07-12 11:43:25 +00:00
committed by Gerrit Code Review
parent c90dc3af6d 3a2aa0199f
commit 044875eb7e
5 changed files with 89 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
Documentation/rest-api-changes.txt
View File

@@ -318,6 +318,11 @@ effect.
A change's mergeability can be requested separately by calling the
link:#get-mergeable[get-mergeable] endpoint.
--
[[skip_diffstat]]
--
* `SKIP_DIFFSTAT`: skip the 'insertions' and 'deletions' field in link:#change-info[ChangeInfo].
For large trees, their computation may be expensive.
--
[[submittable]]
--

6
java/com/google/gerrit/extensions/api/changes/ChangeApi.java
View File

@@ -248,12 +248,16 @@ public interface ChangeApi {
* <ul>
* <li>{@code CHECK} is omitted, to skip consistency checks.
* <li>{@code SKIP_MERGEABLE} is omitted, so the {@code mergeable} bit <em>is</em> set.
* <li>{@code SKIP_DIFFSTAT} is omitted to ensure diffstat calculations.
* </ul>
*/
default ChangeInfo get() throws RestApiException {
return get(
EnumSet.complementOf(
EnumSet.of(ListChangesOption.CHECK, ListChangesOption.SKIP_MERGEABLE)));
EnumSet.of(
ListChangesOption.CHECK,
ListChangesOption.SKIP_MERGEABLE,
ListChangesOption.SKIP_DIFFSTAT)));
}
/** {@link #get(ListChangesOption...)} with no options included. */

8
java/com/google/gerrit/extensions/client/ListChangesOption.java
View File

@@ -75,7 +75,13 @@ public enum ListChangesOption implements ListOption {
TRACKING_IDS(21),
/** Skip mergeability data */
SKIP_MERGEABLE(22);
SKIP_MERGEABLE(22),
/**
* Skip diffstat computation that compute the insertions field (number of lines inserted) and
* deletions field (number of lines deleted)
*/
SKIP_DIFFSTAT(23);
private final int value;

11
java/com/google/gerrit/server/change/ChangeJson.java
View File

@@ -28,6 +28,7 @@ import static com.google.gerrit.extensions.client.ListChangesOption.LABELS;
import static com.google.gerrit.extensions.client.ListChangesOption.MESSAGES;
import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWED;
import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWER_UPDATES;
import static com.google.gerrit.extensions.client.ListChangesOption.SKIP_DIFFSTAT;
import static com.google.gerrit.extensions.client.ListChangesOption.SKIP_MERGEABLE;
import static com.google.gerrit.extensions.client.ListChangesOption.SUBMITTABLE;
import static com.google.gerrit.extensions.client.ListChangesOption.TRACKING_IDS;
@@ -516,10 +517,12 @@ public class ChangeJson {
out.submittable = submittable(cd);
}
}
Optional<ChangedLines> changedLines = cd.changedLines();
if (changedLines.isPresent()) {
out.insertions = changedLines.get().insertions;
out.deletions = changedLines.get().deletions;
if (!has(SKIP_DIFFSTAT)) {
Optional<ChangedLines> changedLines = cd.changedLines();
if (changedLines.isPresent()) {
out.insertions = changedLines.get().insertions;
out.deletions = changedLines.get().deletions;
}
}
out.isPrivate = in.isPrivate() ? true : null;
out.workInProgress = in.isWorkInProgress() ? true : null;

65
javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@@ -55,12 +55,16 @@ import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS
import static com.google.gerrit.server.project.testing.TestLabels.label;
import static com.google.gerrit.server.project.testing.TestLabels.value;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static com.google.gerrit.truth.CacheStatsSubject.assertThat;
import static com.google.gerrit.truth.CacheStatsSubject.cloneStats;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.concurrent.TimeUnit.SECONDS;
import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toSet;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheStats;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
@@ -156,6 +160,12 @@ import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.index.change.ChangeIndex;
import com.google.gerrit.server.index.change.ChangeIndexCollection;
import com.google.gerrit.server.index.change.IndexedChangeQuery;
import com.google.gerrit.server.patch.DiffSummary;
import com.google.gerrit.server.patch.DiffSummaryKey;
import com.google.gerrit.server.patch.IntraLineDiff;
import com.google.gerrit.server.patch.IntraLineDiffKey;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListKey;
import com.google.gerrit.server.project.testing.TestLabels;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeQueryBuilder.ChangeOperatorFactory;
@@ -168,6 +178,7 @@ import com.google.gerrit.testing.FakeEmailSender.Message;
import com.google.gerrit.testing.TestTimeUtil;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
import com.google.inject.name.Named;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.ArrayList;
@@ -207,6 +218,18 @@ public class ChangeIT extends AbstractDaemonTest {
@Inject private ProjectOperations projectOperations;
@Inject private RequestScopeOperations requestScopeOperations;
@Inject
@Named("diff")
private Cache<PatchListKey, PatchList> fileCache;
@Inject
@Named("diff_intraline")
private Cache<IntraLineDiffKey, IntraLineDiff> intraCache;
@Inject
@Named("diff_summary")
private Cache<DiffSummaryKey, DiffSummary> diffSummaryCache;
private ChangeIndexedCounter changeIndexedCounter;
private RegistrationHandle changeIndexedCounterHandle;
@@ -257,6 +280,48 @@ public class ChangeIT extends AbstractDaemonTest {
assertThat(c.owner.avatars).isNull();
}
@Test
public void diffStatShouldComputeInsertionsAndDeletions() throws Exception {
String fileName = "a_new_file.txt";
String fileContent = "First line\nSecond line\n";
PushOneCommit.Result result = createChange("Add a file", fileName, fileContent);
String triplet = project.get() + "~master~" + result.getChangeId();
ChangeInfo change = gApi.changes().id(triplet).get();
assertThat(change.insertions).isNotNull();
assertThat(change.deletions).isNotNull();
}
@Test
public void diffStatShouldSkipInsertionsAndDeletions() throws Exception {
String fileName = "a_new_file.txt";
String fileContent = "First line\nSecond line\n";
PushOneCommit.Result result = createChange("Add a file", fileName, fileContent);
String triplet = project.get() + "~master~" + result.getChangeId();
ChangeInfo change =
gApi.changes().id(triplet).get(ImmutableList.of(ListChangesOption.SKIP_DIFFSTAT));
assertThat(change.insertions).isNull();
assertThat(change.deletions).isNull();
}
@Test
public void skipDiffstatOptionAvoidsAllDiffComputations() throws Exception {
String fileName = "a_new_file.txt";
String fileContent = "First line\nSecond line\n";
PushOneCommit.Result result = createChange("Add a file", fileName, fileContent);
String triplet = project.get() + "~master~" + result.getChangeId();
CacheStats startPatch = cloneStats(fileCache.stats());
CacheStats startIntra = cloneStats(intraCache.stats());
CacheStats startSummary = cloneStats(diffSummaryCache.stats());
gApi.changes().id(triplet).get(ImmutableList.of(ListChangesOption.SKIP_DIFFSTAT));
assertThat(fileCache.stats()).since(startPatch).hasMissCount(0);
assertThat(fileCache.stats()).since(startPatch).hasHitCount(0);
assertThat(intraCache.stats()).since(startIntra).hasMissCount(0);
assertThat(intraCache.stats()).since(startIntra).hasHitCount(0);
assertThat(diffSummaryCache.stats()).since(startSummary).hasMissCount(0);
assertThat(diffSummaryCache.stats()).since(startSummary).hasHitCount(0);
}
@Test
public void skipMergeable() throws Exception {
PushOneCommit.Result r = createChange();