Merge "CodeOwnerApprovalCheck: Allow to use diff cache to get changed files"
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 b0ac537..e69616d 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 06664ba..e5588bf 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;
@@ -101,6 +102,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 d06a9f3..de0ec87 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`:
