Remove old diff cache
This change removes PatchListLoader and does some cleanup to silbling
classses and tests.
Bug: Google b/200147261
Change-Id: I52738c855ff750abe9bc882e67cf513e83629534
diff --git a/java/com/google/gerrit/server/patch/DiffExecutor.java b/java/com/google/gerrit/server/patch/DiffExecutor.java
index 63d5c50..c9b87ff 100644
--- a/java/com/google/gerrit/server/patch/DiffExecutor.java
+++ b/java/com/google/gerrit/server/patch/DiffExecutor.java
@@ -16,14 +16,11 @@
import static java.lang.annotation.RetentionPolicy.RUNTIME;
-import com.google.gerrit.server.patch.filediff.PatchListLoader;
import com.google.inject.BindingAnnotation;
import java.lang.annotation.Retention;
import java.util.concurrent.ExecutorService;
-/**
- * Marker on {@link ExecutorService} used by {@link IntraLineLoader} and {@link PatchListLoader}.
- */
+/** Marker on {@link ExecutorService} used by {@link IntraLineLoader}. */
@Retention(RUNTIME)
@BindingAnnotation
public @interface DiffExecutor {}
diff --git a/java/com/google/gerrit/server/patch/PatchListCache.java b/java/com/google/gerrit/server/patch/PatchListCache.java
index 6d42249..b8651e0 100644
--- a/java/com/google/gerrit/server/patch/PatchListCache.java
+++ b/java/com/google/gerrit/server/patch/PatchListCache.java
@@ -16,18 +16,11 @@
import com.google.gerrit.entities.Project;
-/** Provides a cached list of {@link PatchListEntry}. */
+/**
+ * Provides a cached list of intra-line and summary diffs. Use {@link DiffOperations} to compute
+ * detailed file diffs.
+ */
public interface PatchListCache {
- /**
- * Returns the patch list - list of modified files - between two commits.
- *
- * @param key identifies the old / new commits.
- * @param project name key identifying a specific git project (repository).
- * @return patch list containing the modified files between two commits.
- * @deprecated use {@link DiffOperations} instead.
- */
- @Deprecated
- PatchList get(PatchListKey key, Project.NameKey project) throws PatchListNotAvailableException;
IntraLineDiff getIntraLineDiff(IntraLineDiffKey key, IntraLineDiffArgs args);
diff --git a/java/com/google/gerrit/server/patch/PatchListCacheImpl.java b/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
index dd2bb47..eab0c22 100644
--- a/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
@@ -15,14 +15,12 @@
package com.google.gerrit.server.patch;
-import com.google.common.annotations.VisibleForTesting;
import com.google.common.cache.Cache;
+import com.google.common.flogger.FluentLogger;
import com.google.common.util.concurrent.UncheckedExecutionException;
import com.google.gerrit.entities.Project;
-import com.google.gerrit.server.cache.CacheBackend;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.patch.filediff.PatchListLoader;
import com.google.inject.Inject;
import com.google.inject.Module;
import com.google.inject.Singleton;
@@ -30,12 +28,12 @@
import java.util.concurrent.ExecutionException;
import org.eclipse.jgit.errors.LargeObjectException;
import org.eclipse.jgit.lib.Config;
-import org.eclipse.jgit.lib.ObjectId;
/** Provides a cached list of {@link PatchListEntry}. */
@Singleton
public class PatchListCacheImpl implements PatchListCache {
- public static final String FILE_NAME = "diff";
+ public static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
static final String INTRA_NAME = "diff_intraline";
static final String DIFF_SUMMARY = "diff_summary";
@@ -43,13 +41,6 @@
return new CacheModule() {
@Override
protected void configure() {
- factory(PatchListLoader.Factory.class);
- // TODO(davido): Switch off using legacy cache backend, after fixing PatchListLoader
- // to be recursion free.
- persist(FILE_NAME, PatchListKey.class, PatchList.class, CacheBackend.GUAVA)
- .maximumWeight(10 << 20)
- .weigher(PatchListWeigher.class);
-
factory(IntraLineLoader.Factory.class);
persist(INTRA_NAME, IntraLineDiffKey.class, IntraLineDiff.class)
.maximumWeight(10 << 20)
@@ -67,27 +58,21 @@
};
}
- private final Cache<PatchListKey, PatchList> fileCache;
private final Cache<IntraLineDiffKey, IntraLineDiff> intraCache;
private final Cache<DiffSummaryKey, DiffSummary> diffSummaryCache;
- private final PatchListLoader.Factory fileLoaderFactory;
private final IntraLineLoader.Factory intraLoaderFactory;
private final DiffSummaryLoader.Factory diffSummaryLoaderFactory;
private final boolean computeIntraline;
@Inject
PatchListCacheImpl(
- @Named(FILE_NAME) Cache<PatchListKey, PatchList> fileCache,
@Named(INTRA_NAME) Cache<IntraLineDiffKey, IntraLineDiff> intraCache,
@Named(DIFF_SUMMARY) Cache<DiffSummaryKey, DiffSummary> diffSummaryCache,
- PatchListLoader.Factory fileLoaderFactory,
IntraLineLoader.Factory intraLoaderFactory,
DiffSummaryLoader.Factory diffSummaryLoaderFactory,
@GerritServerConfig Config cfg) {
- this.fileCache = fileCache;
this.intraCache = intraCache;
this.diffSummaryCache = diffSummaryCache;
- this.fileLoaderFactory = fileLoaderFactory;
this.intraLoaderFactory = intraLoaderFactory;
this.diffSummaryLoaderFactory = diffSummaryLoaderFactory;
@@ -97,31 +82,6 @@
}
@Override
- public PatchList get(PatchListKey key, Project.NameKey project)
- throws PatchListNotAvailableException {
- try {
- PatchList pl = fileCache.get(key, fileLoaderFactory.create(key, project));
- if (pl instanceof LargeObjectTombstone) {
- throw new PatchListObjectTooLargeException(
- "Error computing " + key + ". Previous attempt failed with LargeObjectException");
- }
- return pl;
- } catch (ExecutionException e) {
- PatchListLoader.logger.atWarning().withCause(e).log("Error computing %s", key);
- throw new PatchListNotAvailableException(e);
- } catch (UncheckedExecutionException e) {
- if (e.getCause() instanceof LargeObjectException) {
- // Cache negative result so we don't need to redo expensive computations that would yield
- // the same result.
- fileCache.put(key, new LargeObjectTombstone());
- PatchListLoader.logger.atWarning().withCause(e).log("Error computing %s", key);
- throw new PatchListNotAvailableException(e);
- }
- throw e;
- }
- }
-
- @Override
public IntraLineDiff getIntraLineDiff(IntraLineDiffKey key, IntraLineDiffArgs args) {
if (computeIntraline) {
try {
@@ -140,28 +100,14 @@
try {
return diffSummaryCache.get(key, diffSummaryLoaderFactory.create(key, project));
} catch (ExecutionException e) {
- PatchListLoader.logger.atWarning().withCause(e).log("Error computing %s", key);
+ logger.atWarning().withCause(e).log("Error computing %s", key);
throw new PatchListNotAvailableException(e);
} catch (UncheckedExecutionException e) {
if (e.getCause() instanceof LargeObjectException) {
- PatchListLoader.logger.atWarning().withCause(e).log("Error computing %s", key);
+ logger.atWarning().withCause(e).log("Error computing %s", key);
throw new PatchListNotAvailableException(e);
}
throw e;
}
}
-
- /** Used to cache negative results in {@code fileCache}. */
- @VisibleForTesting
- public static class LargeObjectTombstone extends PatchList {
- private static final long serialVersionUID = 1L;
-
- @VisibleForTesting
- public LargeObjectTombstone() {
- // Initialize super class with valid values. We don't care about the inner state, but need to
- // pass valid values that don't break (de)serialization.
- super(
- null, ObjectId.zeroId(), false, ComparisonType.againstAutoMerge(), new PatchListEntry[0]);
- }
- }
}
diff --git a/java/com/google/gerrit/server/patch/PatchListWeigher.java b/java/com/google/gerrit/server/patch/PatchListWeigher.java
deleted file mode 100644
index 942d0e0..0000000
--- a/java/com/google/gerrit/server/patch/PatchListWeigher.java
+++ /dev/null
@@ -1,37 +0,0 @@
-// Copyright (C) 2012 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.patch;
-
-import com.google.common.cache.Weigher;
-
-/** Approximates memory usage for PatchList in bytes of memory used. */
-public class PatchListWeigher implements Weigher<PatchListKey, PatchList> {
- @Override
- public int weigh(PatchListKey key, PatchList value) {
- int size =
- 16
- + 4 * 8
- + 2 * 36
- + 8 // Size of PatchListKey, 64 bit JVM
- + 16
- + 3 * 8
- + 3 * 4
- + 20; // Size of PatchList, 64 bit JVM
- for (PatchListEntry e : value.getPatches()) {
- size += e.weigh();
- }
- return size;
- }
-}
diff --git a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
index 0b08c4f..33300e3 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
@@ -71,28 +71,8 @@
intralineDiffCalculator = calculator;
}
- /** Convert into {@link PatchScript} using the old diff cache output. */
- PatchScript toPatchScriptOld(Repository git, PatchList list, PatchListEntry content)
- throws IOException {
-
- PatchFileChange change =
- new PatchFileChange(
- content.getEdits(),
- content.getEditsDueToRebase(),
- content.getHeaderLines(),
- content.getOldName(),
- content.getNewName(),
- content.getChangeType(),
- content.getPatchType());
- SidesResolver sidesResolver = new SidesResolver(git, list.getComparisonType());
- ResolvedSides sides =
- resolveSides(
- git, sidesResolver, oldName(change), newName(change), list.getOldId(), list.getNewId());
- return build(sides.a, sides.b, change);
- }
-
/** Convert into {@link PatchScript} using the new diff cache output. */
- PatchScript toPatchScriptNew(Repository git, FileDiffOutput content) throws IOException {
+ PatchScript toPatchScript(Repository git, FileDiffOutput content) throws IOException {
PatchFileChange change =
new PatchFileChange(
content.edits().stream().map(TaggedEdit::jgitEdit).collect(toImmutableList()),
diff --git a/java/com/google/gerrit/server/patch/PatchScriptFactory.java b/java/com/google/gerrit/server/patch/PatchScriptFactory.java
index 96b23c8..02f125a 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptFactory.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptFactory.java
@@ -229,7 +229,7 @@
notes.getProjectName(), bId, parentNum, fileName, diffPrefs.ignoreWhitespace)
: diffOperations.getModifiedFile(
notes.getProjectName(), aId, bId, fileName, diffPrefs.ignoreWhitespace);
- return newBuilder().toPatchScriptNew(git, fileDiffOutput);
+ return newBuilder().toPatchScript(git, fileDiffOutput);
}
private Optional<ObjectId> getAId() {
diff --git a/java/com/google/gerrit/server/patch/filediff/PatchListLoader.java b/java/com/google/gerrit/server/patch/filediff/PatchListLoader.java
deleted file mode 100644
index 031f3db..0000000
--- a/java/com/google/gerrit/server/patch/filediff/PatchListLoader.java
+++ /dev/null
@@ -1,705 +0,0 @@
-// Copyright (C) 2020 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.patch.filediff;
-
-import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.collect.ImmutableList.toImmutableList;
-import static java.nio.charset.StandardCharsets.UTF_8;
-import static java.util.stream.Collectors.toSet;
-import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
-
-import com.google.auto.value.AutoValue;
-import com.google.common.base.Throwables;
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMultimap;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Iterables;
-import com.google.common.collect.Multimap;
-import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.entities.Patch;
-import com.google.gerrit.entities.Project;
-import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
-import com.google.gerrit.metrics.Counter0;
-import com.google.gerrit.metrics.Description;
-import com.google.gerrit.metrics.MetricMaker;
-import com.google.gerrit.server.config.ConfigUtil;
-import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.git.InMemoryInserter;
-import com.google.gerrit.server.git.MergeUtil;
-import com.google.gerrit.server.patch.AutoMerger;
-import com.google.gerrit.server.patch.ComparisonType;
-import com.google.gerrit.server.patch.DiffExecutor;
-import com.google.gerrit.server.patch.PatchList;
-import com.google.gerrit.server.patch.PatchListCache;
-import com.google.gerrit.server.patch.PatchListCacheImpl;
-import com.google.gerrit.server.patch.PatchListEntry;
-import com.google.gerrit.server.patch.PatchListKey;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
-import com.google.gerrit.server.patch.Text;
-import com.google.gerrit.server.patch.filediff.EditTransformer.ContextAwareEdit;
-import com.google.inject.Inject;
-import com.google.inject.Singleton;
-import com.google.inject.assistedinject.Assisted;
-import java.io.IOException;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Optional;
-import java.util.Set;
-import java.util.concurrent.Callable;
-import java.util.concurrent.ExecutionException;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Future;
-import java.util.concurrent.TimeUnit;
-import java.util.concurrent.TimeoutException;
-import org.eclipse.jgit.diff.DiffEntry;
-import org.eclipse.jgit.diff.DiffEntry.ChangeType;
-import org.eclipse.jgit.diff.DiffFormatter;
-import org.eclipse.jgit.diff.Edit;
-import org.eclipse.jgit.diff.EditList;
-import org.eclipse.jgit.diff.HistogramDiff;
-import org.eclipse.jgit.diff.RawText;
-import org.eclipse.jgit.diff.RawTextComparator;
-import org.eclipse.jgit.lib.AbbreviatedObjectId;
-import org.eclipse.jgit.lib.Config;
-import org.eclipse.jgit.lib.Constants;
-import org.eclipse.jgit.lib.FileMode;
-import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.ObjectInserter;
-import org.eclipse.jgit.lib.ObjectReader;
-import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.merge.ThreeWayMergeStrategy;
-import org.eclipse.jgit.patch.FileHeader;
-import org.eclipse.jgit.patch.FileHeader.PatchType;
-import org.eclipse.jgit.revwalk.RevCommit;
-import org.eclipse.jgit.revwalk.RevObject;
-import org.eclipse.jgit.revwalk.RevTree;
-import org.eclipse.jgit.revwalk.RevWalk;
-import org.eclipse.jgit.treewalk.TreeWalk;
-import org.eclipse.jgit.util.io.DisabledOutputStream;
-
-public class PatchListLoader implements Callable<PatchList> {
- public static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
- public interface Factory {
- PatchListLoader create(PatchListKey key, Project.NameKey project);
- }
-
- @Singleton
- static class Metrics {
- final Counter0 timeouts;
-
- @Inject
- Metrics(MetricMaker metricMaker) {
- // TODO(ghareeb): Remove this metric from documentation once this class is deprecated.
- timeouts =
- metricMaker.newCounter(
- "caches/diff/legacy/timeouts",
- new Description(
- "Total number of git file diff computations that resulted in timeouts.")
- .setRate()
- .setUnit("count"));
- }
- }
-
- private final GitRepositoryManager repoManager;
- private final PatchListCache patchListCache;
- private final ThreeWayMergeStrategy mergeStrategy;
- private final ExecutorService diffExecutor;
- private final AutoMerger autoMerger;
- private final PatchListKey key;
- private final Metrics metrics;
- private final Project.NameKey project;
- private final long timeoutMillis;
-
- @Inject
- PatchListLoader(
- GitRepositoryManager mgr,
- PatchListCache plc,
- @GerritServerConfig Config cfg,
- @DiffExecutor ExecutorService de,
- AutoMerger am,
- Metrics metrics,
- @Assisted PatchListKey k,
- @Assisted Project.NameKey p) {
- this.repoManager = mgr;
- this.patchListCache = plc;
- this.mergeStrategy = MergeUtil.getMergeStrategy(cfg);
- this.diffExecutor = de;
- this.autoMerger = am;
- this.metrics = metrics;
- this.key = k;
- this.project = p;
- this.timeoutMillis =
- ConfigUtil.getTimeUnit(
- cfg,
- "cache",
- PatchListCacheImpl.FILE_NAME,
- "timeout",
- TimeUnit.MILLISECONDS.convert(5, TimeUnit.SECONDS),
- TimeUnit.MILLISECONDS);
- }
-
- @Override
- public PatchList call() throws IOException, PatchListNotAvailableException {
- try (Repository repo = repoManager.openRepository(project);
- InMemoryInserter ins = new InMemoryInserter(repo);
- ObjectReader reader = ins.newReader();
- RevWalk rw = new RevWalk(reader)) {
- return readPatchList(repo, rw, ins);
- }
- }
-
- private static RawTextComparator comparatorFor(Whitespace ws) {
- switch (ws) {
- case IGNORE_ALL:
- return RawTextComparator.WS_IGNORE_ALL;
-
- case IGNORE_TRAILING:
- return RawTextComparator.WS_IGNORE_TRAILING;
-
- case IGNORE_LEADING_AND_TRAILING:
- return RawTextComparator.WS_IGNORE_CHANGE;
-
- case IGNORE_NONE:
- default:
- return RawTextComparator.DEFAULT;
- }
- }
-
- private PatchList readPatchList(Repository repo, RevWalk rw, InMemoryInserter ins)
- throws IOException, PatchListNotAvailableException {
- ObjectReader reader = rw.getObjectReader();
- checkArgument(reader.getCreatedFromInserter() == ins);
- RawTextComparator cmp = comparatorFor(key.getWhitespace());
- try (DiffFormatter df = new DiffFormatter(DisabledOutputStream.INSTANCE)) {
- RevCommit b = rw.parseCommit(key.getNewId());
- RevObject a = aFor(key, repo, rw, ins, b);
-
- if (a == null) {
- // TODO(sop) Remove this case.
- // This is an octopus merge commit which should be compared against the
- // auto-merge. However since we don't support computing the auto-merge
- // for octopus merge commits, we fall back to diffing against the first
- // parent, even though this wasn't what was requested.
- //
- ComparisonType comparisonType = ComparisonType.againstParent(1);
- PatchListEntry[] entries = new PatchListEntry[2];
- entries[0] = newCommitMessage(cmp, reader, null, b);
- entries[1] = newMergeList(cmp, reader, null, b, comparisonType);
- return new PatchList(a, b, true, comparisonType, entries);
- }
-
- ComparisonType comparisonType = getComparisonType(a, b);
-
- RevCommit aCommit = a instanceof RevCommit ? (RevCommit) a : null;
- RevTree aTree = rw.parseTree(a);
- RevTree bTree = b.getTree();
-
- df.setReader(reader, repo.getConfig());
- df.setDiffComparator(cmp);
- df.setDetectRenames(true);
- List<DiffEntry> diffEntries = df.scan(aTree, bTree);
-
- EditsDueToRebaseResult editsDueToRebaseResult =
- determineEditsDueToRebase(aCommit, b, diffEntries, df, rw);
- diffEntries = editsDueToRebaseResult.getRelevantOriginalDiffEntries();
- Multimap<String, ContextAwareEdit> editsDueToRebasePerFilePath =
- editsDueToRebaseResult.getEditsDueToRebasePerFilePath();
-
- List<PatchListEntry> entries = new ArrayList<>();
- entries.add(
- newCommitMessage(
- cmp, reader, comparisonType.isAgainstParentOrAutoMerge() ? null : aCommit, b));
- boolean isMerge = b.getParentCount() > 1;
- if (isMerge) {
- entries.add(
- newMergeList(
- cmp,
- reader,
- comparisonType.isAgainstParentOrAutoMerge() ? null : aCommit,
- b,
- comparisonType));
- }
- for (DiffEntry diffEntry : diffEntries) {
- Set<ContextAwareEdit> editsDueToRebase =
- getEditsDueToRebase(editsDueToRebasePerFilePath, diffEntry);
- Optional<PatchListEntry> patchListEntry =
- getPatchListEntry(reader, df, diffEntry, aTree, bTree, editsDueToRebase);
- patchListEntry.ifPresent(entries::add);
- }
- return new PatchList(
- a, b, isMerge, comparisonType, entries.toArray(new PatchListEntry[entries.size()]));
- }
- }
-
- /**
- * Identifies the edits which are present between {@code commitA} and {@code commitB} due to other
- * commits in between those two. Edits which cannot be clearly attributed to those other commits
- * (because they overlap with modifications introduced by {@code commitA} or {@code commitB}) are
- * omitted from the result. The edits are expressed as differences between {@code treeA} of {@code
- * commitA} and {@code treeB} of {@code commitB}.
- *
- * <p><b>Note:</b> If one of the commits is a merge commit, an empty {@code Multimap} will be
- * returned.
- *
- * <p><b>Warning:</b> This method assumes that commitA and commitB are either a parent and child
- * commit or represent two patch sets which belong to the same change. No checks are made to
- * confirm this assumption! Passing arbitrary commits to this method may lead to strange results
- * or take very long.
- *
- * <p>This logic could be expanded to arbitrary commits if the following adjustments were applied:
- *
- * <ul>
- * <li>If {@code commitA} is an ancestor of {@code commitB} (or the other way around), {@code
- * commitA} (or {@code commitB}) is used instead of its parent in this method.
- * <li>Special handling for merge commits is added. If only one of them is a merge commit, the
- * whole computation has to be done between the single parent and all parents of the merge
- * commit. If both of them are merge commits, all combinations of parents have to be
- * considered. Alternatively, we could decide to not support this feature for merge commits
- * (or just for specific types of merge commits).
- * </ul>
- *
- * @param commitA the commit defining {@code treeA}
- * @param commitB the commit defining {@code treeB}
- * @param diffEntries the list of {@code DiffEntries} for the diff between {@code commitA} and
- * {@code commitB}
- * @param df the {@code DiffFormatter}
- * @param rw the current {@code RevWalk}
- * @return an aggregated result of the computation
- * @throws PatchListNotAvailableException if the edits can't be identified
- * @throws IOException if an error occurred while accessing the repository
- */
- private EditsDueToRebaseResult determineEditsDueToRebase(
- RevCommit commitA,
- RevCommit commitB,
- List<DiffEntry> diffEntries,
- DiffFormatter df,
- RevWalk rw)
- throws PatchListNotAvailableException, IOException {
- if (commitA == null
- || isRootOrMergeCommit(commitA)
- || isRootOrMergeCommit(commitB)
- || areParentChild(commitA, commitB)
- || haveCommonParent(commitA, commitB)) {
- return EditsDueToRebaseResult.create(diffEntries, ImmutableMultimap.of());
- }
-
- PatchListKey oldKey = PatchListKey.againstDefaultBase(key.getOldId(), key.getWhitespace());
- PatchList oldPatchList = patchListCache.get(oldKey, project);
- PatchListKey newKey = PatchListKey.againstDefaultBase(key.getNewId(), key.getWhitespace());
- PatchList newPatchList = patchListCache.get(newKey, project);
-
- List<PatchListEntry> oldPatches = oldPatchList.getPatches();
- List<PatchListEntry> newPatches = newPatchList.getPatches();
- // TODO(aliceks): Have separate but more limited lists for parents and patch sets (but don't
- // mess up renames/copies).
- Set<String> touchedFilePaths = new HashSet<>();
- for (PatchListEntry patchListEntry : oldPatches) {
- touchedFilePaths.addAll(getTouchedFilePaths(patchListEntry));
- }
- for (PatchListEntry patchListEntry : newPatches) {
- touchedFilePaths.addAll(getTouchedFilePaths(patchListEntry));
- }
-
- List<DiffEntry> relevantDiffEntries =
- diffEntries.stream()
- .filter(diffEntry -> isTouched(touchedFilePaths, diffEntry))
- .collect(toImmutableList());
-
- RevCommit parentCommitA = commitA.getParent(0);
- rw.parseBody(parentCommitA);
- RevCommit parentCommitB = commitB.getParent(0);
- rw.parseBody(parentCommitB);
- List<DiffEntry> parentDiffEntries = df.scan(parentCommitA, parentCommitB);
- // TODO(aliceks): Find a way to not construct a PatchListEntry as it contains many unnecessary
- // details and we don't fill all of them properly.
- List<PatchListEntry> parentPatchListEntries =
- getRelevantPatchListEntries(
- parentDiffEntries, parentCommitA, parentCommitB, touchedFilePaths, df);
-
- EditTransformer editTransformer = new EditTransformer(toFileEditsList(parentPatchListEntries));
- editTransformer.transformReferencesOfSideA(toFileEditsList(oldPatches));
- editTransformer.transformReferencesOfSideB(toFileEditsList(newPatches));
- return EditsDueToRebaseResult.create(
- relevantDiffEntries, editTransformer.getEditsPerFilePath());
- }
-
- private ImmutableList<FileEdits> toFileEditsList(List<PatchListEntry> entries) {
- return entries.stream().map(PatchListLoader::toFileEdits).collect(toImmutableList());
- }
-
- private static FileEdits toFileEdits(PatchListEntry patchListEntry) {
- Optional<String> oldName = Optional.empty();
- Optional<String> newName = Optional.empty();
- switch (patchListEntry.getChangeType()) {
- case DELETED:
- oldName = Optional.of(patchListEntry.getNewName());
- break;
- case ADDED:
- case MODIFIED:
- case REWRITE:
- newName = Optional.of(patchListEntry.getNewName());
- break;
-
- case COPIED:
- case RENAMED:
- oldName = Optional.of(patchListEntry.getOldName());
- newName = Optional.of(patchListEntry.getNewName());
- break;
- }
- return FileEdits.createFromJgitEdits(patchListEntry.getEdits(), oldName, newName);
- }
-
- private static boolean isRootOrMergeCommit(RevCommit commit) {
- return commit.getParentCount() != 1;
- }
-
- private static boolean areParentChild(RevCommit commitA, RevCommit commitB) {
- return ObjectId.isEqual(commitA.getParent(0), commitB)
- || ObjectId.isEqual(commitB.getParent(0), commitA);
- }
-
- private static boolean haveCommonParent(RevCommit commitA, RevCommit commitB) {
- return ObjectId.isEqual(commitA.getParent(0), commitB.getParent(0));
- }
-
- private static Set<String> getTouchedFilePaths(PatchListEntry patchListEntry) {
- String oldFilePath = patchListEntry.getOldName();
- String newFilePath = patchListEntry.getNewName();
-
- return oldFilePath == null
- ? ImmutableSet.of(newFilePath)
- : ImmutableSet.of(oldFilePath, newFilePath);
- }
-
- private static boolean isTouched(Set<String> touchedFilePaths, DiffEntry diffEntry) {
- String oldFilePath = diffEntry.getOldPath();
- String newFilePath = diffEntry.getNewPath();
- // One of the above file paths could be /dev/null but we need not explicitly check for this
- // value as the set of file paths shouldn't contain it.
- return touchedFilePaths.contains(oldFilePath) || touchedFilePaths.contains(newFilePath);
- }
-
- private List<PatchListEntry> getRelevantPatchListEntries(
- List<DiffEntry> parentDiffEntries,
- RevCommit parentCommitA,
- RevCommit parentCommitB,
- Set<String> touchedFilePaths,
- DiffFormatter diffFormatter)
- throws IOException {
- List<PatchListEntry> parentPatchListEntries = new ArrayList<>(parentDiffEntries.size());
- for (DiffEntry parentDiffEntry : parentDiffEntries) {
- if (!isTouched(touchedFilePaths, parentDiffEntry)) {
- continue;
- }
- FileHeader fileHeader = toFileHeader(parentCommitB, diffFormatter, parentDiffEntry);
- // The code which uses this PatchListEntry doesn't care about the last three parameters. As
- // they are expensive to compute, we use arbitrary values for them.
- PatchListEntry patchListEntry =
- newEntry(parentCommitA.getTree(), fileHeader, ImmutableSet.of(), 0, 0);
- parentPatchListEntries.add(patchListEntry);
- }
- return parentPatchListEntries;
- }
-
- private static Set<ContextAwareEdit> getEditsDueToRebase(
- Multimap<String, ContextAwareEdit> editsDueToRebasePerFilePath, DiffEntry diffEntry) {
- if (editsDueToRebasePerFilePath.isEmpty()) {
- return ImmutableSet.of();
- }
-
- String filePath = diffEntry.getNewPath();
- if (diffEntry.getChangeType() == ChangeType.DELETE) {
- filePath = diffEntry.getOldPath();
- }
- return ImmutableSet.copyOf(editsDueToRebasePerFilePath.get(filePath));
- }
-
- private Optional<PatchListEntry> getPatchListEntry(
- ObjectReader objectReader,
- DiffFormatter diffFormatter,
- DiffEntry diffEntry,
- RevTree treeA,
- RevTree treeB,
- Set<ContextAwareEdit> editsDueToRebase)
- throws IOException {
- FileHeader fileHeader = toFileHeader(key.getNewId(), diffFormatter, diffEntry);
- long oldSize =
- getFileSize(
- objectReader,
- diffEntry.getOldId(),
- diffEntry.getOldMode(),
- diffEntry.getOldPath(),
- treeA);
- long newSize =
- getFileSize(
- objectReader,
- diffEntry.getNewId(),
- diffEntry.getNewMode(),
- diffEntry.getNewPath(),
- treeB);
- Set<Edit> contentEditsDueToRebase = getContentEdits(editsDueToRebase);
- PatchListEntry patchListEntry =
- newEntry(treeA, fileHeader, contentEditsDueToRebase, newSize, newSize - oldSize);
- // All edits in a file are due to rebase -> exclude the file from the diff.
- if (EditTransformer.toEdits(toFileEdits(patchListEntry)).allMatch(editsDueToRebase::contains)) {
- return Optional.empty();
- }
- return Optional.of(patchListEntry);
- }
-
- private static Set<Edit> getContentEdits(Set<ContextAwareEdit> editsDueToRebase) {
- return editsDueToRebase.stream()
- .map(ContextAwareEdit::toEdit)
- .filter(Optional::isPresent)
- .map(Optional::get)
- .collect(toSet());
- }
-
- private ComparisonType getComparisonType(RevObject a, RevCommit b) {
- for (int i = 0; i < b.getParentCount(); i++) {
- if (b.getParent(i).equals(a)) {
- return ComparisonType.againstParent(i + 1);
- }
- }
-
- if (key.getOldId() == null && b.getParentCount() > 0) {
- return ComparisonType.againstAutoMerge();
- }
-
- return ComparisonType.againstOtherPatchSet();
- }
-
- private static long getFileSize(
- ObjectReader reader, AbbreviatedObjectId abbreviatedId, FileMode mode, String path, RevTree t)
- throws IOException {
- if (!isBlob(mode)) {
- return 0;
- }
- ObjectId fileId =
- toObjectId(reader, abbreviatedId).orElseGet(() -> lookupObjectId(reader, path, t));
- if (ObjectId.zeroId().equals(fileId)) {
- return 0;
- }
- return reader.getObjectSize(fileId, OBJ_BLOB);
- }
-
- private static boolean isBlob(FileMode mode) {
- int t = mode.getBits() & FileMode.TYPE_MASK;
- return t == FileMode.TYPE_FILE || t == FileMode.TYPE_SYMLINK;
- }
-
- private static Optional<ObjectId> toObjectId(
- ObjectReader reader, AbbreviatedObjectId abbreviatedId) throws IOException {
- if (abbreviatedId == null) {
- // In theory, DiffEntry#getOldId or DiffEntry#getNewId can be null for pure renames or pure
- // mode changes (e.g. DiffEntry#modify doesn't set the IDs). However, the method we call
- // for diffs (DiffFormatter#scan) seems to always produce DiffEntries with set IDs, even for
- // pure renames.
- return Optional.empty();
- }
- if (abbreviatedId.isComplete()) {
- // With the current JGit version and the method we call for diffs (DiffFormatter#scan), this
- // is the only code path taken right now.
- return Optional.ofNullable(abbreviatedId.toObjectId());
- }
- Collection<ObjectId> objectIds = reader.resolve(abbreviatedId);
- // It seems very unlikely that an ObjectId which was just abbreviated by the diff computation
- // now can't be resolved to exactly one ObjectId. The API allows this possibility, though.
- return objectIds.size() == 1
- ? Optional.of(Iterables.getOnlyElement(objectIds))
- : Optional.empty();
- }
-
- private static ObjectId lookupObjectId(ObjectReader reader, String path, RevTree tree) {
- // This variant is very expensive.
- try (TreeWalk treeWalk = TreeWalk.forPath(reader, path, tree)) {
- return treeWalk != null ? treeWalk.getObjectId(0) : ObjectId.zeroId();
- } catch (IOException e) {
- throw new StorageException(e);
- }
- }
-
- private FileHeader toFileHeader(
- ObjectId commitB, DiffFormatter diffFormatter, DiffEntry diffEntry) throws IOException {
-
- Future<FileHeader> result =
- diffExecutor.submit(
- () -> {
- synchronized (diffEntry) {
- return diffFormatter.toFileHeader(diffEntry);
- }
- });
-
- try {
- return result.get(timeoutMillis, TimeUnit.MILLISECONDS);
- } catch (InterruptedException | TimeoutException e) {
- metrics.timeouts.increment();
- logger.atWarning().log(
- "%s ms timeout reached for Diff loader in project %s"
- + " on commit %s on path %s comparing %s..%s",
- timeoutMillis,
- project,
- commitB.name(),
- diffEntry.getNewPath(),
- diffEntry.getOldId().name(),
- diffEntry.getNewId().name());
- result.cancel(true);
- synchronized (diffEntry) {
- return toFileHeaderWithoutMyersDiff(diffFormatter, diffEntry);
- }
- } catch (ExecutionException e) {
- // If there was an error computing the result, carry it
- // up to the caller so the cache knows this key is invalid.
- Throwables.throwIfInstanceOf(e.getCause(), IOException.class);
- throw new IOException(e.getMessage(), e.getCause());
- }
- }
-
- private FileHeader toFileHeaderWithoutMyersDiff(DiffFormatter diffFormatter, DiffEntry diffEntry)
- throws IOException {
- HistogramDiff histogramDiff = new HistogramDiff();
- histogramDiff.setFallbackAlgorithm(null);
- diffFormatter.setDiffAlgorithm(histogramDiff);
- return diffFormatter.toFileHeader(diffEntry);
- }
-
- private PatchListEntry newCommitMessage(
- RawTextComparator cmp, ObjectReader reader, RevCommit aCommit, RevCommit bCommit)
- throws IOException {
- Text aText = aCommit != null ? Text.forCommit(reader, aCommit) : Text.EMPTY;
- Text bText = Text.forCommit(reader, bCommit);
- return createPatchListEntry(cmp, aCommit, aText, bText, Patch.COMMIT_MSG);
- }
-
- private PatchListEntry newMergeList(
- RawTextComparator cmp,
- ObjectReader reader,
- RevCommit aCommit,
- RevCommit bCommit,
- ComparisonType comparisonType)
- throws IOException {
- Text aText = aCommit != null ? Text.forMergeList(comparisonType, reader, aCommit) : Text.EMPTY;
- Text bText = Text.forMergeList(comparisonType, reader, bCommit);
- return createPatchListEntry(cmp, aCommit, aText, bText, Patch.MERGE_LIST);
- }
-
- private static PatchListEntry createPatchListEntry(
- RawTextComparator cmp, RevCommit aCommit, Text aText, Text bText, String fileName) {
- byte[] rawHdr = getRawHeader(aCommit != null, fileName);
- byte[] aContent = aText.getContent();
- byte[] bContent = bText.getContent();
- long size = bContent.length;
- long sizeDelta = size - aContent.length;
- RawText aRawText = new RawText(aContent);
- RawText bRawText = new RawText(bContent);
- EditList edits = new HistogramDiff().diff(cmp, aRawText, bRawText);
- FileHeader fh = new FileHeader(rawHdr, edits, PatchType.UNIFIED);
- return new PatchListEntry(fh, edits, ImmutableSet.of(), size, sizeDelta);
- }
-
- private static byte[] getRawHeader(boolean hasA, String fileName) {
- StringBuilder hdr = new StringBuilder();
- hdr.append("diff --git");
- if (hasA) {
- hdr.append(" a/").append(fileName);
- } else {
- hdr.append(" ").append(FileHeader.DEV_NULL);
- }
- hdr.append(" b/").append(fileName);
- hdr.append("\n");
-
- if (hasA) {
- hdr.append("--- a/").append(fileName).append("\n");
- } else {
- hdr.append("--- ").append(FileHeader.DEV_NULL).append("\n");
- }
- hdr.append("+++ b/").append(fileName).append("\n");
- return hdr.toString().getBytes(UTF_8);
- }
-
- private static PatchListEntry newEntry(
- RevTree aTree, FileHeader fileHeader, Set<Edit> editsDueToRebase, long size, long sizeDelta) {
- if (aTree == null // want combined diff
- || fileHeader.getPatchType() != PatchType.UNIFIED
- || fileHeader.getHunks().isEmpty()) {
- return new PatchListEntry(fileHeader, ImmutableList.of(), ImmutableSet.of(), size, sizeDelta);
- }
-
- List<Edit> edits = fileHeader.toEditList();
- if (edits.isEmpty()) {
- return new PatchListEntry(fileHeader, ImmutableList.of(), ImmutableSet.of(), size, sizeDelta);
- }
- return new PatchListEntry(fileHeader, edits, editsDueToRebase, size, sizeDelta);
- }
-
- private RevObject aFor(
- PatchListKey key, Repository repo, RevWalk rw, InMemoryInserter ins, RevCommit b)
- throws IOException {
- if (key.getOldId() != null) {
- return rw.parseAny(key.getOldId());
- }
-
- switch (b.getParentCount()) {
- case 0:
- return rw.parseAny(emptyTree(ins));
- case 1:
- {
- RevCommit r = b.getParent(0);
- rw.parseBody(r);
- return r;
- }
- default:
- if (key.getParentNum() != null) {
- RevCommit r = b.getParent(key.getParentNum() - 1);
- rw.parseBody(r);
- return r;
- }
- // Only support auto-merge for 2 parents, not octopus merges
- if (b.getParentCount() == 2) {
- return autoMerger.lookupFromGitOrMergeInMemory(repo, rw, ins, b, mergeStrategy);
- }
- return null;
- }
- }
-
- private static ObjectId emptyTree(ObjectInserter ins) throws IOException {
- ObjectId id = ins.insert(Constants.OBJ_TREE, new byte[] {});
- ins.flush();
- return id;
- }
-
- @AutoValue
- abstract static class EditsDueToRebaseResult {
- public static EditsDueToRebaseResult create(
- List<DiffEntry> relevantDiffEntries,
- Multimap<String, ContextAwareEdit> editsDueToRebasePerFilePath) {
- return new AutoValue_PatchListLoader_EditsDueToRebaseResult(
- relevantDiffEntries, editsDueToRebasePerFilePath);
- }
-
- public abstract List<DiffEntry> getRelevantOriginalDiffEntries();
-
- /** Returns the edits per file path they modify in {@code treeB}. */
- public abstract Multimap<String, ContextAwareEdit> getEditsDueToRebasePerFilePath();
- }
-}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 95a8950..c7cd47d 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -180,8 +180,6 @@
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;
@@ -238,10 +236,6 @@
@Inject private ExtensionRegistry extensionRegistry;
@Inject
- @Named("diff")
- private Cache<PatchListKey, PatchList> fileCache;
-
- @Inject
@Named("diff_intraline")
private Cache<IntraLineDiffKey, IntraLineDiff> intraCache;
@@ -302,13 +296,10 @@
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);
diff --git a/javatests/com/google/gerrit/server/patch/PatchListTest.java b/javatests/com/google/gerrit/server/patch/PatchListTest.java
deleted file mode 100644
index 182ce49..0000000
--- a/javatests/com/google/gerrit/server/patch/PatchListTest.java
+++ /dev/null
@@ -1,97 +0,0 @@
-// Copyright (C) 2017 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.patch;
-
-import static com.google.common.truth.Truth.assertThat;
-
-import com.google.gerrit.entities.Patch;
-import com.google.gerrit.entities.Patch.ChangeType;
-import com.google.gerrit.server.patch.PatchList.ChangeTypeCmp;
-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
-import java.io.InputStream;
-import java.io.ObjectInputStream;
-import java.io.ObjectOutputStream;
-import java.util.Arrays;
-import java.util.List;
-import org.junit.Test;
-
-public class PatchListTest {
-
- @Test
- public void fileOrder() {
- String[] names = {
- "zzz",
- "def/g",
- "/!xxx",
- "abc",
- Patch.MERGE_LIST,
- "qrx",
- Patch.COMMIT_MSG,
- Patch.PATCHSET_LEVEL
- };
- String[] want = {
- Patch.COMMIT_MSG,
- Patch.MERGE_LIST,
- Patch.PATCHSET_LEVEL,
- "/!xxx",
- "abc",
- "def/g",
- "qrx",
- "zzz",
- };
- Arrays.sort(names, 0, names.length, PatchList.FILE_PATH_CMP);
- assertThat(names).isEqualTo(want);
- }
-
- @Test
- public void fileOrderNoMerge() {
- String[] names = {
- "zzz", "def/g", "/!xxx", "abc", "qrx", Patch.COMMIT_MSG,
- };
- String[] want = {
- Patch.COMMIT_MSG, "/!xxx", "abc", "def/g", "qrx", "zzz",
- };
-
- Arrays.sort(names, 0, names.length, PatchList.FILE_PATH_CMP);
- assertThat(names).isEqualTo(want);
- }
-
- @Test
- public void changeTypeOrderIsComplete() {
- List<ChangeType> changeTypeOrder = ChangeTypeCmp.order;
- ChangeType[] allTypes = ChangeType.values();
-
- Arrays.sort(allTypes, PatchList.CHANGE_TYPE_CMP);
- assertThat(changeTypeOrder).containsExactlyElementsIn(allTypes).inOrder();
- }
-
- @Test
- public void largeObjectTombstoneCanBeSerializedAndDeserialized() throws Exception {
- // Serialize
- byte[] serializedObject;
- try (ByteArrayOutputStream baos = new ByteArrayOutputStream();
- ObjectOutputStream objectStream = new ObjectOutputStream(baos)) {
- objectStream.writeObject(new PatchListCacheImpl.LargeObjectTombstone());
- serializedObject = baos.toByteArray();
- assertThat(serializedObject).isNotNull();
- }
- // Deserialize
- try (InputStream is = new ByteArrayInputStream(serializedObject);
- ObjectInputStream ois = new ObjectInputStream(is)) {
- assertThat(ois.readObject()).isInstanceOf(PatchListCacheImpl.LargeObjectTombstone.class);
- }
- }
-}
