PatchScriptFactory: Remove usage of old diff cache
Switching DiffOperations to be the default for getting file diffs.
Bug: Google b/200147261
Change-Id: Ifb1fa558735c0b47cbd1f8097bc58c2c56f9908e
diff --git a/java/com/google/gerrit/server/patch/PatchScriptFactory.java b/java/com/google/gerrit/server/patch/PatchScriptFactory.java
index 6fa14e1..96b23c8 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptFactory.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptFactory.java
@@ -26,21 +26,13 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
-import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.metrics.Counter1;
-import com.google.gerrit.metrics.Description;
-import com.google.gerrit.metrics.Field;
-import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.PatchSetUtil;
-import com.google.gerrit.server.change.FileInfoJsonExperimentImpl;
import com.google.gerrit.server.edit.ChangeEdit;
import com.google.gerrit.server.edit.ChangeEditUtil;
-import com.google.gerrit.server.experiments.ExperimentFeatures;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.LargeObjectException;
-import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.PatchScriptBuilder.IntraLineDiffCalculatorResult;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
@@ -51,9 +43,7 @@
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
-import com.google.inject.Inject;
import com.google.inject.Provider;
-import com.google.inject.Singleton;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.io.IOException;
@@ -88,27 +78,6 @@
CurrentUser currentUser);
}
- /** These metrics are temporary for launching the new redesigned diff cache. */
- @Singleton
- static class Metrics {
- final Counter1<String> diffs;
- static final String MATCH = "match";
- static final String MISMATCH = "mismatch";
- static final String ERROR = "error";
-
- @Inject
- Metrics(MetricMaker metricMaker) {
- diffs =
- metricMaker.newCounter(
- "diff/get_diff/dark_launch",
- new Description(
- "Total number of matching, non-matching, or error in diffs in the old and new diff cache implementations.")
- .setRate()
- .setUnit("count"),
- Field.ofString("type", Metadata.Builder::eventType).build());
- }
- }
-
private final GitRepositoryManager repoManager;
private final PatchSetUtil psUtil;
private final Provider<PatchScriptBuilder> builderFactory;
@@ -130,8 +99,6 @@
private ChangeNotes notes;
- private final boolean runNewDiffCache;
-
@AssistedInject
PatchScriptFactory(
GitRepositoryManager grm,
@@ -139,7 +106,6 @@
Provider<PatchScriptBuilder> builderFactory,
PatchListCache patchListCache,
ChangeEditUtil editReader,
- ExperimentFeatures experimentFeatures,
PermissionBackend permissionBackend,
ProjectCache projectCache,
DiffOperations diffOperations,
@@ -165,10 +131,6 @@
this.psb = patchSetB;
this.diffPrefs = diffPrefs;
this.currentUser = currentUser;
-
- this.runNewDiffCache =
- experimentFeatures.isFeatureEnabled(FileInfoJsonExperimentImpl.NEW_DIFF_CACHE_FEATURE);
-
changeId = patchSetB.changeId();
}
@@ -179,7 +141,6 @@
Provider<PatchScriptBuilder> builderFactory,
PatchListCache patchListCache,
ChangeEditUtil editReader,
- ExperimentFeatures experimentFeatures,
PermissionBackend permissionBackend,
ProjectCache projectCache,
DiffOperations diffOperations,
@@ -205,10 +166,6 @@
this.psb = patchSetB;
this.diffPrefs = diffPrefs;
this.currentUser = currentUser;
-
- this.runNewDiffCache =
- experimentFeatures.isFeatureEnabled(FileInfoJsonExperimentImpl.NEW_DIFF_CACHE_FEATURE);
-
changeId = patchSetB.changeId();
checkArgument(parentNum > 0, "parentNum must be > 0");
}
@@ -246,11 +203,7 @@
}
bId = edit.get().getEditCommit();
}
- return runNewDiffCache
- ? getPatchScriptWithNewDiffCache(git, aId, bId)
- : getPatchScriptWithOldDiffCache(git, aId, bId);
- } catch (PatchListNotAvailableException e) {
- throw new NoSuchChangeException(changeId, e);
+ return getPatchScript(git, aId, bId);
} catch (DiffNotAvailableException e) {
throw new StorageException(e);
} catch (IOException e) {
@@ -268,15 +221,7 @@
}
}
- private PatchScript getPatchScriptWithOldDiffCache(Repository git, ObjectId aId, ObjectId bId)
- throws IOException, PatchListNotAvailableException {
- PatchScriptBuilder patchScriptBuilder = newBuilder();
- PatchList list = listFor(keyFor(aId, bId, diffPrefs.ignoreWhitespace));
- PatchListEntry content = list.get(fileName);
- return patchScriptBuilder.toPatchScriptOld(git, list, content);
- }
-
- private PatchScript getPatchScriptWithNewDiffCache(Repository git, ObjectId aId, ObjectId bId)
+ private PatchScript getPatchScript(Repository git, ObjectId aId, ObjectId bId)
throws IOException, DiffNotAvailableException {
FileDiffOutput fileDiffOutput =
aId == null
@@ -304,17 +249,6 @@
return Optional.of(getCommitId(psb));
}
- private PatchListKey keyFor(ObjectId aId, ObjectId bId, Whitespace whitespace) {
- if (parentNum == 0) {
- return PatchListKey.againstCommit(aId, bId, whitespace);
- }
- return PatchListKey.againstParentNum(parentNum, bId, whitespace);
- }
-
- private PatchList listFor(PatchListKey key) throws PatchListNotAvailableException {
- return patchListCache.get(key, notes.getProjectName());
- }
-
private PatchScriptBuilder newBuilder() {
final PatchScriptBuilder b = builderFactory.get();
b.setDiffPrefs(diffPrefs);
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
index eadd259..ba35096 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
@@ -2865,13 +2865,11 @@
DiffInfo diffInfo = gApi.changes().id(cId).current().file(symlink).diff(initialRev);
assertThat(diffInfo.content).hasSize(1);
assertThat(diffInfo).content().element(0).linesOfB().containsExactly("new content");
- // TODO(ghareeb): remove conditional assertion when new diff cache is fully rolled out.
- assertThat(diffInfo.changeType)
- .isEqualTo(useNewDiffCache ? ChangeType.REWRITE : ChangeType.ADDED);
+ assertThat(diffInfo.changeType).isEqualTo(ChangeType.REWRITE);
}
@Test
- public void renameDeleteByJgit_isIdentifiedAsRewritten6() throws Exception {
+ public void renameDeleteByJgit_isIdentifiedAsRewritten() throws Exception {
String target = "file.txt";
String symlink = "link.lnk";
PushOneCommit push =
@@ -2908,9 +2906,7 @@
"rename to link.lnk");
assertThat(diffInfo.content).hasSize(1);
assertThat(diffInfo).content().element(0).commonLines().containsExactly("content");
- // TODO(ghareeb): remove conditional assertion when new diff cache is fully rolled out.
- assertThat(diffInfo.changeType)
- .isEqualTo(useNewDiffCache ? ChangeType.REWRITE : ChangeType.RENAMED);
+ assertThat(diffInfo.changeType).isEqualTo(ChangeType.REWRITE);
}
@Test
