Add option to skip diff computations when requesting change details.
When requesting change details, an internal diff request is executed
to fill the two fields 'insertions' and 'deletions' of ChangeInfo.
Most of the time, this request is fast as the result of the underlying
diff computation is already stored in the DiffSummary or PatchList
cache due to previous diff requests (e.g. part of the automatic indexing
after change upload). In some cases (e.g. a very slow diff computation,
flushed caches), a cached result is not available and hence the change
detail request will be delayed until the diff computation finishes.
Interestingly, though, our frontend doesn't even use the two fields
'insertions' and 'deletions' on the change screen. Hence, introduce
an option which allows to skip those two fields and hence the diff
computation. When used on the frontend (adjusted in a separate change),
this will allow change screens to show more details beyond just
"Loading ..." when the result of the diff computation is not available yet.
Change-Id: I7b049a4dcc612e5401d240579f6b37c59de254ea
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index e395abe..d0d77a4 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -318,6 +318,11 @@
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]]
--
diff --git a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
index 47ccb49..010ef6d 100644
--- a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
+++ b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
@@ -248,12 +248,16 @@
* <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. */
diff --git a/java/com/google/gerrit/extensions/client/ListChangesOption.java b/java/com/google/gerrit/extensions/client/ListChangesOption.java
index c842adc..425265b 100644
--- a/java/com/google/gerrit/extensions/client/ListChangesOption.java
+++ b/java/com/google/gerrit/extensions/client/ListChangesOption.java
@@ -75,7 +75,13 @@
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;
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index d4b347b..a3c2e92 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -28,6 +28,7 @@
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 @@
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;
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 5128c3f..c9c139e 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -55,12 +55,16 @@
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.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.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 @@
@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;
@@ -258,6 +281,48 @@
}
@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();
String triplet = project.get() + "~master~" + r.getChangeId();
