diff --git a/java/com/google/gerrit/server/change/FileInfoJsonModule.java b/java/com/google/gerrit/server/change/FileInfoJsonModule.java index 11d60d1f74..de116bbaa9 100644 --- a/java/com/google/gerrit/server/change/FileInfoJsonModule.java +++ b/java/com/google/gerrit/server/change/FileInfoJsonModule.java @@ -19,13 +19,15 @@ import com.google.inject.AbstractModule; import org.eclipse.jgit.lib.Config; public class FileInfoJsonModule extends AbstractModule { + /** Use the new diff cache implementation {@link FileInfoJsonNewImpl}. */ private final boolean useNewDiffCache; /** Used to dark launch the new diff cache with the list files endpoint. */ private final boolean runNewDiffCacheAsync; public FileInfoJsonModule(@GerritServerConfig Config cfg) { - this.useNewDiffCache = cfg.getBoolean("cache", "diff_cache", "useNewDiffCache", false); + this.useNewDiffCache = + cfg.getBoolean("cache", "diff_cache", "runNewDiffCache_ListFiles", false); this.runNewDiffCacheAsync = cfg.getBoolean("cache", "diff_cache", "runNewDiffCacheAsync_listFiles", false); } diff --git a/java/com/google/gerrit/server/patch/PatchScriptFactory.java b/java/com/google/gerrit/server/patch/PatchScriptFactory.java index 91dc6e3510..07b01c4563 100644 --- a/java/com/google/gerrit/server/patch/PatchScriptFactory.java +++ b/java/com/google/gerrit/server/patch/PatchScriptFactory.java @@ -82,8 +82,7 @@ public class PatchScriptFactory implements Callable { @Assisted("patchSetA") PatchSet.Id patchSetA, @Assisted("patchSetB") PatchSet.Id patchSetB, DiffPreferencesInfo diffPrefs, - CurrentUser currentUser, - boolean useNewDiffCache); + CurrentUser currentUser); PatchScriptFactory create( ChangeNotes notes, @@ -91,8 +90,7 @@ public class PatchScriptFactory implements Callable { int parentNum, PatchSet.Id patchSetB, DiffPreferencesInfo diffPrefs, - CurrentUser currentUser, - boolean useNewDiffCache); + CurrentUser currentUser); } /** These metrics are temporary for launching the new redesigned diff cache. */ @@ -139,10 +137,7 @@ public class PatchScriptFactory implements Callable { private ChangeNotes notes; - private final boolean runNewDiffCacheAsync; - - // TODO(ghareeb): temporary field used for testing. Please remove. - private final boolean useNewDiffCache; + private final boolean runNewDiffCache; @AssistedInject PatchScriptFactory( @@ -162,8 +157,7 @@ public class PatchScriptFactory implements Callable { @Assisted("patchSetA") @Nullable PatchSet.Id patchSetA, @Assisted("patchSetB") PatchSet.Id patchSetB, @Assisted DiffPreferencesInfo diffPrefs, - @Assisted CurrentUser currentUser, - @Assisted boolean useNewDiffCache) { + @Assisted CurrentUser currentUser) { this.repoManager = grm; this.psUtil = psUtil; this.builderFactory = builderFactory; @@ -183,9 +177,7 @@ public class PatchScriptFactory implements Callable { this.diffPrefs = diffPrefs; this.currentUser = currentUser; - this.runNewDiffCacheAsync = - cfg.getBoolean("cache", "diff_cache", "runNewDiffCacheAsync_getDiff", false); - this.useNewDiffCache = useNewDiffCache; + this.runNewDiffCache = cfg.getBoolean("cache", "diff_cache", "runNewDiffCache_GetDiff", false); changeId = patchSetB.changeId(); } @@ -208,8 +200,7 @@ public class PatchScriptFactory implements Callable { @Assisted int parentNum, @Assisted PatchSet.Id patchSetB, @Assisted DiffPreferencesInfo diffPrefs, - @Assisted CurrentUser currentUser, - @Assisted boolean useNewDiffCache) { + @Assisted CurrentUser currentUser) { this.repoManager = grm; this.psUtil = psUtil; this.builderFactory = builderFactory; @@ -229,9 +220,7 @@ public class PatchScriptFactory implements Callable { this.diffPrefs = diffPrefs; this.currentUser = currentUser; - this.runNewDiffCacheAsync = - cfg.getBoolean("cache", "diff_cache", "runNewDiffCacheAsync_getDiff", false); - this.useNewDiffCache = useNewDiffCache; + this.runNewDiffCache = cfg.getBoolean("cache", "diff_cache", "runNewDiffCache_GetDiff", false); changeId = patchSetB.changeId(); checkArgument(parentNum >= 0, "parentNum must be >= 0"); @@ -270,28 +259,15 @@ public class PatchScriptFactory implements Callable { } bId = edit.get().getEditCommit(); } - - if (useNewDiffCache) { - FileDiffOutput fileDiffOutput = - aId == null - ? diffOperations.getModifiedFileAgainstParent( - notes.getProjectName(), - bId, - parentNum == -1 ? null : parentNum + 1, - fileName, - diffPrefs.ignoreWhitespace) - : diffOperations.getModifiedFile( - notes.getProjectName(), aId, bId, fileName, diffPrefs.ignoreWhitespace); - return newBuilder().toPatchScriptNew(git, fileDiffOutput); + if (runNewDiffCache) { + PatchScript patchScript = getPatchScriptWithNewDiffCache(git, aId, bId); + // TODO(ghareeb): remove the async run. This is temporarily used to keep sanity checking + // the results while rolling out the new diff cache. + runOldDiffCacheAsyncAndExportMetrics(git, aId, bId, patchScript); + return patchScript; + } else { + return getPatchScriptWithOldDiffCache(git, aId, bId); } - PatchScriptBuilder patchScriptBuilder = newBuilder(); - PatchList list = listFor(keyFor(aId, bId, diffPrefs.ignoreWhitespace)); - PatchListEntry content = list.get(fileName); - PatchScript patchScript = patchScriptBuilder.toPatchScriptOld(git, list, content); - if (runNewDiffCacheAsync) { - runNewDiffCacheAsyncAndExportMetrics(git, aId, bId, patchScript); - } - return patchScript; } catch (PatchListNotAvailableException e) { throw new NoSuchChangeException(changeId, e); } catch (DiffNotAvailableException e) { @@ -311,24 +287,14 @@ public class PatchScriptFactory implements Callable { } } - private void runNewDiffCacheAsyncAndExportMetrics( + private void runOldDiffCacheAsyncAndExportMetrics( Repository git, ObjectId aId, ObjectId bId, PatchScript expected) { @SuppressWarnings("unused") Future possiblyIgnoredError = executor.submit( () -> { try { - FileDiffOutput fileDiffOutput = - aId == null - ? diffOperations.getModifiedFileAgainstParent( - notes.getProjectName(), - bId, - parentNum == -1 ? null : parentNum + 1, - fileName, - diffPrefs.ignoreWhitespace) - : diffOperations.getModifiedFile( - notes.getProjectName(), aId, bId, fileName, diffPrefs.ignoreWhitespace); - PatchScript patchScript = newBuilder().toPatchScriptNew(git, fileDiffOutput); + PatchScript patchScript = getPatchScriptWithOldDiffCache(git, aId, bId); if (areEqualPatchscripts(patchScript, expected)) { metrics.diffs.increment(metrics.MATCH); } else { @@ -337,7 +303,7 @@ public class PatchScriptFactory implements Callable { "Mismatching diff for change %s, old commit ID: %s, new commit ID: %s, file name: %s.", changeId.toString(), aId, bId, fileName); } - } catch (DiffNotAvailableException | IOException e) { + } catch (PatchListNotAvailableException | IOException e) { metrics.diffs.increment(metrics.ERROR); logger.atSevere().atMostEvery(10, TimeUnit.SECONDS).log( String.format( @@ -348,6 +314,29 @@ public class PatchScriptFactory implements Callable { }); } + 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) + throws IOException, DiffNotAvailableException { + FileDiffOutput fileDiffOutput = + aId == null + ? diffOperations.getModifiedFileAgainstParent( + notes.getProjectName(), + bId, + parentNum == -1 ? null : parentNum + 1, + fileName, + diffPrefs.ignoreWhitespace) + : diffOperations.getModifiedFile( + notes.getProjectName(), aId, bId, fileName, diffPrefs.ignoreWhitespace); + return newBuilder().toPatchScriptNew(git, fileDiffOutput); + } + /** * The comparison is not exhaustive but is using the most important fields. Comparing all fields * will require some work in {@link PatchScript} to, e.g., convert it to autovalue. This diff --git a/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java b/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java index 24c03c7547..18d532b32b 100644 --- a/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java +++ b/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java @@ -163,8 +163,7 @@ public class SubmitWithStickyApprovalDiff { latestApprovedPatchsetId, currentPatchsetId, diffPreferencesInfo, - currentUser, - /* useNewDiffCache= */ false); + currentUser); PatchScript patchScript = null; try { patchScript = patchScriptFactory.call(); diff --git a/java/com/google/gerrit/server/restapi/change/GetDiff.java b/java/com/google/gerrit/server/restapi/change/GetDiff.java index af1236e353..b8902b78ab 100644 --- a/java/com/google/gerrit/server/restapi/change/GetDiff.java +++ b/java/com/google/gerrit/server/restapi/change/GetDiff.java @@ -91,10 +91,6 @@ public class GetDiff implements RestReadView { @Option(name = "--intraline") boolean intraline; - // TODO(ghareeb): This is a temporary parameter for debugging. Please remove. - @Option(name = "--use-new-diff-cache") - boolean useNewDiffCache; - @Inject GetDiff( ProjectCache projectCache, @@ -143,15 +139,13 @@ public class GetDiff implements RestReadView { } psf = patchScriptFactoryFactory.create( - notes, fileName, basePatchSet.id(), pId, prefs, currentUser.get(), useNewDiffCache); + notes, fileName, basePatchSet.id(), pId, prefs, currentUser.get()); } else if (parentNum > 0) { psf = patchScriptFactoryFactory.create( - notes, fileName, parentNum - 1, pId, prefs, currentUser.get(), useNewDiffCache); + notes, fileName, parentNum - 1, pId, prefs, currentUser.get()); } else { - psf = - patchScriptFactoryFactory.create( - notes, fileName, null, pId, prefs, currentUser.get(), useNewDiffCache); + psf = patchScriptFactoryFactory.create(notes, fileName, null, pId, prefs, currentUser.get()); } try { diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java index ec59674ffc..7a2839edcc 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java @@ -76,7 +76,7 @@ public class RevisionDiffIT extends AbstractDaemonTest { private static final String FILE_CONTENT2 = "1st line\n2nd line\n3rd line\n"; private boolean intraline; - private boolean useNewDiffCache; + private boolean useNewDiffCacheListFiles; private boolean useNewDiffCacheGetDiff; private ObjectId commit1; @@ -91,9 +91,10 @@ public class RevisionDiffIT extends AbstractDaemonTest { baseConfig.setString("cache", "diff_intraline", "timeout", "1 minute"); intraline = baseConfig.getBoolean(TEST_PARAMETER_MARKER, "intraline", false); - useNewDiffCache = baseConfig.getBoolean("cache", "diff_cache", "useNewDiffCache", false); + useNewDiffCacheListFiles = + baseConfig.getBoolean("cache", "diff_cache", "runNewDiffCache_ListFiles", false); useNewDiffCacheGetDiff = - baseConfig.getBoolean("cache", "diff_cache", "useNewDiffCache_getDiff", false); + baseConfig.getBoolean("cache", "diff_cache", "runNewDiffCache_GetDiff", false); ObjectId headCommit = testRepo.getRepository().resolve("HEAD"); commit1 = @@ -1276,7 +1277,7 @@ public class RevisionDiffIT extends AbstractDaemonTest { public void renamedUnrelatedFileIsIgnored_ForPatchSetDiffWithRebase_WhenEquallyModifiedInBoth() throws Exception { // TODO(ghareeb): fix this test for the new diff cache implementation - assume().that(useNewDiffCache).isFalse(); + assume().that(useNewDiffCacheListFiles).isFalse(); Function contentModification = fileContent -> fileContent.replace("1st line\n", "First line\n"); @@ -1369,7 +1370,7 @@ public class RevisionDiffIT extends AbstractDaemonTest { @Test public void filesTouchedByPatchSetsAndContainingOnlyRebaseHunksAreIgnored() throws Exception { // TODO(ghareeb): fix this test for the new diff cache implementation - assume().that(useNewDiffCache).isFalse(); + assume().that(useNewDiffCacheListFiles).isFalse(); addModifiedPatchSet( changeId, FILE_NAME, fileContent -> fileContent.replace("Line 50\n", "Line fifty\n")); @@ -2751,7 +2752,7 @@ public class RevisionDiffIT extends AbstractDaemonTest { @Test public void symlinkConvertedToRegularFileIsIdentifiedAsAdded() throws Exception { // TODO(ghareeb): fix this test for the new diff cache implementation - assume().that(useNewDiffCache).isFalse(); + assume().that(useNewDiffCacheListFiles).isFalse(); assume().that(useNewDiffCacheGetDiff).isFalse(); String target = "file.txt"; diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheForSingleFileIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheForSingleFileIT.java index e55f432729..46954e982d 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheForSingleFileIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheForSingleFileIT.java @@ -26,7 +26,7 @@ public class RevisionNewDiffCacheForSingleFileIT extends RevisionDiffIT { @ConfigSuite.Default public static Config newDiffCacheConfig() { Config config = new Config(); - config.setBoolean("cache", "diff_cache", "runNewDiffCacheAsync_getDiff", true); + config.setBoolean("cache", "diff_cache", "runNewDiffCache_GetDiff", true); return config; } } diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheIT.java index 4b85c30dde..ec0bcc62de 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheIT.java @@ -18,14 +18,15 @@ import com.google.gerrit.testing.ConfigSuite; import org.eclipse.jgit.lib.Config; /** - * Runs the {@link RevisionDiffIT} tests with the new diff cache. This is temporary until the new - * diff cache is fully deployed. The new diff cache will become the default in the future. + * Runs the {@link RevisionDiffIT} with the list files endpoint using the new diff cache. This is + * temporary until the new diff cache is fully deployed. The new diff cache will become the default + * in the future. */ public class RevisionNewDiffCacheIT extends RevisionDiffIT { @ConfigSuite.Default public static Config newDiffCacheConfig() { Config config = new Config(); - config.setBoolean("cache", "diff_cache", "useNewDiffCache", true); + config.setBoolean("cache", "diff_cache", "runNewDiffCache_ListFiles", true); return config; } }