Merge "Fix gr-check-results"
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 76e1f82..84e68ed 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -5637,7 +5637,7 @@
Email address that Gerrit refers to itself as when it creates a
new Git commit, such as a merge commit during change submission.
+
-If not set, Gerrit generates this as "gerrit@`hostname`", where
+If not set, Gerrit generates this as "gerrit@``hostname``", where
`hostname` is the hostname of the system Gerrit is running on.
+
By default, not set, generating the value at startup.
diff --git a/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java b/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java
new file mode 100644
index 0000000..b1f9726
--- /dev/null
+++ b/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java
@@ -0,0 +1,119 @@
+// Copyright (C) 2021 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.change;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static java.util.stream.Collectors.toSet;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.index.IndexConfig;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.InternalChangeQuery;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+/** Utility class that gets the ancestor changes and the descendent changes of a specific change. */
+@Singleton
+public class GetRelatedChangesUtil {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private final Provider<InternalChangeQuery> queryProvider;
+ private final RelatedChangesSorter sorter;
+ private final IndexConfig indexConfig;
+ private final ChangeData.Factory changeDataFactory;
+
+ @Inject
+ GetRelatedChangesUtil(
+ Provider<InternalChangeQuery> queryProvider,
+ RelatedChangesSorter sorter,
+ IndexConfig indexConfig,
+ ChangeData.Factory changeDataFactory) {
+ this.queryProvider = queryProvider;
+ this.sorter = sorter;
+ this.indexConfig = indexConfig;
+ this.changeDataFactory = changeDataFactory;
+ }
+
+ /**
+ * Gets related changes of a specific change revision.
+ *
+ * @param changeData the change of the inputted revision.
+ * @param basePs the revision that the method checks for related changes.
+ * @return list of related changes, sorted via {@link RelatedChangesSorter}
+ */
+ public List<RelatedChangesSorter.PatchSetData> getRelated(ChangeData changeData, PatchSet basePs)
+ throws IOException, PermissionBackendException {
+ Set<String> groups = getAllGroups(changeData.patchSets());
+ logger.atFine().log("groups = %s", groups);
+ if (groups.isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ List<ChangeData> cds =
+ InternalChangeQuery.byProjectGroups(
+ queryProvider, indexConfig, changeData.project(), groups);
+ if (cds.isEmpty()) {
+ return Collections.emptyList();
+ }
+ if (cds.size() == 1 && cds.get(0).getId().equals(changeData.getId())) {
+ return Collections.emptyList();
+ }
+
+ cds = reloadChangeIfStale(cds, changeData, basePs);
+
+ return sorter.sort(cds, basePs);
+ }
+
+ private List<ChangeData> reloadChangeIfStale(
+ List<ChangeData> changeDatasFromIndex, ChangeData wantedChange, PatchSet wantedPs) {
+ checkArgument(
+ wantedChange.getId().equals(wantedPs.id().changeId()),
+ "change of wantedPs (%s) doesn't match wantedChange (%s)",
+ wantedPs.id().changeId(),
+ wantedChange.getId());
+
+ List<ChangeData> changeDatas = new ArrayList<>(changeDatasFromIndex.size() + 1);
+ changeDatas.addAll(changeDatasFromIndex);
+
+ // Reload the change in case the patch set is absent.
+ changeDatas.stream()
+ .filter(
+ cd -> cd.getId().equals(wantedPs.id().changeId()) && cd.patchSet(wantedPs.id()) == null)
+ .forEach(ChangeData::reloadChange);
+
+ if (changeDatas.stream().noneMatch(cd -> cd.getId().equals(wantedPs.id().changeId()))) {
+ // The change of the wanted patch set is missing in the result from the index.
+ // Load it from NoteDb and add it to the result.
+ changeDatas.add(changeDataFactory.create(wantedChange.change()));
+ }
+
+ return changeDatas;
+ }
+
+ @VisibleForTesting
+ public static Set<String> getAllGroups(Collection<PatchSet> patchSets) {
+ return patchSets.stream().flatMap(ps -> ps.groups().stream()).collect(toSet());
+ }
+}
diff --git a/java/com/google/gerrit/server/restapi/change/RelatedChangesSorter.java b/java/com/google/gerrit/server/change/RelatedChangesSorter.java
similarity index 96%
rename from java/com/google/gerrit/server/restapi/change/RelatedChangesSorter.java
rename to java/com/google/gerrit/server/change/RelatedChangesSorter.java
index 0634081..547452e 100644
--- a/java/com/google/gerrit/server/restapi/change/RelatedChangesSorter.java
+++ b/java/com/google/gerrit/server/change/RelatedChangesSorter.java
@@ -11,8 +11,7 @@
// 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.restapi.change;
+package com.google.gerrit.server.change;
import static com.google.common.base.Preconditions.checkArgument;
import static java.util.Objects.requireNonNull;
@@ -30,6 +29,7 @@
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.server.change.RelatedChangesSorter.PatchSetData;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -57,7 +57,7 @@
import org.eclipse.jgit.revwalk.RevWalk;
@Singleton
-class RelatedChangesSorter {
+public class RelatedChangesSorter {
private final GitRepositoryManager repoManager;
private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
@@ -247,23 +247,23 @@
}
@AutoValue
- abstract static class PatchSetData {
+ public abstract static class PatchSetData {
@VisibleForTesting
static PatchSetData create(ChangeData cd, PatchSet ps, RevCommit commit) {
return new AutoValue_RelatedChangesSorter_PatchSetData(cd, ps, commit);
}
- abstract ChangeData data();
+ public abstract ChangeData data();
- abstract PatchSet patchSet();
+ public abstract PatchSet patchSet();
- abstract RevCommit commit();
+ public abstract RevCommit commit();
- PatchSet.Id psId() {
+ public PatchSet.Id psId() {
return patchSet().id();
}
- Change.Id id() {
+ public Change.Id id() {
return psId().changeId();
}
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/java/com/google/gerrit/server/restapi/change/GetRelated.java b/java/com/google/gerrit/server/restapi/change/GetRelated.java
index ba1a1dc..0eef468 100644
--- a/java/com/google/gerrit/server/restapi/change/GetRelated.java
+++ b/java/com/google/gerrit/server/restapi/change/GetRelated.java
@@ -14,10 +14,6 @@
package com.google.gerrit.server.restapi.change;
-import static com.google.common.base.Preconditions.checkArgument;
-import static java.util.stream.Collectors.toSet;
-
-import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Lists;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
@@ -29,56 +25,39 @@
import com.google.gerrit.extensions.common.CommitInfo;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestReadView;
-import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CommonConverters;
-import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.change.GetRelatedChangesUtil;
+import com.google.gerrit.server.change.RelatedChangesSorter;
import com.google.gerrit.server.change.RevisionResource;
-import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.inject.Inject;
-import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Locale;
-import java.util.Set;
-import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.revwalk.RevCommit;
@Singleton
public class GetRelated implements RestReadView<RevisionResource> {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- private final Provider<InternalChangeQuery> queryProvider;
- private final PatchSetUtil psUtil;
- private final RelatedChangesSorter sorter;
- private final IndexConfig indexConfig;
private final ChangeData.Factory changeDataFactory;
+ private final GetRelatedChangesUtil getRelatedChangesUtil;
@Inject
- GetRelated(
- Provider<InternalChangeQuery> queryProvider,
- PatchSetUtil psUtil,
- RelatedChangesSorter sorter,
- IndexConfig indexConfig,
- ChangeData.Factory changeDataFactory) {
- this.queryProvider = queryProvider;
- this.psUtil = psUtil;
- this.sorter = sorter;
- this.indexConfig = indexConfig;
+ GetRelated(ChangeData.Factory changeDataFactory, GetRelatedChangesUtil getRelatedChangesUtil) {
this.changeDataFactory = changeDataFactory;
+ this.getRelatedChangesUtil = getRelatedChangesUtil;
}
@Override
public Response<RelatedChangesInfo> apply(RevisionResource rsrc)
- throws RepositoryNotFoundException, IOException, NoSuchProjectException,
- PermissionBackendException {
+ throws IOException, NoSuchProjectException, PermissionBackendException {
RelatedChangesInfo relatedChangesInfo = new RelatedChangesInfo();
relatedChangesInfo.changes = getRelated(rsrc);
return Response.ok(relatedChangesInfo);
@@ -86,30 +65,15 @@
public List<RelatedChangeAndCommitInfo> getRelated(RevisionResource rsrc)
throws IOException, PermissionBackendException {
- Set<String> groups = getAllGroups(rsrc.getNotes(), psUtil);
- logger.atFine().log("groups = %s", groups);
- if (groups.isEmpty()) {
- return Collections.emptyList();
- }
-
- List<ChangeData> cds =
- InternalChangeQuery.byProjectGroups(
- queryProvider, indexConfig, rsrc.getChange().getProject(), groups);
- if (cds.isEmpty()) {
- return Collections.emptyList();
- }
- if (cds.size() == 1 && cds.get(0).getId().equals(rsrc.getChange().getId())) {
- return Collections.emptyList();
- }
- List<RelatedChangeAndCommitInfo> result = new ArrayList<>(cds.size());
-
boolean isEdit = rsrc.getEdit().isPresent();
PatchSet basePs = isEdit ? rsrc.getEdit().get().getBasePatchSet() : rsrc.getPatchSet();
logger.atFine().log("isEdit = %s, basePs = %s", isEdit, basePs);
- cds = reloadChangeIfStale(cds, rsrc.getChange(), basePs);
+ List<RelatedChangesSorter.PatchSetData> sortedResult =
+ getRelatedChangesUtil.getRelated(changeDataFactory.create(rsrc.getNotes()), basePs);
- for (RelatedChangesSorter.PatchSetData d : sorter.sort(cds, basePs)) {
+ List<RelatedChangeAndCommitInfo> result = new ArrayList<>(sortedResult.size());
+ for (RelatedChangesSorter.PatchSetData d : sortedResult) {
PatchSet ps = d.patchSet();
RevCommit commit;
if (isEdit && ps.id().equals(basePs.id())) {
@@ -134,37 +98,6 @@
return result;
}
- @VisibleForTesting
- public static Set<String> getAllGroups(ChangeNotes notes, PatchSetUtil psUtil) {
- return psUtil.byChange(notes).stream().flatMap(ps -> ps.groups().stream()).collect(toSet());
- }
-
- private List<ChangeData> reloadChangeIfStale(
- List<ChangeData> changeDatasFromIndex, Change wantedChange, PatchSet wantedPs) {
- checkArgument(
- wantedChange.getId().equals(wantedPs.id().changeId()),
- "change of wantedPs (%s) doesn't match wantedChange (%s)",
- wantedPs.id().changeId(),
- wantedChange.getId());
-
- List<ChangeData> changeDatas = new ArrayList<>(changeDatasFromIndex.size() + 1);
- changeDatas.addAll(changeDatasFromIndex);
-
- // Reload the change in case the patch set is absent.
- changeDatas.stream()
- .filter(
- cd -> cd.getId().equals(wantedPs.id().changeId()) && cd.patchSet(wantedPs.id()) == null)
- .forEach(ChangeData::reloadChange);
-
- if (changeDatas.stream().noneMatch(cd -> cd.getId().equals(wantedPs.id().changeId()))) {
- // The change of the wanted patch set is missing in the result from the index.
- // Load it from NoteDb and add it to the result.
- changeDatas.add(changeDataFactory.create(wantedChange));
- }
-
- return changeDatas;
- }
-
static RelatedChangeAndCommitInfo newChangeAndCommit(
Project.NameKey project, @Nullable Change change, @Nullable PatchSet ps, RevCommit c) {
RelatedChangeAndCommitInfo info = new RelatedChangeAndCommitInfo();
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 59011f6..4c17800 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -184,8 +184,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;
@@ -243,10 +241,6 @@
@Inject private ExtensionRegistry extensionRegistry;
@Inject
- @Named("diff")
- private Cache<PatchListKey, PatchList> fileCache;
-
- @Inject
@Named("diff_intraline")
private Cache<IntraLineDiffKey, IntraLineDiff> intraCache;
@@ -307,13 +301,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/acceptance/server/change/GetRelatedIT.java b/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
index 74dfa04..e778a5c 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
@@ -45,9 +45,9 @@
import com.google.gerrit.extensions.common.CommitInfo;
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.index.IndexConfig;
+import com.google.gerrit.server.change.GetRelatedChangesUtil;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.restapi.change.ChangesCollection;
-import com.google.gerrit.server.restapi.change.GetRelated;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -573,7 +573,7 @@
ChangeData cd = getChange(last);
assertThat(cd.patchSets()).hasSize(n);
- assertThat(GetRelated.getAllGroups(cd.notes(), psUtil)).hasSize(n);
+ assertThat(GetRelatedChangesUtil.getAllGroups(cd.notes().getPatchSets().values())).hasSize(n);
assertRelated(cd.change().currentPatchSetId());
}
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);
- }
- }
-}
diff --git a/plugins/BUILD b/plugins/BUILD
index 01ede06..0e5df2c 100644
--- a/plugins/BUILD
+++ b/plugins/BUILD
@@ -73,6 +73,7 @@
"//lib/commons:compress",
"//lib/commons:dbcp",
"//lib/commons:lang",
+ "//lib/commons:lang3",
"//lib/dropwizard:dropwizard-core",
"//lib/flogger:api",
"//lib/guice:guice",
diff --git a/plugins/plugin-manager b/plugins/plugin-manager
index ea992c3..44808dc 160000
--- a/plugins/plugin-manager
+++ b/plugins/plugin-manager
@@ -1 +1 @@
-Subproject commit ea992c3b37eed5493c7031ee20faba9dd875170f
+Subproject commit 44808dcad3c978ed12bb2cb454c6ad320912aa8a
diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts
index 78c1ebd..aaeb924 100644
--- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts
+++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts
@@ -55,7 +55,6 @@
'commentby:',
'commit:',
'committer:',
- 'conflicts:',
'deleted:',
'delta:',
'dir:',
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts b/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts
index 356a248..82af365 100644
--- a/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts
+++ b/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts
@@ -19,7 +19,7 @@
import {getRootElement} from '../../../scripts/rootElement';
import {Constructor} from '../../../utils/common-util';
import {PolymerElement} from '@polymer/polymer/polymer-element';
-import {property, observe} from '@polymer/decorators';
+import {observe, property} from '@polymer/decorators';
import {
pushScrollLock,
removeScrollLock,
@@ -134,8 +134,6 @@
this._target.addEventListener('focus', this.debounceShow);
this._target.addEventListener('mouseleave', this.debounceHide);
this._target.addEventListener('blur', this.debounceHide);
-
- // when click, dismiss immediately
this._target.addEventListener('click', this.hide);
// show the hovercard if mouse moves to hovercard
@@ -151,6 +149,11 @@
this.cancelShowTask();
this.cancelHideTask();
this.unlock();
+ this._target?.removeEventListener('mouseenter', this.debounceShow);
+ this._target?.removeEventListener('focus', this.debounceShow);
+ this._target?.removeEventListener('mouseleave', this.debounceHide);
+ this._target?.removeEventListener('blur', this.debounceHide);
+ this._target?.removeEventListener('click', this.hide);
super.disconnectedCallback();
}
@@ -160,14 +163,6 @@
this.container = getHovercardContainer({createIfNotExists: true});
}
- removeListeners() {
- this._target?.removeEventListener('mouseenter', this.debounceShow);
- this._target?.removeEventListener('focus', this.debounceShow);
- this._target?.removeEventListener('mouseleave', this.debounceHide);
- this._target?.removeEventListener('blur', this.debounceHide);
- this._target?.removeEventListener('click', this.hide);
- }
-
readonly debounceHide = () => {
this.cancelShowTask();
if (!this._isShowing || this.isScheduledToHide) return;
@@ -185,10 +180,10 @@
};
cancelHideTask() {
- if (this.hideTask) {
- this.hideTask.cancel();
- this.isScheduledToHide = false;
- }
+ if (!this.hideTask) return;
+ this.hideTask.cancel();
+ this.isScheduledToHide = false;
+ this.hideTask = undefined;
}
/**
@@ -315,10 +310,10 @@
}
cancelShowTask() {
- if (this.showTask) {
- this.showTask.cancel();
- this.isScheduledToShow = false;
- }
+ if (!this.showTask) return;
+ this.showTask.cancel();
+ this.isScheduledToShow = false;
+ this.showTask = undefined;
}
/**
@@ -332,7 +327,7 @@
* Shows/opens the hovercard. This occurs when the user triggers the
* `mousenter` event on the hovercard's `target` element.
*/
- readonly show = () => {
+ readonly show = async () => {
this.cancelHideTask();
this.cancelShowTask();
if (this._isShowing || !this.container) {
@@ -352,7 +347,7 @@
// Make sure that the hovercard actually rendered and all dom-if
// statements processed, so that we can measure the (invisible)
// hovercard properly in updatePosition().
- flush();
+ await flush();
this.updatePosition();
this.classList.remove(HIDE_CLASS);
};
@@ -473,9 +468,6 @@
_target: HTMLElement | null;
_isShowing: boolean;
ready(): void;
- removeListeners(): void;
- debounceHide(): void;
- cancelHideTask(): void;
dispatchEventThroughTarget(eventName: string, detail?: unknown): void;
hide(e?: MouseEvent): void;
debounceShow(): void;
@@ -483,5 +475,4 @@
cancelShowTask(): void;
show(): void;
updatePosition(): void;
- updatePositionTo(position: string): void;
}
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard_test.js b/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard_test.js
index 27ef23f..d5e0061 100644
--- a/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard_test.js
+++ b/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard_test.js
@@ -84,8 +84,8 @@
assert.notEqual(element.container, element.parentNode);
});
- test('show', () => {
- element.show({});
+ test('show', async () => {
+ await element.show({});
const style = getComputedStyle(element);
assert.isTrue(element._isShowing);
assert.isTrue(element.classList.contains('hovered'));
@@ -120,6 +120,7 @@
button.dispatchEvent(new CustomEvent('mouseenter'));
await enterPromise;
+ await flush();
assert.isTrue(element.isScheduledToShow);
element.showTask.flush();
assert.isTrue(element._isShowing);
@@ -152,6 +153,7 @@
button.dispatchEvent(new CustomEvent('mouseenter'));
await enterPromise;
+ await flush();
assert.isTrue(element.isScheduledToShow);
MockInteractions.tap(button);
