CodeOwnerApprovalCheck: Allow to use diff cache to get changed files
To know if a change has sufficient code owner approvals to be
submittable or to compute the code owner file statuses we must know
which files were touched in a change. Since the PatchListCache was known
to be slow and having issues, the code-owners plugin implemented its own
changed file computation.
The disadvantages of this computation are:
- it is redone on each access (no caching)
- rename detection is switched off (as it is too expensive to be done on
each access)
By now a new diff cache has been implemented and we want to make use of
it.
Using the new diff cache will enable the rename detection. This allows
us to report a single code owner status for renamed files now. This is
important because it means that the number of returned code owner
statuses will now match the number of changed files that are shown on
the change screen, and also the order will be consistent (that was not
the case when renamend files were reported as 2 code owner statuses, one
status for the old path and one status for the new path). In a follow-up
change we can make use of this and limit the number of returned code
owner statuses for large changes. For large changes the change screen
only shows the first 200 files. In this case it is sufficient to return
only the first 200 code owner statuses so that the files that are shown
on the change screen are covered.
Unfortunately we cannot use the new diff cache to get changed files in
all places yet:
- If changed files are computed to know which OWNERS files need to be
validated it may be that the commit that should be validated was newly
created and hence is only available from a specific RevWalk instance.
Passing in this RevWalk instance into the diff cache is not possible
and using another RevWalk instance would not find the commit and fail
with MissingObjectException. The same problem applies with the old
diff cache.
- The diff cache doesn't support getting changed files for initial
commits (commits without parent commits).
Latency-wise it should be fine using the diff cache (with rename
detection being enabled) since the results are cached and any time a
change screen is viewed the cache is already being accessed.
Since the new diff cache still has a few issues, by default it is not
used yet, but there is an experiment flag that allows us to enable using
it. This way we can also safely roll this out when the new diff cache is
ready to be used.
Change-Id: Ifab1139e3e691523a5b0365eb271fdc7089b4c30
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
index f03d337..2835935 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
@@ -14,12 +14,16 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
+import static java.util.Comparator.comparing;
import static java.util.Objects.requireNonNull;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.Project;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
@@ -28,16 +32,22 @@
import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.experiments.ExperimentFeatures;
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.DiffNotAvailableException;
+import com.google.gerrit.server.patch.DiffOperations;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.List;
+import java.util.Map;
+import java.util.stream.Stream;
import org.eclipse.jgit.diff.DiffEntry;
import org.eclipse.jgit.diff.DiffFormatter;
import org.eclipse.jgit.diff.RawTextComparator;
@@ -51,12 +61,21 @@
import org.eclipse.jgit.util.io.DisabledOutputStream;
/**
- * Class to compute the files that have been changed in a revision.
+ * Class to get/compute the files that have been changed in a revision.
*
- * <p>The file diff is newly computed on each access and not retrieved from any cache. This is
- * better than using {@link com.google.gerrit.server.patch.PatchListCache} which does a lot of
- * unneeded computations and hence is slower. The Gerrit diff caches are currently being redesigned.
- * Once the envisioned {@code ModifiedFilesCache} is available we should consider using it.
+ * <p>The {@link #getFromDiffCache(Project.NameKey, ObjectId)} method is retrieving the file diff
+ * from the diff cache and has rename detection enabled.
+ *
+ * <p>In contrast to this, for the {@code compute} methods the file diff is newly computed on each
+ * access and rename detection is disabled (as it's too expensive to do it on each access).
+ *
+ * <p>If possible, using {@link #getFromDiffCache(Project.NameKey, ObjectId)} is preferred, however
+ * {@link #getFromDiffCache(Project.NameKey, ObjectId)} cannot be used for newly created commits
+ * that are only available from a specific {@link RevWalk} instance since the {@link RevWalk}
+ * instance cannot be passed in.
+ *
+ * <p>The {@link com.google.gerrit.server.patch.PatchListCache} is deprecated, and hence it not
+ * being used here.
*/
@Singleton
public class ChangedFiles {
@@ -66,29 +85,64 @@
private final GitRepositoryManager repoManager;
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
+ private final DiffOperations diffOperations;
private final Provider<AutoMerger> autoMergerProvider;
private final CodeOwnerMetrics codeOwnerMetrics;
private final ThreeWayMergeStrategy mergeStrategy;
+ private final ExperimentFeatures experimentFeatures;
@Inject
public ChangedFiles(
@GerritServerConfig Config cfg,
GitRepositoryManager repoManager,
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
+ DiffOperations diffOperations,
Provider<AutoMerger> autoMergerProvider,
- CodeOwnerMetrics codeOwnerMetrics) {
+ CodeOwnerMetrics codeOwnerMetrics,
+ ExperimentFeatures experimentFeatures) {
this.repoManager = repoManager;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
+ this.diffOperations = diffOperations;
this.autoMergerProvider = autoMergerProvider;
this.codeOwnerMetrics = codeOwnerMetrics;
+ this.experimentFeatures = experimentFeatures;
this.mergeStrategy = MergeUtil.getMergeStrategy(cfg);
}
/**
+ * Returns the changed files for the given revision.
+ *
+ * <p>By default the changed files are computed on access (see {@link #compute(Project.NameKey,
+ * ObjectId)}).
+ *
+ * <p>Only if enabled via the {@link CodeOwnersExperimentFeaturesConstants#USE_DIFF_CACHE}
+ * experiment feature flag the changed files are retrieved from the diff cache (see {@link
+ * #getFromDiffCache(Project.NameKey, ObjectId)}).
+ *
+ * @param project the project
+ * @param revision the revision for which the changed files should be computed
+ * @return the files that have been changed in the given revision, sorted alphabetically by path
+ */
+ public ImmutableList<ChangedFile> getOrCompute(Project.NameKey project, ObjectId revision)
+ throws IOException, PatchListNotAvailableException, DiffNotAvailableException {
+ if (experimentFeatures.isFeatureEnabled(CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)) {
+ if (isInitialCommit(project, revision)) {
+ // DiffOperations doesn't support getting the list of modified files for the initial commit.
+ return compute(project, revision);
+ }
+
+ return getFromDiffCache(project, revision);
+ }
+ return compute(project, revision);
+ }
+
+ /**
* Computes the files that have been changed in the given revision.
*
* <p>The diff is computed against the parent commit.
*
+ * <p>Rename detection is disabled.
+ *
* @param revisionResource the revision resource for which the changed files should be computed
* @return the files that have been changed in the given revision, sorted alphabetically by path
* @throws IOException thrown if the computation fails due to an I/O error
@@ -106,6 +160,8 @@
*
* <p>The diff is computed against the parent commit.
*
+ * <p>Rename detection is disabled.
+ *
* @param project the project
* @param revision the revision for which the changed files should be computed
* @return the files that have been changed in the given revision, sorted alphabetically by path
@@ -180,6 +236,8 @@
*
* <p>The computation also works if the commit doesn't have any parent.
*
+ * <p>Rename detection is disabled.
+ *
* @param repoConfig the repository configuration
* @param revWalk the rev walk
* @param commit the commit for which the changed files should be computed
@@ -215,4 +273,68 @@
}
}
}
+
+ /**
+ * Gets the changed files from the diff cache.
+ *
+ * <p>Doesn't support getting changed files for an initial revision. This is because the diff
+ * cache doesn't support getting changed files for commits that don't have any parent.
+ *
+ * <p>Rename detection is enabled.
+ *
+ * @throws IllegalStateException thrown if invoked for an initial revision
+ */
+ @VisibleForTesting
+ ImmutableList<ChangedFile> getFromDiffCache(Project.NameKey project, ObjectId revision)
+ throws IOException, DiffNotAvailableException {
+ requireNonNull(project, "project");
+ requireNonNull(revision, "revision");
+
+ checkState(!isInitialCommit(project, revision), "diff cache doesn't support initial commits");
+
+ MergeCommitStrategy mergeCommitStrategy =
+ codeOwnersPluginConfiguration.getProjectConfig(project).getMergeCommitStrategy();
+
+ try (Timer0.Context ctx = codeOwnerMetrics.getChangedFiles.start()) {
+ Map<String, FileDiffOutput> fileDiffOutputs;
+ if (mergeCommitStrategy.equals(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION)) {
+ // Use parentNum=null to do the comparison against the default base.
+ // For non-merge commits the default base is the only parent (aka parent 1, initial commits
+ // are not supported).
+ // For merge commits the default base is the auto-merge commit which should be used as base
+ // if the merge commit strategy is FILES_WITH_CONFLICT_RESOLUTION.
+ fileDiffOutputs =
+ diffOperations.listModifiedFilesAgainstParent(project, revision, /* parentNum=*/ null);
+ } else {
+ checkState(mergeCommitStrategy.equals(MergeCommitStrategy.ALL_CHANGED_FILES));
+ // Always use parent 1 to do the comparison.
+ // Non-merge commits should always be compared against against the first parent (initial
+ // commits are not supported).
+ // For merge commits also the first parent should be used if the merge commit strategy is
+ // ALL_CHANGED_FILES.
+ fileDiffOutputs = diffOperations.listModifiedFilesAgainstParent(project, revision, 1);
+ }
+
+ return toChangedFiles(filterOutMagicFilesAndSort(fileDiffOutputs)).collect(toImmutableList());
+ }
+ }
+
+ private boolean isInitialCommit(Project.NameKey project, ObjectId objectId) throws IOException {
+ try (Repository repo = repoManager.openRepository(project);
+ RevWalk revWalk = new RevWalk(repo)) {
+ return revWalk.parseCommit(objectId).getParentCount() == 0;
+ }
+ }
+
+ private Stream<Map.Entry<String, FileDiffOutput>> filterOutMagicFilesAndSort(
+ Map<String, FileDiffOutput> fileDiffOutputs) {
+ return fileDiffOutputs.entrySet().stream()
+ .filter(e -> !Patch.isMagic(e.getKey()))
+ .sorted(comparing(Map.Entry::getKey));
+ }
+
+ private Stream<ChangedFile> toChangedFiles(
+ Stream<Map.Entry<String, FileDiffOutput>> fileDiffOutputs) {
+ return fileDiffOutputs.map(Map.Entry::getValue).map(ChangedFile::create);
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 792087d..236e62a 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -44,6 +44,7 @@
import com.google.gerrit.server.git.PureRevertCache;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -158,7 +159,7 @@
ownedPaths = ownedPaths.limit(limit);
}
return ownedPaths.collect(toImmutableList());
- } catch (IOException | PatchListNotAvailableException e) {
+ } catch (IOException | PatchListNotAvailableException | DiffNotAvailableException e) {
throw new CodeOwnersInternalServerErrorException(
String.format(
"failed to compute owned paths of patch set %s for account %d",
@@ -174,7 +175,8 @@
* @return whether the given change has sufficient code owner approvals to be submittable
*/
public boolean isSubmittable(ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, PatchListNotAvailableException {
+ throws ResourceConflictException, IOException, PatchListNotAvailableException,
+ DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
logger.atFine().log(
"checking if change %d in project %s is submittable",
@@ -212,7 +214,8 @@
* @see #getFileStatuses(CodeOwnerConfigHierarchy, ChangeNotes)
*/
public ImmutableSet<FileCodeOwnerStatus> getFileStatusesAsSet(ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, PatchListNotAvailableException {
+ throws ResourceConflictException, IOException, PatchListNotAvailableException,
+ DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatuses.start()) {
logger.atFine().log(
@@ -253,7 +256,8 @@
*/
private Stream<FileCodeOwnerStatus> getFileStatuses(
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy, ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, PatchListNotAvailableException {
+ throws ResourceConflictException, IOException, PatchListNotAvailableException,
+ DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
try (Timer0.Context ctx = codeOwnerMetrics.prepareFileStatusComputation.start()) {
logger.atFine().log(
@@ -326,7 +330,7 @@
FallbackCodeOwners fallbackCodeOwners = codeOwnersConfig.getFallbackCodeOwners();
return changedFiles
- .compute(changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId())
+ .getOrCompute(changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId())
.stream()
.map(
changedFile ->
@@ -360,7 +364,8 @@
@VisibleForTesting
public Stream<FileCodeOwnerStatus> getFileStatusesForAccount(
ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId)
- throws ResourceConflictException, IOException, PatchListNotAvailableException {
+ throws ResourceConflictException, IOException, PatchListNotAvailableException,
+ DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
requireNonNull(patchSet, "patchSet");
requireNonNull(accountId, "accountId");
@@ -392,7 +397,7 @@
}
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy = codeOwnerConfigHierarchyProvider.get();
- return changedFiles.compute(changeNotes.getProjectName(), patchSet.commitId()).stream()
+ return changedFiles.getOrCompute(changeNotes.getProjectName(), patchSet.commitId()).stream()
.map(
changedFile ->
getFileStatus(
@@ -429,8 +434,8 @@
private Stream<FileCodeOwnerStatus> getAllPathsAsApproved(
ChangeNotes changeNotes, PatchSet patchSet)
- throws IOException, PatchListNotAvailableException {
- return changedFiles.compute(changeNotes.getProjectName(), patchSet.commitId()).stream()
+ throws IOException, PatchListNotAvailableException, DiffNotAvailableException {
+ return changedFiles.getOrCompute(changeNotes.getProjectName(), patchSet.commitId()).stream()
.map(
changedFile ->
FileCodeOwnerStatus.create(
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
index ea0f38a..7f07943 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
@@ -27,6 +27,7 @@
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.rules.SubmitRule;
@@ -156,7 +157,8 @@
}
private SubmitRecord getSubmitRecord(ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, PatchListNotAvailableException {
+ throws ResourceConflictException, IOException, PatchListNotAvailableException,
+ DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
return codeOwnerApprovalCheck.isSubmittable(changeNotes) ? ok() : notReady();
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExperimentFeaturesConstants.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExperimentFeaturesConstants.java
new file mode 100644
index 0000000..a0e82ad
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExperimentFeaturesConstants.java
@@ -0,0 +1,38 @@
+// 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.plugins.codeowners.backend;
+
+/**
+ * Constants for {@link com.google.gerrit.server.experiments.ExperimentFeatures} in the code-owners
+ * plugin.
+ */
+public final class CodeOwnersExperimentFeaturesConstants {
+ /**
+ * Whether {@link com.google.gerrit.server.patch.DiffOperations}, and thus the diff cache, should
+ * be used to get changed files, instead of computing the changed files on our own.
+ *
+ * @see ChangedFiles#getOrCompute(com.google.gerrit.entities.Project.NameKey,
+ * org.eclipse.jgit.lib.ObjectId)
+ */
+ public static final String USE_DIFF_CACHE =
+ "GerritBackendRequestFeature__code_owners_use_diff_cache";
+
+ /**
+ * Private constructor to prevent instantiation of this class.
+ *
+ * <p>The class only contains static fields, hence the class never needs to be instantiated.
+ */
+ private CodeOwnersExperimentFeaturesConstants() {}
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/common/ChangedFile.java b/java/com/google/gerrit/plugins/codeowners/common/ChangedFile.java
index f66c31b..267e3c7 100644
--- a/java/com/google/gerrit/plugins/codeowners/common/ChangedFile.java
+++ b/java/com/google/gerrit/plugins/codeowners/common/ChangedFile.java
@@ -24,6 +24,7 @@
import com.google.gerrit.entities.Patch;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.gerrit.server.patch.PatchListEntry;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import java.nio.file.Path;
import java.util.Optional;
import org.eclipse.jgit.diff.DiffEntry;
@@ -162,6 +163,26 @@
return Optional.ofNullable(path).map(newName -> JgitPath.of(newName).getAsAbsolutePath());
}
+ /**
+ * Creates a {@link ChangedFile} instance from a {@link FileDiffOutput}.
+ *
+ * @param fileDiffOutput the file diff output
+ */
+ public static ChangedFile create(FileDiffOutput fileDiffOutput) {
+ requireNonNull(fileDiffOutput, "fileDiffOutput");
+
+ return new AutoValue_ChangedFile(
+ convertPathFromFileDiffOutput(fileDiffOutput.newPath()),
+ convertPathFromFileDiffOutput(fileDiffOutput.oldPath()),
+ CHANGE_TYPE.get(fileDiffOutput.changeType()));
+ }
+
+ /** Converts the given string path to an absolute path. */
+ private static Optional<Path> convertPathFromFileDiffOutput(Optional<String> path) {
+ requireNonNull(path, "path");
+ return path.map(p -> JgitPath.of(p).getAsAbsolutePath());
+ }
+
public static ChangedFile create(
Optional<String> newPath, Optional<String> oldPath, ChangeType changeType) {
requireNonNull(changeType, "changeType");
diff --git a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
index 4e952be..cdb8174 100644
--- a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
+++ b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
@@ -40,6 +40,7 @@
public final Timer0 computePatchSetApprovals;
public final Timer0 extendChangeMessageOnPostReview;
public final Timer0 getAutoMerge;
+ public final Timer0 getChangedFiles;
public final Timer0 prepareFileStatusComputation;
public final Timer0 prepareFileStatusComputationForAccount;
public final Timer0 resolveCodeOwnerConfig;
@@ -100,6 +101,9 @@
this.getAutoMerge =
createLatencyTimer(
"get_auto_merge", "Latency for getting the auto merge commit of a merge commit");
+ this.getChangedFiles =
+ createLatencyTimer(
+ "get_changed_files", "Latency for getting changed files from diff cache");
this.prepareFileStatusComputation =
createLatencyTimer(
"prepare_file_status_computation", "Latency for preparing the file status computation");
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnerStatus.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnerStatus.java
index fdf7bd7..34a1dfe 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnerStatus.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnerStatus.java
@@ -22,6 +22,7 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerApprovalCheck;
import com.google.gerrit.plugins.codeowners.backend.FileCodeOwnerStatus;
import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.inject.Inject;
@@ -60,7 +61,7 @@
@Override
public Response<CodeOwnerStatusInfo> apply(ChangeResource changeResource)
throws RestApiException, IOException, PermissionBackendException,
- PatchListNotAvailableException {
+ PatchListNotAvailableException, DiffNotAvailableException {
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(changeResource.getNotes());
return Response.ok(
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
index 79f235f..04eb2fd 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
@@ -19,9 +19,11 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
import com.google.gerrit.plugins.codeowners.api.CodeOwnerStatusInfo;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnersExperimentFeaturesConstants;
import com.google.gerrit.plugins.codeowners.backend.FileCodeOwnerStatus;
import com.google.gerrit.plugins.codeowners.common.CodeOwnerStatus;
import com.google.inject.Inject;
@@ -69,6 +71,18 @@
@Test
public void getStatusForRenamedFile() throws Exception {
+ testGetStatusForRenamedFile(/* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForRenamedFile_useDiffCache() throws Exception {
+ testGetStatusForRenamedFile(/* useDiffCache= */ true);
+ }
+
+ private void testGetStatusForRenamedFile(boolean useDiffCache) throws Exception {
TestAccount user2 = accountCreator.user2();
setAsCodeOwners("/foo/bar/", user);
@@ -80,34 +94,67 @@
CodeOwnerStatusInfo codeOwnerStatus =
changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
- assertThat(codeOwnerStatus)
- .hasFileCodeOwnerStatusesThat()
- .comparingElementsUsing(isFileCodeOwnerStatus())
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ if (useDiffCache) {
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ } else {
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
// Add a reviewer that is a code owner of the old path.
gApi.changes().id(changeId).addReviewer(user.email());
codeOwnerStatus = changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
- assertThat(codeOwnerStatus)
- .hasFileCodeOwnerStatusesThat()
- .comparingElementsUsing(isFileCodeOwnerStatus())
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.PENDING),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ if (useDiffCache) {
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.PENDING,
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ } else {
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.PENDING),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
// Add a reviewer that is a code owner of the new path.
gApi.changes().id(changeId).addReviewer(user2.email());
codeOwnerStatus = changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
- assertThat(codeOwnerStatus)
- .hasFileCodeOwnerStatusesThat()
- .comparingElementsUsing(isFileCodeOwnerStatus())
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.PENDING),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.PENDING));
+ if (useDiffCache) {
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath, CodeOwnerStatus.PENDING, newPath, CodeOwnerStatus.PENDING));
+ } else {
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.PENDING),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.PENDING));
+ }
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java
index a1ac734..2c84634 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java
@@ -24,7 +24,9 @@
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.common.ChangedFile;
import com.google.gerrit.server.patch.PatchListEntry;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import java.nio.file.Paths;
+import java.util.Optional;
import org.eclipse.jgit.diff.DiffEntry;
import org.eclipse.jgit.diff.DiffEntry.ChangeType;
import org.junit.Rule;
@@ -40,6 +42,7 @@
@Mock private DiffEntry diffEntry;
@Mock private PatchListEntry patchListEntry;
+ @Mock private FileDiffOutput fileDiffOutput;
@Test
public void getNewPath_diffEntry() throws Exception {
@@ -58,6 +61,15 @@
}
@Test
+ public void getNewPath_fileDiffOutput() throws Exception {
+ String newPath = "foo/bar/baz.txt";
+ setupFileDiffOutput(newPath, /* oldPath= */ null, Patch.ChangeType.ADDED);
+ assertThat(ChangedFile.create(fileDiffOutput).newPath())
+ .value()
+ .isEqualTo(Paths.get("/" + newPath));
+ }
+
+ @Test
public void getNewPathWhenNewPathIsNotSet_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.ADD);
assertThat(ChangedFile.create(diffEntry).newPath()).isEmpty();
@@ -70,6 +82,12 @@
}
@Test
+ public void getNewPathWhenNewPathIsNotSet_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.ADDED);
+ assertThat(ChangedFile.create(fileDiffOutput).newPath()).isEmpty();
+ }
+
+ @Test
public void hasNewPath_diffEntry() throws Exception {
String newPath = "foo/bar/baz.txt";
setupDiffEntry(newPath, /* oldPath= */ null, ChangeType.ADD);
@@ -90,6 +108,16 @@
}
@Test
+ public void hasNewPath_fileDiffOutput() throws Exception {
+ String newPath = "foo/bar/baz.txt";
+ setupFileDiffOutput(newPath, /* oldPath= */ null, Patch.ChangeType.ADDED);
+
+ ChangedFile changedFile = ChangedFile.create(fileDiffOutput);
+ assertThat(changedFile.hasNewPath(Paths.get("/" + newPath))).isTrue();
+ assertThat(changedFile.hasNewPath(Paths.get("/otherPath"))).isFalse();
+ }
+
+ @Test
public void cannotCheckHasNewPathWithNull_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.ADD);
@@ -112,6 +140,17 @@
}
@Test
+ public void cannotCheckHasNewPathWithNull_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.ADDED);
+
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> ChangedFile.create(fileDiffOutput).hasNewPath(/* absolutePath= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("absolutePath");
+ }
+
+ @Test
public void cannotCheckHasNewPathWithRelativePath_diffEntry() throws Exception {
String relativePath = "foo/bar/baz.txt";
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.ADD);
@@ -138,6 +177,19 @@
}
@Test
+ public void cannotCheckHasNewPathWithRelativePath_fileDiffOutput() throws Exception {
+ String relativePath = "foo/bar/baz.txt";
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.ADDED);
+ IllegalStateException exception =
+ assertThrows(
+ IllegalStateException.class,
+ () -> ChangedFile.create(fileDiffOutput).hasNewPath(Paths.get(relativePath)));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(String.format("path %s must be absolute", relativePath));
+ }
+
+ @Test
public void getOldPath_diffEntry() throws Exception {
String oldPath = "foo/bar/baz.txt";
setupDiffEntry(/* newPath= */ null, oldPath, ChangeType.DELETE);
@@ -154,6 +206,15 @@
}
@Test
+ public void getOldPath_fileDiffOutput() throws Exception {
+ String oldPath = "foo/bar/baz.txt";
+ setupFileDiffOutput(/* newPath= */ null, oldPath, Patch.ChangeType.DELETED);
+ assertThat(ChangedFile.create(fileDiffOutput).oldPath())
+ .value()
+ .isEqualTo(Paths.get("/" + oldPath));
+ }
+
+ @Test
public void getOldPathWhenOldPathIsNotSet_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.DELETE);
when(diffEntry.getOldPath()).thenReturn(DiffEntry.DEV_NULL);
@@ -167,6 +228,12 @@
}
@Test
+ public void getOldPathWhenOldPathIsNotSet_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.DELETED);
+ assertThat(ChangedFile.create(fileDiffOutput).oldPath()).isEmpty();
+ }
+
+ @Test
public void hasOldPath_diffEntry() throws Exception {
String oldPath = "foo/bar/baz.txt";
setupDiffEntry(/* newPath= */ null, oldPath, ChangeType.DELETE);
@@ -187,6 +254,16 @@
}
@Test
+ public void hasOldPath_fileDiffOutput() throws Exception {
+ String oldPath = "foo/bar/baz.txt";
+ setupFileDiffOutput(/* newPath= */ null, oldPath, Patch.ChangeType.DELETED);
+
+ ChangedFile changedFile = ChangedFile.create(fileDiffOutput);
+ assertThat(changedFile.hasOldPath(Paths.get("/" + oldPath))).isTrue();
+ assertThat(changedFile.hasOldPath(Paths.get("/otherPath"))).isFalse();
+ }
+
+ @Test
public void cannotCheckHasOldPathWithNull_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.DELETE);
NullPointerException npe =
@@ -207,6 +284,16 @@
}
@Test
+ public void cannotCheckHasOldPathWithNull_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.DELETED);
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> ChangedFile.create(fileDiffOutput).hasOldPath(/* absolutePath= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("absolutePath");
+ }
+
+ @Test
public void cannotCheckHasOldPathWithRelativePath_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.DELETE);
String relativePath = "foo/bar/baz.txt";
@@ -233,6 +320,19 @@
}
@Test
+ public void cannotCheckHasOldPathWithRelativePath_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.DELETED);
+ String relativePath = "foo/bar/baz.txt";
+ IllegalStateException exception =
+ assertThrows(
+ IllegalStateException.class,
+ () -> ChangedFile.create(fileDiffOutput).hasOldPath(Paths.get(relativePath)));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(String.format("path %s must be absolute", relativePath));
+ }
+
+ @Test
public void isRename_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.RENAME);
ChangedFile changedFile = ChangedFile.create(diffEntry);
@@ -249,6 +349,14 @@
}
@Test
+ public void isRename_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.RENAMED);
+ ChangedFile changedFile = ChangedFile.create(fileDiffOutput);
+ assertThat(changedFile.isRename()).isTrue();
+ assertThat(changedFile.isDeletion()).isFalse();
+ }
+
+ @Test
public void isDeletion_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.DELETE);
ChangedFile changedFile = ChangedFile.create(diffEntry);
@@ -265,6 +373,14 @@
}
@Test
+ public void isDeletion_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.DELETED);
+ ChangedFile changedFile = ChangedFile.create(fileDiffOutput);
+ assertThat(changedFile.isRename()).isFalse();
+ assertThat(changedFile.isDeletion()).isTrue();
+ }
+
+ @Test
public void isAddition_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.ADD);
ChangedFile changedFile = ChangedFile.create(diffEntry);
@@ -281,6 +397,14 @@
}
@Test
+ public void isAddition_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.ADDED);
+ ChangedFile changedFile = ChangedFile.create(fileDiffOutput);
+ assertThat(changedFile.isRename()).isFalse();
+ assertThat(changedFile.isDeletion()).isFalse();
+ }
+
+ @Test
public void isModify_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.MODIFY);
ChangedFile changedFile = ChangedFile.create(diffEntry);
@@ -297,6 +421,14 @@
}
@Test
+ public void isModify_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.MODIFIED);
+ ChangedFile changedFile = ChangedFile.create(fileDiffOutput);
+ assertThat(changedFile.isRename()).isFalse();
+ assertThat(changedFile.isDeletion()).isFalse();
+ }
+
+ @Test
public void isCopy_diffEntry() throws Exception {
setupDiffEntry(/* newPath= */ null, /* oldPath= */ null, ChangeType.COPY);
ChangedFile changedFile = ChangedFile.create(diffEntry);
@@ -312,6 +444,14 @@
assertThat(changedFile.isDeletion()).isFalse();
}
+ @Test
+ public void isCopy_fileDiffOutput() throws Exception {
+ setupFileDiffOutput(/* newPath= */ null, /* oldPath= */ null, Patch.ChangeType.COPIED);
+ ChangedFile changedFile = ChangedFile.create(fileDiffOutput);
+ assertThat(changedFile.isRename()).isFalse();
+ assertThat(changedFile.isDeletion()).isFalse();
+ }
+
private void setupDiffEntry(
@Nullable String newPath, @Nullable String oldPath, ChangeType changeType) {
when(diffEntry.getNewPath()).thenReturn(newPath != null ? newPath : DiffEntry.DEV_NULL);
@@ -331,4 +471,11 @@
when(patchListEntry.getChangeType()).thenReturn(changeType);
}
}
+
+ private void setupFileDiffOutput(
+ @Nullable String newPath, @Nullable String oldPath, Patch.ChangeType changeType) {
+ when(fileDiffOutput.newPath()).thenReturn(Optional.ofNullable(newPath));
+ when(fileDiffOutput.oldPath()).thenReturn(Optional.ofNullable(oldPath));
+ when(fileDiffOutput.changeType()).thenReturn(changeType);
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
index 8e70676..4534df2 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
@@ -47,6 +47,7 @@
import com.google.inject.Inject;
import java.nio.file.Paths;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Before;
import org.junit.Test;
@@ -335,7 +336,7 @@
}
@Test
- public void sortedByPath() throws Exception {
+ public void computeReturnsChangedFilesSortedByPath() throws Exception {
String file1 = "foo/bar.baz";
String file2 = "foo/baz.bar";
String file3 = "bar/foo.baz";
@@ -367,19 +368,23 @@
@Test
@GerritConfig(name = "plugin.code-owners.mergeCommitStrategy", value = "ALL_CHANGED_FILES")
- public void sortedByPath_mergeCommitAgainstFirstParent() throws Exception {
- testSortedByPathForMerge(MergeCommitStrategy.ALL_CHANGED_FILES);
+ public void computeReturnsChangedFilesSortedByPath_mergeCommitAgainstFirstParent()
+ throws Exception {
+ testComputeReturnsChangedFilesSortedByPathForMerge(MergeCommitStrategy.ALL_CHANGED_FILES);
}
@Test
@GerritConfig(
name = "plugin.code-owners.mergeCommitStrategy",
value = "FILES_WITH_CONFLICT_RESOLUTION")
- public void sortedByPath_mergeCommitAgainstAutoMerge() throws Exception {
- testSortedByPathForMerge(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION);
+ public void computeReturnsChangedFilesSortedByPath_mergeCommitAgainstAutoMerge()
+ throws Exception {
+ testComputeReturnsChangedFilesSortedByPathForMerge(
+ MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION);
}
- private void testSortedByPathForMerge(MergeCommitStrategy mergeCommitStrategy) throws Exception {
+ private void testComputeReturnsChangedFilesSortedByPathForMerge(
+ MergeCommitStrategy mergeCommitStrategy) throws Exception {
setAsRootCodeOwners(admin);
String file1 = "foo/bar.baz";
@@ -481,6 +486,416 @@
}
}
+ @Test
+ public void cannotGetFromDiffCacheForNullProject() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> changedFiles.getFromDiffCache(/* project= */ null, ObjectId.zeroId()));
+ assertThat(npe).hasMessageThat().isEqualTo("project");
+ }
+
+ @Test
+ public void cannotGetFromDiffCacheForNullRevision() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> changedFiles.getFromDiffCache(project, /* revision= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("revision");
+ }
+
+ @Test
+ public void getFromDiffCacheForChangeThatAddedAFile() throws Exception {
+ String path = "/foo/bar/baz.txt";
+ RevCommit commit =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getCommit();
+
+ ImmutableList<ChangedFile> changedFilesSet = changedFiles.getFromDiffCache(project, commit);
+ assertThat(changedFilesSet).hasSize(1);
+ ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
+ assertThat(changedFile).hasNewPath().value().isEqualTo(Paths.get(path));
+ assertThat(changedFile).hasOldPath().isEmpty();
+ assertThat(changedFile).isNoRename();
+ assertThat(changedFile).isNoDeletion();
+ }
+
+ @Test
+ public void getFromDiffCacheForChangeThatModifiedAFile() throws Exception {
+ String path = "/foo/bar/baz.txt";
+ createChange("Test Change", JgitPath.of(path).get(), "file content").getChangeId();
+
+ RevCommit commit =
+ createChange("Change Modifying A File", JgitPath.of(path).get(), "new file content")
+ .getCommit();
+
+ ImmutableList<ChangedFile> changedFilesSet = changedFiles.getFromDiffCache(project, commit);
+ assertThat(changedFilesSet).hasSize(1);
+ ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
+ assertThat(changedFile).hasNewPath().value().isEqualTo(Paths.get(path));
+ assertThat(changedFile).hasOldPath().value().isEqualTo(Paths.get(path));
+ assertThat(changedFile).isNoRename();
+ assertThat(changedFile).isNoDeletion();
+ }
+
+ @Test
+ public void getFromDiffCacheForChangeThatDeletedAFile() throws Exception {
+ String path = "/foo/bar/baz.txt";
+ String changeId = createChangeWithFileDeletion(path);
+
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.getFromDiffCache(
+ project, getRevisionResource(changeId).getPatchSet().commitId());
+ assertThat(changedFilesSet).hasSize(1);
+ ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
+ assertThat(changedFile).hasNewPath().isEmpty();
+ assertThat(changedFile).hasOldPath().value().isEqualTo(Paths.get(path));
+ assertThat(changedFile).isNoRename();
+ assertThat(changedFile).isDeletion();
+ }
+
+ @Test
+ public void getFromDiffCacheForChangeThatRenamedAFile() throws Exception {
+ String oldPath = "/foo/bar/old.txt";
+ String newPath = "/foo/bar/new.txt";
+ String changeId = createChangeWithFileRename(oldPath, newPath);
+
+ gApi.changes().id(changeId).current().files();
+
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.getFromDiffCache(
+ project, getRevisionResource(changeId).getPatchSet().commitId());
+ ChangedFileSubject changedFile = assertThatCollection(changedFilesSet).onlyElement();
+ changedFile.hasNewPath().value().isEqualTo(Paths.get(newPath));
+ changedFile.hasOldPath().value().isEqualTo(Paths.get(oldPath));
+ changedFile.isRename();
+ changedFile.isNoDeletion();
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void getFromDiffCacheForInitialChangeThatAddedAFile() throws Exception {
+ String path = "/foo/bar/baz.txt";
+ RevCommit commit =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getCommit();
+ assertThat(commit.getParents()).isEmpty();
+
+ IllegalStateException exception =
+ assertThrows(
+ IllegalStateException.class, () -> changedFiles.getFromDiffCache(project, commit));
+ assertThat(exception).hasMessageThat().isEqualTo("diff cache doesn't support initial commits");
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.mergeCommitStrategy", value = "ALL_CHANGED_FILES")
+ public void getFromFileDiffCacheForMergeChange_allChangedFiles() throws Exception {
+ testGetFromFileDiffCacheForMergeChange(MergeCommitStrategy.ALL_CHANGED_FILES);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.mergeCommitStrategy",
+ value = "FILES_WITH_CONFLICT_RESOLUTION")
+ public void getFromFileDiffCacheForMergeChange_filesWithConflictResolution() throws Exception {
+ testGetFromFileDiffCacheForMergeChange(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION);
+ }
+
+ private void testGetFromFileDiffCacheForMergeChange(MergeCommitStrategy mergeCommitStrategy)
+ throws Exception {
+ setAsRootCodeOwners(admin);
+
+ String file1 = "foo/a.txt";
+ String file2 = "bar/b.txt";
+
+ // Create a base change.
+ Change.Id baseChange =
+ changeOperations.newChange().branch("master").file(file1).content("base content").create();
+ approveAndSubmit(baseChange);
+
+ // Create another branch
+ String branchName = "foo";
+ BranchInput branchInput = new BranchInput();
+ branchInput.ref = branchName;
+ branchInput.revision = projectOperations.project(project).getHead("master").name();
+ gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput);
+
+ // Create a change in master that touches file1.
+ Change.Id changeInMaster =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .file(file1)
+ .content("master content")
+ .create();
+ approveAndSubmit(changeInMaster);
+
+ // Create a change in the other branch and that touches file1 and creates file2.
+ Change.Id changeInOtherBranch =
+ changeOperations
+ .newChange()
+ .branch(branchName)
+ .file(file1)
+ .content("other content")
+ .file(file2)
+ .content("content")
+ .create();
+ approveAndSubmit(changeInOtherBranch);
+
+ // Create a merge change with a conflict resolution for file1 and file2 with the same content as
+ // in the other branch (no conflict on file2).
+ Change.Id mergeChange =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .mergeOfButBaseOnFirst()
+ .tipOfBranch("master")
+ .and()
+ .tipOfBranch(branchName)
+ .file(file1)
+ .content("merged content")
+ .file(file2)
+ .content("content")
+ .create();
+
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.getFromDiffCache(
+ project,
+ getRevisionResource(Integer.toString(mergeChange.get())).getPatchSet().commitId());
+
+ if (MergeCommitStrategy.ALL_CHANGED_FILES.equals(mergeCommitStrategy)) {
+ assertThat(changedFilesSet).comparingElementsUsing(hasPath()).containsExactly(file1, file2);
+ } else if (MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION.equals(mergeCommitStrategy)) {
+ assertThat(changedFilesSet).comparingElementsUsing(hasPath()).containsExactly(file1);
+ } else {
+ fail("expected merge commit strategy: " + mergeCommitStrategy);
+ }
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.mergeCommitStrategy", value = "ALL_CHANGED_FILES")
+ public void
+ getFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution_allChangedFiles()
+ throws Exception {
+ testGetFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.mergeCommitStrategy",
+ value = "FILES_WITH_CONFLICT_RESOLUTION")
+ public void
+ getFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution_filesWithConflictResolution()
+ throws Exception {
+ testGetFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution();
+ }
+
+ private void testGetFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution()
+ throws Exception {
+ setAsRootCodeOwners(admin);
+
+ String file = "foo/a.txt";
+
+ // Create a base change.
+ Change.Id baseChange =
+ changeOperations.newChange().branch("master").file(file).content("base content").create();
+ approveAndSubmit(baseChange);
+
+ // Create another branch
+ String branchName = "foo";
+ BranchInput branchInput = new BranchInput();
+ branchInput.ref = branchName;
+ branchInput.revision = projectOperations.project(project).getHead("master").name();
+ gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput);
+
+ // Create a change in master that touches file1.
+ Change.Id changeInMaster =
+ changeOperations.newChange().branch("master").file(file).content("master content").create();
+ approveAndSubmit(changeInMaster);
+
+ // Create a change in the other branch and that deleted file1.
+ PushOneCommit push =
+ pushFactory.create(admin.newIdent(), testRepo, "Change Deleting A File", file, "");
+ Result r = push.rm("refs/for/master");
+ r.assertOkStatus();
+ approveAndSubmit(r.getChange().getId());
+
+ // Create a merge change with resolving the conflict on file between the edit in master and the
+ // deletion in the other branch by deleting the file.
+ Change.Id mergeChange =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .mergeOf()
+ .tipOfBranch("master")
+ .and()
+ .tipOfBranch(branchName)
+ .file(file)
+ .delete()
+ .create();
+
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.getFromDiffCache(
+ project,
+ getRevisionResource(Integer.toString(mergeChange.get())).getPatchSet().commitId());
+ ImmutableSet<String> oldPaths =
+ changedFilesSet.stream()
+ .map(changedFile -> JgitPath.of(changedFile.oldPath().get()).get())
+ .collect(toImmutableSet());
+ assertThat(oldPaths).containsExactly(file);
+ }
+
+ @Test
+ public void getFromDiffCacheReturnsChangedFilesSortedByPath() throws Exception {
+ String file1 = "foo/bar.baz";
+ String file2 = "foo/baz.bar";
+ String file3 = "bar/foo.baz";
+ String file4 = "bar/baz.foo";
+ String file5 = "baz/foo.bar";
+ RevCommit commit =
+ createChange(
+ "Test Change",
+ ImmutableMap.of(
+ file1,
+ "file content",
+ file2,
+ "file content",
+ file3,
+ "file content",
+ file4,
+ "file content",
+ file5,
+ "file content"))
+ .getCommit();
+
+ ImmutableList<ChangedFile> changedFilesSet = changedFiles.getFromDiffCache(project, commit);
+ assertThat(changedFilesSet)
+ .comparingElementsUsing(hasPath())
+ .containsExactly(file4, file3, file5, file1, file2)
+ .inOrder();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.mergeCommitStrategy", value = "ALL_CHANGED_FILES")
+ public void getFromDiffCacheReturnsChangedFilesSortedByPath_mergeCommitAgainstFirstParent()
+ throws Exception {
+ testGetFromDiffCacheReturnsChangedFilesSortedByPathForMerge(
+ MergeCommitStrategy.ALL_CHANGED_FILES);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.mergeCommitStrategy",
+ value = "FILES_WITH_CONFLICT_RESOLUTION")
+ public void getFromDiffCacheReturnsChangedFilesSortedByPath_mergeCommitAgainstAutoMerge()
+ throws Exception {
+ testGetFromDiffCacheReturnsChangedFilesSortedByPathForMerge(
+ MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION);
+ }
+
+ private void testGetFromDiffCacheReturnsChangedFilesSortedByPathForMerge(
+ MergeCommitStrategy mergeCommitStrategy) throws Exception {
+ setAsRootCodeOwners(admin);
+
+ String file1 = "foo/bar.baz";
+ String file2 = "foo/baz.bar";
+ String file3 = "bar/foo.baz";
+ String file4 = "bar/baz.foo";
+ String file5 = "baz/foo.bar";
+
+ // Create a base change.
+ Change.Id baseChange =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .file(file1)
+ .content("base content")
+ .file(file3)
+ .content("base content")
+ .file(file5)
+ .content("base content")
+ .create();
+ approveAndSubmit(baseChange);
+
+ // Create another branch
+ String branchName = "foo";
+ BranchInput branchInput = new BranchInput();
+ branchInput.ref = branchName;
+ branchInput.revision = projectOperations.project(project).getHead("master").name();
+ gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput);
+
+ // Create a change in master that touches file1, file3 and file5
+ Change.Id changeInMaster =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .file(file1)
+ .content("master content")
+ .file(file3)
+ .content("master content")
+ .file(file5)
+ .content("master content")
+ .create();
+ approveAndSubmit(changeInMaster);
+
+ // Create a change in the other branch and that touches file1, file3, file5 and creates file2,
+ // file4.
+ Change.Id changeInOtherBranch =
+ changeOperations
+ .newChange()
+ .branch(branchName)
+ .file(file1)
+ .content("other content")
+ .file(file2)
+ .content("content")
+ .file(file3)
+ .content("other content")
+ .file(file4)
+ .content("content")
+ .file(file5)
+ .content("other content")
+ .create();
+ approveAndSubmit(changeInOtherBranch);
+
+ // Create a merge change with a conflict resolution for file1 and file2 with the same content as
+ // in the other branch (no conflict on file2).
+ Change.Id mergeChange =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .mergeOfButBaseOnFirst()
+ .tipOfBranch("master")
+ .and()
+ .tipOfBranch(branchName)
+ .file(file1)
+ .content("merged content")
+ .file(file2)
+ .content("content")
+ .file(file3)
+ .content("merged content")
+ .file(file4)
+ .content("content")
+ .file(file5)
+ .content("merged content")
+ .create();
+
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.getFromDiffCache(
+ project,
+ getRevisionResource(Integer.toString(mergeChange.get())).getPatchSet().commitId());
+
+ if (MergeCommitStrategy.ALL_CHANGED_FILES.equals(mergeCommitStrategy)) {
+ assertThat(changedFilesSet)
+ .comparingElementsUsing(hasPath())
+ .containsExactly(file4, file3, file5, file1, file2)
+ .inOrder();
+ } else if (MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION.equals(mergeCommitStrategy)) {
+ assertThat(changedFilesSet)
+ .comparingElementsUsing(hasPath())
+ .containsExactly(file3, file5, file1);
+ } else {
+ fail("expected merge commit strategy: " + mergeCommitStrategy);
+ }
+ }
+
private void approveAndSubmit(Change.Id changeId) throws Exception {
approve(Integer.toString(changeId.get()));
gApi.changes().id(changeId.get()).current().submit();
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index 596eead..6c6b0f2 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -152,6 +152,19 @@
@Test
public void getStatusForFileRename_insufficientReviewers() throws Exception {
+ testGetStatusForFileRename_insufficientReviewers(/* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_insufficientReviewers_useDiffCache() throws Exception {
+ testGetStatusForFileRename_insufficientReviewers(/* useDiffCache= */ true);
+ }
+
+ private void testGetStatusForFileRename_insufficientReviewers(boolean useDiffCache)
+ throws Exception {
TestAccount user2 = accountCreator.user2();
Path oldPath = Paths.get("/foo/old.bar");
@@ -167,10 +180,20 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ if (useDiffCache) {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ } else {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
}
@Test
@@ -245,6 +268,18 @@
@Test
public void getStatusForFileRename_pendingOldPath() throws Exception {
+ testGetStatusForFileRename_pendingOldPath(/* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_pendingOldPath_useDiffCache() throws Exception {
+ testGetStatusForFileRename_pendingOldPath(/* useDiffCache= */ true);
+ }
+
+ private void testGetStatusForFileRename_pendingOldPath(boolean useDiffCache) throws Exception {
TestAccount user2 = accountCreator.user2();
setAsCodeOwners("/foo/bar/", user);
@@ -262,14 +297,36 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.PENDING),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ if (useDiffCache) {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.PENDING,
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ } else {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.PENDING),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
}
@Test
public void getStatusForFileRename_pendingNewPath() throws Exception {
+ testGetStatusForFileRename_pendingNewPath(/* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_pendingNewPath_useDiffCache() throws Exception {
+ testGetStatusForFileRename_pendingNewPath(/* useDiffCache= */ true);
+ }
+
+ private void testGetStatusForFileRename_pendingNewPath(boolean useDiffCache) throws Exception {
TestAccount user2 = accountCreator.user2();
setAsCodeOwners("/foo/baz/", user);
@@ -287,10 +344,20 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.PENDING));
+ if (useDiffCache) {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ newPath,
+ CodeOwnerStatus.PENDING));
+ } else {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.PENDING));
+ }
}
@Test
@@ -350,6 +417,18 @@
@Test
public void getStatusForFileRename_approvedOldPath() throws Exception {
+ testGetStatusForFileRename_approvedOldPath(/* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_approvedOldPath_useDiffCache() throws Exception {
+ testGetStatusForFileRename_approvedOldPath(/* useDiffCache= */ true);
+ }
+
+ private void testGetStatusForFileRename_approvedOldPath(boolean useDiffCache) throws Exception {
setAsCodeOwners("/foo/bar/", user);
Path oldPath = Paths.get("/foo/bar/abc.txt");
@@ -363,14 +442,36 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.APPROVED),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ if (useDiffCache) {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.APPROVED,
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ } else {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.APPROVED),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
}
@Test
public void getStatusForFileRename_approvedNewPath() throws Exception {
+ testGetStatusForFileRename_approvedNewPath(/* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_approvedNewPath_useDiffCache() throws Exception {
+ testGetStatusForFileRename_approvedNewPath(/* useDiffCache= */ true);
+ }
+
+ private void testGetStatusForFileRename_approvedNewPath(boolean useDiffCache) throws Exception {
setAsCodeOwners("/foo/baz/", user);
Path oldPath = Paths.get("/foo/bar/abc.txt");
@@ -384,10 +485,20 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.APPROVED));
+ if (useDiffCache) {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ newPath,
+ CodeOwnerStatus.APPROVED));
+ } else {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.APPROVED));
+ }
}
@Test
@@ -495,7 +606,17 @@
public void getStatusForFileRename_noImplicitApprovalByPatchSetUploaderOnOldPath()
throws Exception {
testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnOldPath(
- /* implicitApprovalsEnabled= */ false);
+ /* implicitApprovalsEnabled= */ false, /* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_noImplicitApprovalByPatchSetUploaderOnOldPath_useDiffCache()
+ throws Exception {
+ testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnOldPath(
+ /* implicitApprovalsEnabled= */ false, /* useDiffCache= */ true);
}
@Test
@@ -503,11 +624,22 @@
public void getStatusForFileRename_withImplicitApprovalByPatchSetUploaderOnOldPath()
throws Exception {
testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnOldPath(
- /* implicitApprovalsEnabled= */ true);
+ /* implicitApprovalsEnabled= */ true, /* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_withImplicitApprovalByPatchSetUploaderOnOldPath_useDiffCache()
+ throws Exception {
+ testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnOldPath(
+ /* implicitApprovalsEnabled= */ true, /* useDiffCache= */ true);
}
private void testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnOldPath(
- boolean implicitApprovalsEnabled) throws Exception {
+ boolean implicitApprovalsEnabled, boolean useDiffCache) throws Exception {
setAsCodeOwners("/foo/bar/", user);
Path oldPath = Paths.get("/foo/bar/abc.txt");
@@ -517,21 +649,43 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(
- oldPath,
- implicitApprovalsEnabled
- ? CodeOwnerStatus.APPROVED
- : CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ if (useDiffCache) {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ } else {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(
+ oldPath,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
}
@Test
public void getStatusForFileRename_noImplicitApprovalByPatchSetUploaderOnNewPath()
throws Exception {
testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnNewPath(
- /* implicitApprovalsEnabled= */ false);
+ /* implicitApprovalsEnabled= */ false, /* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_noImplicitApprovalByPatchSetUploaderOnNewPath_useDiffCache()
+ throws Exception {
+ testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnNewPath(
+ /* implicitApprovalsEnabled= */ false, /* useDiffCache= */ true);
}
@Test
@@ -539,11 +693,22 @@
public void getStatusForFileRename_withImplicitApprovalByPatchSetUploaderOnNewPath()
throws Exception {
testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnNewPath(
- /* implicitApprovalsEnabled= */ true);
+ /* implicitApprovalsEnabled= */ true, /* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_withImplicitApprovalByPatchSetUploaderOnNewPath_useDiffCache()
+ throws Exception {
+ testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnNewPath(
+ /* implicitApprovalsEnabled= */ true, /* useDiffCache= */ true);
}
private void testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnNewPath(
- boolean implicitApprovalsEnabled) throws Exception {
+ boolean implicitApprovalsEnabled, boolean useDiffCache) throws Exception {
setAsCodeOwners("/foo/baz/", user);
Path oldPath = Paths.get("/foo/bar/abc.txt");
@@ -553,14 +718,26 @@
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(
- newPath,
- implicitApprovalsEnabled
- ? CodeOwnerStatus.APPROVED
- : CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ if (useDiffCache) {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ newPath,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ } else {
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
+ FileCodeOwnerStatus.addition(
+ newPath,
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
}
@Test
diff --git a/resources/Documentation/metrics.md b/resources/Documentation/metrics.md
index 00659f7..fd782a7 100644
--- a/resources/Documentation/metrics.md
+++ b/resources/Documentation/metrics.md
@@ -27,6 +27,8 @@
approval is applied.
* `get_auto_merge`:
Latency for getting the auto merge commit of a merge commit.
+* `get_changed_files`:
+ Latency for getting changed files from diff cache.
* `prepare_file_status_computation`:
Latency for preparing the file status computation.
* `prepare_file_status_computation_for_account`:
