Always use new diff cache for computing changed files
This removes the experiment flag that guarded the rollout of the new
diff cache usage to compute the changed files.
Since changed files are now always retrieved from the new diff cache we
can drop all code to compute the changed files on our own.
Change-Id: I811c28f6420bd770a5d42084b6019bc851dab5bb
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 127377f..81f31b3 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
@@ -19,10 +19,7 @@
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;
@@ -31,266 +28,59 @@
import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy;
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.filediff.FileDiffOutput;
-import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.inject.Inject;
-import com.google.inject.OutOfScopeException;
-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;
-import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.merge.ThreeWayMergeStrategy;
-import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
-import org.eclipse.jgit.util.io.DisabledOutputStream;
/**
- * Class to get/compute the files that have been changed in a revision.
+ * Class to get the files that have been changed in a revision.
*
* <p>The {@link #getFromDiffCache(Project.NameKey, ObjectId, MergeCommitStrategy)} method is
* retrieving the file diff from the diff cache and has rename detection enabled.
*
- * <p>In contrast to this, for the {@link #compute(Project.NameKey, ObjectId, MergeCommitStrategy)}
- * method 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>Which of these methods is invoked when calling any of {@code getOrCompute} methods is
- * controlled by the {@link CodeOwnersExperimentFeaturesConstants#USE_DIFF_CACHE} experiment feature
- * flag.
- *
* <p>The {@link com.google.gerrit.server.patch.PatchListCache} is deprecated, and hence it not
* being used here.
*/
@Singleton
public class ChangedFiles {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
- private static int MAX_CHANGED_FILES_TO_LOG = 25;
-
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 ThreadLocalRequestContext threadLocalRequestContext;
- private final ExperimentFeatures experimentFeatures;
@Inject
public ChangedFiles(
- @GerritServerConfig Config cfg,
GitRepositoryManager repoManager,
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
DiffOperations diffOperations,
- Provider<AutoMerger> autoMergerProvider,
- CodeOwnerMetrics codeOwnerMetrics,
- ThreadLocalRequestContext threadLocalRequestContext,
- ExperimentFeatures experimentFeatures) {
+ CodeOwnerMetrics codeOwnerMetrics) {
this.repoManager = repoManager;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.diffOperations = diffOperations;
- this.autoMergerProvider = autoMergerProvider;
this.codeOwnerMetrics = codeOwnerMetrics;
- this.threadLocalRequestContext = threadLocalRequestContext;
- 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, MergeCommitStrategy)}).
- *
- * <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, MergeCommitStrategy)}).
- *
- * @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, MergeCommitStrategy mergeCommitStrategy)
- throws IOException, DiffNotAvailableException {
- requireNonNull(project, "project");
- requireNonNull(revision, "revision");
- requireNonNull(mergeCommitStrategy, "mergeCommitStrategy");
-
- if (useDiffCache()) {
- return getFromDiffCache(project, revision, mergeCommitStrategy);
- }
- return compute(project, revision, mergeCommitStrategy);
- }
-
- /** Checks whether the experiment flag to use the diff cache is set. */
- private boolean useDiffCache() {
- // Checking whether the experiment flag is set requires a current user, but since this code
- // may be called from a background thread (e.g. when indexing a change) a current user may not
- // be available. Hence check for the experiment flag only when a current user is available,
- // otherwise return false to not use the diff cache in this case.
- try {
- threadLocalRequestContext.getContext().getUser();
- } catch (OutOfScopeException e) {
- return false;
- }
- return experimentFeatures.isFeatureEnabled(
- CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE);
- }
-
- /**
- * Returns the changed files for the given revision.
- *
- * <p>Uses the configured merge commit strategy.
- *
- * @param project the project
- * @param revision the revision for which the changed files should be computed
- * @throws IOException thrown if the computation fails due to an I/O error
- * @see #getOrCompute(Project.NameKey, ObjectId, MergeCommitStrategy)
- */
- public ImmutableList<ChangedFile> getOrCompute(Project.NameKey project, ObjectId revision)
- throws IOException, DiffNotAvailableException {
- requireNonNull(project, "project");
- requireNonNull(revision, "revision");
-
- return getOrCompute(
- project,
- revision,
- codeOwnersPluginConfiguration.getProjectConfig(project).getMergeCommitStrategy());
- }
-
- /**
- * Returns the changed files for the given revision.
- *
- * <p>Uses the configured merge commit strategy.
- *
- * @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
- * @see #getOrCompute(Project.NameKey, ObjectId, MergeCommitStrategy)
- */
- public ImmutableList<ChangedFile> getOrCompute(RevisionResource revisionResource)
- throws IOException, DiffNotAvailableException {
- requireNonNull(revisionResource, "revisionResource");
- return getOrCompute(revisionResource.getProject(), revisionResource.getPatchSet().commitId());
- }
-
- /**
- * Computed the changed files for the given revision.
- *
- * <p>The changed files are newly computed on each access. Rename detection is disabled (as it's
- * too expensive to do it on each access).
- *
- * @param project the project
- * @param revision the revision for which the changed files should be computed
- * @param mergeCommitStrategy the merge commit strategy that should be used to compute the changed
- * files for merge commits
- * @return the changed files
- */
- private ImmutableList<ChangedFile> compute(
- Project.NameKey project, ObjectId revision, MergeCommitStrategy mergeCommitStrategy)
- throws IOException {
- requireNonNull(project, "project");
- requireNonNull(revision, "revision");
- requireNonNull(mergeCommitStrategy, "mergeCommitStrategy");
-
- logger.atFine().log(
- "computing changed files for revision %s in project %s", revision.name(), project);
-
- try (Repository repo = repoManager.openRepository(project);
- RevWalk revWalk = new RevWalk(repo)) {
- RevCommit revCommit = revWalk.parseCommit(revision);
- if (revCommit.getParentCount() > 1
- && MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION.equals(mergeCommitStrategy)) {
- RevCommit autoMergeCommit = getAutoMergeCommit(project, revCommit);
- return compute(repo.getConfig(), revWalk, revCommit, autoMergeCommit);
- }
-
- RevCommit baseCommit = revCommit.getParentCount() > 0 ? revCommit.getParent(0) : null;
- return compute(repo.getConfig(), revWalk, revCommit, baseCommit);
- }
- }
-
- private RevCommit getAutoMergeCommit(Project.NameKey project, RevCommit mergeCommit)
- throws IOException {
- try (Timer0.Context ctx = codeOwnerMetrics.getAutoMerge.start();
- Repository repository = repoManager.openRepository(project);
- InMemoryInserter inserter = new InMemoryInserter(repository);
- ObjectReader reader = inserter.newReader();
- RevWalk revWalk = new RevWalk(reader)) {
- return autoMergerProvider
- .get()
- .lookupFromGitOrMergeInMemory(repository, revWalk, inserter, mergeCommit, mergeStrategy);
- }
- }
-
- /**
- * Computes the changed files by comparing the given commit against the given base commit.
- *
- * <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
- * @param baseCommit the base commit against which the given commit should be compared, {@code
- * null} if the commit doesn't have any parent commit
- * @return the changed files for the given commit, sorted alphabetically by path
- */
- private ImmutableList<ChangedFile> compute(
- Config repoConfig, RevWalk revWalk, RevCommit commit, @Nullable RevCommit baseCommit)
- throws IOException {
- logger.atFine().log("baseCommit = %s", baseCommit != null ? baseCommit.name() : "n/a");
- try (Timer0.Context ctx = codeOwnerMetrics.computeChangedFiles.start()) {
- // Detecting renames is expensive (since it requires Git to load and compare file contents of
- // added and deleted files) and can significantly increase the latency for changes that touch
- // large files. To avoid this latency we do not enable the rename detection on the
- // DiffFormater. As a result of this renamed files will be returned as 2 ChangedFile's, one
- // for the deletion of the old path and one for the addition of the new path.
- try (DiffFormatter diffFormatter = new DiffFormatter(DisabledOutputStream.INSTANCE)) {
- diffFormatter.setReader(revWalk.getObjectReader(), repoConfig);
- diffFormatter.setDiffComparator(RawTextComparator.DEFAULT);
- List<DiffEntry> diffEntries = diffFormatter.scan(baseCommit, commit);
- ImmutableList<ChangedFile> changedFiles =
- diffEntries.stream().map(ChangedFile::create).collect(toImmutableList());
- if (changedFiles.size() <= MAX_CHANGED_FILES_TO_LOG) {
- logger.atFine().log("changed files = %s", changedFiles);
- } else {
- logger.atFine().log(
- "changed files = %s (and %d more)",
- changedFiles.asList().subList(0, MAX_CHANGED_FILES_TO_LOG),
- changedFiles.size() - MAX_CHANGED_FILES_TO_LOG);
- }
- return changedFiles;
- }
- }
}
/**
* Gets the changed files from the diff cache.
*
* <p>Rename detection is enabled.
+ *
+ * @param project the project
+ * @param revision the revision for which the changed files should be retrieved
+ * @param mergeCommitStrategy the merge commit strategy that should be used to compute the changed
+ * files for merge commits
+ * @return the files that have been changed in the given revision, sorted alphabetically by path
*/
- @VisibleForTesting
- ImmutableList<ChangedFile> getFromDiffCache(
+ public ImmutableList<ChangedFile> getFromDiffCache(
Project.NameKey project, ObjectId revision, MergeCommitStrategy mergeCommitStrategy)
throws IOException, DiffNotAvailableException {
requireNonNull(project, "project");
@@ -322,6 +112,47 @@
}
}
+ /**
+ * Gets the changed files from the diff cache.
+ *
+ * <p>Rename detection is enabled.
+ *
+ * <p>Uses the configured merge commit strategy.
+ *
+ * @param project the project
+ * @param revision the revision for which the changed files should be retrieved
+ * @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
+ */
+ public ImmutableList<ChangedFile> getFromDiffCache(Project.NameKey project, ObjectId revision)
+ throws IOException, DiffNotAvailableException {
+ requireNonNull(project, "project");
+ requireNonNull(revision, "revision");
+ return getFromDiffCache(
+ project,
+ revision,
+ codeOwnersPluginConfiguration.getProjectConfig(project).getMergeCommitStrategy());
+ }
+
+ /**
+ * Gets the changed files from the diff cache.
+ *
+ * <p>Rename detection is enabled.
+ *
+ * <p>Uses the configured merge commit strategy.
+ *
+ * @param revisionResource the revision resource for which the changed files should be retrieved
+ * @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
+ * @see #getFromDiffCache(Project.NameKey, ObjectId, MergeCommitStrategy)
+ */
+ public ImmutableList<ChangedFile> getFromDiffCache(RevisionResource revisionResource)
+ throws IOException, DiffNotAvailableException {
+ requireNonNull(revisionResource, "revisionResource");
+ return getFromDiffCache(
+ revisionResource.getProject(), revisionResource.getPatchSet().commitId());
+ }
+
private boolean isInitialCommit(Project.NameKey project, ObjectId objectId) throws IOException {
try (Repository repo = repoManager.openRepository(project);
RevWalk revWalk = new RevWalk(repo)) {
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 12d17fa..6988acb 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -361,7 +361,8 @@
FallbackCodeOwners fallbackCodeOwners = codeOwnersConfig.getFallbackCodeOwners();
return changedFiles
- .getOrCompute(changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId())
+ .getFromDiffCache(
+ changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId())
.stream()
.map(
changedFile ->
@@ -427,7 +428,8 @@
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy = codeOwnerConfigHierarchyProvider.get();
CodeOwnerResolver codeOwnerResolver =
codeOwnerResolverProvider.get().enforceVisibility(false);
- return changedFiles.getOrCompute(changeNotes.getProjectName(), patchSet.commitId()).stream()
+ return changedFiles.getFromDiffCache(changeNotes.getProjectName(), patchSet.commitId())
+ .stream()
.map(
changedFile ->
getFileStatus(
@@ -467,7 +469,7 @@
ChangeNotes changeNotes, PatchSet patchSet, String reason)
throws IOException, DiffNotAvailableException {
logger.atFine().log("all paths are approved (reason = %s)", reason);
- return changedFiles.getOrCompute(changeNotes.getProjectName(), patchSet.commitId()).stream()
+ return changedFiles.getFromDiffCache(changeNotes.getProjectName(), patchSet.commitId()).stream()
.map(
changedFile ->
FileCodeOwnerStatus.create(
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExperimentFeaturesConstants.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExperimentFeaturesConstants.java
deleted file mode 100644
index 01bf7d6..0000000
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExperimentFeaturesConstants.java
+++ /dev/null
@@ -1,39 +0,0 @@
-// 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,
- * com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy)
- */
- 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/backend/config/CodeOwnersPluginConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
index a92e0eb..97029e0 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
@@ -127,7 +127,7 @@
private boolean isFileChanged(CommitReceivedEvent receiveEvent, String fileName)
throws IOException, DiffNotAvailableException {
return changedFiles
- .getOrCompute(
+ .getFromDiffCache(
receiveEvent.project.getNameKey(),
receiveEvent.commit,
MergeCommitStrategy.ALL_CHANGED_FILES)
diff --git a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
index 8d6dbad..d6ac638 100644
--- a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
+++ b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
@@ -33,13 +33,11 @@
public class CodeOwnerMetrics {
// latency metrics
public final Timer1<String> addChangeMessageOnAddReviewer;
- public final Timer0 computeChangedFiles;
public final Timer0 computeFileStatus;
public final Timer0 computeFileStatuses;
public final Timer0 computeOwnedPaths;
public final Timer0 computePatchSetApprovals;
public final Timer0 extendChangeMessageOnPostReview;
- public final Timer0 getAutoMerge;
public final Timer0 getChangedFiles;
public final Timer0 prepareFileStatusComputation;
public final Timer0 prepareFileStatusComputationForAccount;
@@ -87,8 +85,6 @@
.description(
"Whether the change message was posted synchronously or asynchronously.")
.build());
- this.computeChangedFiles =
- createTimer("compute_changed_files", "Latency for computing changed files");
this.computeFileStatus =
createTimer("compute_file_status", "Latency for computing the file status of one file");
this.computeFileStatuses =
@@ -108,9 +104,6 @@
"extend_change_message_on_post_review",
"Latency for extending the change message with the owned path when a code owner"
+ " approval is applied");
- this.getAutoMerge =
- createTimer(
- "get_auto_merge", "Latency for getting the auto merge commit of a merge commit");
this.getChangedFiles =
createTimer("get_changed_files", "Latency for getting changed files from diff cache");
this.prepareFileStatusComputation =
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFilesInRevision.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFilesInRevision.java
index a53b19b..af4b97f 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFilesInRevision.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFilesInRevision.java
@@ -95,7 +95,7 @@
RevWalk rw = new RevWalk(repository)) {
RevCommit commit = rw.parseCommit(revisionResource.getPatchSet().commitId());
return Response.ok(
- changedFiles.getOrCompute(revisionResource.getProject(), commit).stream()
+ changedFiles.getFromDiffCache(revisionResource.getProject(), commit).stream()
// filter out deletions (files without new path)
.filter(changedFile -> changedFile.newPath().isPresent())
// filter out non code owner config files
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java
index 2ceb086..20e8dfa 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java
@@ -103,7 +103,7 @@
private void checkThatFileExists(
RevisionResource revisionResource, PathResource pathResource, IdString id)
throws RestApiException, IOException, DiffNotAvailableException {
- if (!changedFiles.getOrCompute(revisionResource).stream()
+ if (!changedFiles.getFromDiffCache(revisionResource).stream()
.anyMatch(
changedFile ->
// Check whether the path matches any file in the change.
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index 1666568..136ffcb 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -410,7 +410,7 @@
// MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION is configured.
ImmutableList<ChangedFile> modifiedCodeOwnerConfigFiles =
changedFiles
- .getOrCompute(
+ .getFromDiffCache(
branchNameKey.project(), revCommit, MergeCommitStrategy.ALL_CHANGED_FILES)
.stream()
// filter out deletions (files without new path)
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 56ba30d..fb87053 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
@@ -22,12 +22,10 @@
import com.google.common.collect.ImmutableMap;
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.extensions.restapi.BadRequestException;
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.gerrit.plugins.codeowners.util.JgitPath;
@@ -341,18 +339,6 @@
@Test
public void getStatusWithLimitForRename() throws Exception {
- testGetStatusWithLimitForRenamedFile(/* useDiffCache= */ false);
- }
-
- @Test
- @GerritConfig(
- name = "experiments.enabled",
- value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
- public void getStatusWithLimitForRename_useDiffCache() throws Exception {
- testGetStatusWithLimitForRenamedFile(/* useDiffCache= */ true);
- }
-
- private void testGetStatusWithLimitForRenamedFile(boolean useDiffCache) throws Exception {
codeOwnerConfigOperations
.newCodeOwnerConfig()
.project(project)
@@ -370,42 +356,19 @@
CodeOwnerStatusInfo codeOwnerStatus =
changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().withLimit(1).get();
- if (useDiffCache) {
- assertThat(codeOwnerStatus)
- .hasFileCodeOwnerStatusesThat()
- .comparingElementsUsing(isFileCodeOwnerStatus())
- .containsExactly(
- FileCodeOwnerStatus.rename(
- oldPath,
- CodeOwnerStatus.PENDING,
- String.format(
- "reviewer %s is a code owner",
- AccountTemplateUtil.getAccountTemplate(user.id())),
- newPath,
- CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
- /* reasonNewPath= */ null));
- } else {
- assertThat(codeOwnerStatus)
- .hasFileCodeOwnerStatusesThat()
- .comparingElementsUsing(isFileCodeOwnerStatus())
- .containsExactly(
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
- assertThat(codeOwnerStatus).hasMoreThat().isTrue();
-
- codeOwnerStatus =
- changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().withLimit(2).get();
- assertThat(codeOwnerStatus)
- .hasFileCodeOwnerStatusesThat()
- .comparingElementsUsing(isFileCodeOwnerStatus())
- .containsExactly(
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.deletion(
- oldPath,
- CodeOwnerStatus.PENDING,
- String.format(
- "reviewer %s is a code owner",
- AccountTemplateUtil.getAccountTemplate(user.id()))));
- }
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a code owner",
+ AccountTemplateUtil.getAccountTemplate(user.id())),
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ /* reasonNewPath= */ null));
assertThat(codeOwnerStatus).hasMoreThat().isNull();
assertThat(codeOwnerStatus).hasAccounts(user);
}
@@ -515,18 +478,6 @@
@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);
@@ -538,96 +489,54 @@
CodeOwnerStatusInfo codeOwnerStatus =
changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
- 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));
- }
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ 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();
- if (useDiffCache) {
- assertThat(codeOwnerStatus)
- .hasFileCodeOwnerStatusesThat()
- .comparingElementsUsing(isFileCodeOwnerStatus())
- .containsExactly(
- FileCodeOwnerStatus.rename(
- oldPath,
- CodeOwnerStatus.PENDING,
- String.format(
- "reviewer %s is a code owner",
- AccountTemplateUtil.getAccountTemplate(user.id())),
- newPath,
- CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
- /* reasonNewPath= */ null));
- } else {
- assertThat(codeOwnerStatus)
- .hasFileCodeOwnerStatusesThat()
- .comparingElementsUsing(isFileCodeOwnerStatus())
- .containsExactly(
- FileCodeOwnerStatus.deletion(
- oldPath,
- CodeOwnerStatus.PENDING,
- String.format(
- "reviewer %s is a code owner",
- AccountTemplateUtil.getAccountTemplate(user.id()))),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
- }
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a code owner",
+ AccountTemplateUtil.getAccountTemplate(user.id())),
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ /* reasonNewPath= */ null));
assertThat(codeOwnerStatus).hasAccounts(user);
// 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();
- if (useDiffCache) {
- assertThat(codeOwnerStatus)
- .hasFileCodeOwnerStatusesThat()
- .comparingElementsUsing(isFileCodeOwnerStatus())
- .containsExactly(
- FileCodeOwnerStatus.rename(
- oldPath,
- CodeOwnerStatus.PENDING,
- String.format(
- "reviewer %s is a code owner",
- AccountTemplateUtil.getAccountTemplate(user.id())),
- newPath,
- CodeOwnerStatus.PENDING,
- String.format(
- "reviewer %s is a code owner",
- AccountTemplateUtil.getAccountTemplate(user2.id()))));
- } else {
- assertThat(codeOwnerStatus)
- .hasFileCodeOwnerStatusesThat()
- .comparingElementsUsing(isFileCodeOwnerStatus())
- .containsExactly(
- FileCodeOwnerStatus.deletion(
- oldPath,
- CodeOwnerStatus.PENDING,
- String.format(
- "reviewer %s is a code owner",
- AccountTemplateUtil.getAccountTemplate(user.id()))),
- FileCodeOwnerStatus.addition(
- newPath,
- CodeOwnerStatus.PENDING,
- String.format(
- "reviewer %s is a code owner",
- AccountTemplateUtil.getAccountTemplate(user2.id()))));
- }
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a code owner",
+ AccountTemplateUtil.getAccountTemplate(user.id())),
+ newPath,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a code owner",
+ AccountTemplateUtil.getAccountTemplate(user2.id()))));
assertThat(codeOwnerStatus).hasAccounts(user, user2);
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
index fc8f176..d72e981 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
@@ -65,431 +65,34 @@
}
@Test
- public void cannotComputeForNullRevisionResource() throws Exception {
+ public void cannotGetFromDiffCacheForNullRevisionResource() throws Exception {
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> changedFiles.getOrCompute(/* revisionResource= */ null));
+ () -> changedFiles.getFromDiffCache(/* revisionResource= */ null));
assertThat(npe).hasMessageThat().isEqualTo("revisionResource");
}
@Test
- public void cannotGetOrComputeForNullProject() throws Exception {
+ public void cannotGetFromDiffCacheForNullProject_v1() throws Exception {
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> changedFiles.getOrCompute(/* project= */ null, ObjectId.zeroId()));
+ () -> changedFiles.getFromDiffCache(/* project= */ null, ObjectId.zeroId()));
assertThat(npe).hasMessageThat().isEqualTo("project");
}
@Test
- public void cannotGetOrComputeForNullRevision() throws Exception {
+ public void cannotGetFromDiffCacheForNullRevision_v1() throws Exception {
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> changedFiles.getOrCompute(project, /* revision= */ null));
+ () -> changedFiles.getFromDiffCache(project, /* revision= */ null));
assertThat(npe).hasMessageThat().isEqualTo("revision");
}
@Test
- public void computeForChangeThatAddedAFile() throws Exception {
- String path = "/foo/bar/baz.txt";
- String changeId =
- createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
-
- ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.getOrCompute(getRevisionResource(changeId));
- 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 computeForChangeThatModifiedAFile() throws Exception {
- String path = "/foo/bar/baz.txt";
- createChange("Test Change", JgitPath.of(path).get(), "file content").getChangeId();
-
- String changeId =
- createChange("Change Modifying A File", JgitPath.of(path).get(), "new file content")
- .getChangeId();
-
- ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.getOrCompute(getRevisionResource(changeId));
- 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 computeForChangeThatDeletedAFile() throws Exception {
- String path = "/foo/bar/baz.txt";
- String changeId = createChangeWithFileDeletion(path);
-
- ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.getOrCompute(getRevisionResource(changeId));
- 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 computeForChangeThatRenamedAFile() 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();
-
- // A renamed file is reported as addition of new path + deletion of old path. This is because
- // ChangedFiles uses a DiffFormatter without rename detection (because rename detection requires
- // loading the file contents which is too expensive).
- ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.getOrCompute(getRevisionResource(changeId));
- assertThat(changedFilesSet).hasSize(2);
- ChangedFileSubject changedFile1 = assertThatCollection(changedFilesSet).element(0);
- changedFile1.hasNewPath().value().isEqualTo(Paths.get(newPath));
- changedFile1.hasOldPath().isEmpty();
- changedFile1.isNoRename();
- changedFile1.isNoDeletion();
- ChangedFileSubject changedFile2 = assertThatCollection(changedFilesSet).element(1);
- changedFile2.hasNewPath().isEmpty();
- changedFile2.hasOldPath().value().isEqualTo(Paths.get(oldPath));
- changedFile2.isNoRename();
- changedFile2.isDeletion();
- }
-
- @Test
- @TestProjectInput(createEmptyCommit = false)
- public void computeForInitialChangeThatAddedAFile() throws Exception {
- String path = "/foo/bar/baz.txt";
- Result r = createChange("Change Adding A File", JgitPath.of(path).get(), "file content");
- assertThat(r.getCommit().getParents()).isEmpty();
- String changeId = r.getChangeId();
-
- ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.getOrCompute(getRevisionResource(changeId));
- 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
- @GerritConfig(name = "plugin.code-owners.mergeCommitStrategy", value = "ALL_CHANGED_FILES")
- public void computeForMergeChange_allChangedFiles() throws Exception {
- testComputeForMergeChange(MergeCommitStrategy.ALL_CHANGED_FILES);
- }
-
- @Test
- @GerritConfig(
- name = "plugin.code-owners.mergeCommitStrategy",
- value = "FILES_WITH_CONFLICT_RESOLUTION")
- public void computeForMergeChange_filesWithConflictResolution() throws Exception {
- testComputeForMergeChange(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION);
- }
-
- private void testComputeForMergeChange(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.getOrCompute(getRevisionResource(Integer.toString(mergeChange.get())));
-
- 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 computeForMergeChangeThatContainsADeletedFileAsConflictResolution_allChangedFiles()
- throws Exception {
- testComputeForMergeChangeThatContainsADeletedFileAsConflictResolution();
- }
-
- @Test
- @GerritConfig(
- name = "plugin.code-owners.mergeCommitStrategy",
- value = "FILES_WITH_CONFLICT_RESOLUTION")
- public void
- computeForMergeChangeThatContainsADeletedFileAsConflictResolution_filesWithConflictResolution()
- throws Exception {
- testComputeForMergeChangeThatContainsADeletedFileAsConflictResolution();
- }
-
- private void testComputeForMergeChangeThatContainsADeletedFileAsConflictResolution()
- 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.getOrCompute(getRevisionResource(Integer.toString(mergeChange.get())));
- ImmutableSet<String> oldPaths =
- changedFilesSet.stream()
- .map(changedFile -> JgitPath.of(changedFile.oldPath().get()).get())
- .collect(toImmutableSet());
- assertThat(oldPaths).containsExactly(file);
- }
-
- @Test
- public void computeReturnsChangedFilesSortedByPath() 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";
- String changeId =
- createChange(
- "Test Change",
- ImmutableMap.of(
- file1,
- "file content",
- file2,
- "file content",
- file3,
- "file content",
- file4,
- "file content",
- file5,
- "file content"))
- .getChangeId();
-
- ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.getOrCompute(getRevisionResource(changeId));
- assertThat(changedFilesSet)
- .comparingElementsUsing(hasPath())
- .containsExactly(file4, file3, file5, file1, file2)
- .inOrder();
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.mergeCommitStrategy", value = "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 computeReturnsChangedFilesSortedByPath_mergeCommitAgainstAutoMerge()
- throws Exception {
- testComputeReturnsChangedFilesSortedByPathForMerge(
- MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION);
- }
-
- private void testComputeReturnsChangedFilesSortedByPathForMerge(
- 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.getOrCompute(getRevisionResource(Integer.toString(mergeChange.get())));
-
- 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);
- }
- }
-
- @Test
- public void cannotGetFromDiffCacheForNullProject() throws Exception {
+ public void cannotGetFromDiffCacheForNullProject_v2() throws Exception {
NullPointerException npe =
assertThrows(
NullPointerException.class,
@@ -500,7 +103,7 @@
}
@Test
- public void cannotGetFromDiffCacheForNullRevision() throws Exception {
+ public void cannotGetFromDiffCacheForNullRevision_v2() throws Exception {
NullPointerException npe =
assertThrows(
NullPointerException.class,
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index 786f737..34642d5 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -156,19 +156,6 @@
@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");
@@ -183,20 +170,13 @@
recommend(changeId);
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- 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));
- }
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
@@ -286,18 +266,6 @@
@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);
@@ -314,45 +282,21 @@
recommend(changeId);
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- if (useDiffCache) {
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.rename(
- oldPath,
- CodeOwnerStatus.PENDING,
- String.format(
- "reviewer %s is a code owner",
- AccountTemplateUtil.getAccountTemplate(user.id())),
- newPath,
- CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
- /* reasonNewPath= */ null));
- } else {
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(
- oldPath,
- CodeOwnerStatus.PENDING,
- String.format(
- "reviewer %s is a code owner",
- AccountTemplateUtil.getAccountTemplate(user.id()))),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
- }
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a code owner",
+ AccountTemplateUtil.getAccountTemplate(user.id())),
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ /* reasonNewPath= */ null));
}
@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);
@@ -369,29 +313,17 @@
recommend(changeId);
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- if (useDiffCache) {
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.rename(
- oldPath,
- CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
- /* reasonOldPath= */ null,
- newPath,
- CodeOwnerStatus.PENDING,
- String.format(
- "reviewer %s is a code owner",
- AccountTemplateUtil.getAccountTemplate(user.id()))));
- } else {
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(
- newPath,
- CodeOwnerStatus.PENDING,
- String.format(
- "reviewer %s is a code owner",
- AccountTemplateUtil.getAccountTemplate(user.id()))));
- }
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ /* reasonOldPath= */ null,
+ newPath,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a code owner",
+ AccountTemplateUtil.getAccountTemplate(user.id()))));
}
@Test
@@ -466,18 +398,6 @@
@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");
@@ -490,45 +410,21 @@
recommend(changeId);
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- if (useDiffCache) {
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.rename(
- oldPath,
- CodeOwnerStatus.APPROVED,
- String.format(
- "approved by %s who is a code owner",
- AccountTemplateUtil.getAccountTemplate(user.id())),
- newPath,
- CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
- /* reasonNewPath= */ null));
- } else {
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(
- oldPath,
- CodeOwnerStatus.APPROVED,
- String.format(
- "approved by %s who is a code owner",
- AccountTemplateUtil.getAccountTemplate(user.id()))),
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
- }
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(user.id())),
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ /* reasonNewPath= */ null));
}
@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");
@@ -541,29 +437,17 @@
recommend(changeId);
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- if (useDiffCache) {
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.rename(
- oldPath,
- CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
- /* reasonOldPath= */ null,
- newPath,
- CodeOwnerStatus.APPROVED,
- String.format(
- "approved by %s who is a code owner",
- AccountTemplateUtil.getAccountTemplate(user.id()))));
- } else {
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- FileCodeOwnerStatus.addition(
- newPath,
- CodeOwnerStatus.APPROVED,
- String.format(
- "approved by %s who is a code owner",
- AccountTemplateUtil.getAccountTemplate(user.id()))));
- }
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ /* reasonOldPath= */ null,
+ newPath,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(user.id()))));
}
@Test
@@ -744,20 +628,7 @@
@Test
public void getStatusForFileRename_noImplicitApprovalOnOldPath() throws Exception {
testImplicitApprovalOnGetStatusForFileRenameOnOldPath(
- /* implicitApprovalsEnabled= */ false,
- /* uploaderMatchesChangeOwner= */ true,
- /* useDiffCache= */ false);
- }
-
- @Test
- @GerritConfig(
- name = "experiments.enabled",
- value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
- public void getStatusForFileRename_noImplicitApprovalOnOldPath_useDiffCache() throws Exception {
- testImplicitApprovalOnGetStatusForFileRenameOnOldPath(
- /* implicitApprovalsEnabled= */ false,
- /* uploaderMatchesChangeOwner= */ true,
- /* useDiffCache= */ true);
+ /* implicitApprovalsEnabled= */ false, /* uploaderMatchesChangeOwner= */ true);
}
@Test
@@ -765,49 +636,18 @@
public void getStatusForFileRename_noImplicitApprovalOnOldPath_uploaderDoesntMatchChangeOwner()
throws Exception {
testImplicitApprovalOnGetStatusForFileRenameOnOldPath(
- /* implicitApprovalsEnabled= */ true,
- /* uploaderMatchesChangeOwner= */ false,
- /* useDiffCache= */ false);
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- @GerritConfig(
- name = "experiments.enabled",
- value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
- public void
- getStatusForFileRename_noImplicitApprovalOnOldPath_uploaderDoesntMatchChangeOwner_useDiffCache()
- throws Exception {
- testImplicitApprovalOnGetStatusForFileRenameOnOldPath(
- /* implicitApprovalsEnabled= */ true,
- /* uploaderMatchesChangeOwner= */ false,
- /* useDiffCache= */ true);
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ false);
}
@Test
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
public void getStatusForFileRename_withImplicitApprovalOnOldPath() throws Exception {
testImplicitApprovalOnGetStatusForFileRenameOnOldPath(
- /* implicitApprovalsEnabled= */ true,
- /* uploaderMatchesChangeOwner= */ true,
- /* useDiffCache= */ false);
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- @GerritConfig(
- name = "experiments.enabled",
- value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
- public void getStatusForFileRename_withImplicitApprovalOnOldPath_useDiffCache() throws Exception {
- testImplicitApprovalOnGetStatusForFileRenameOnOldPath(
- /* implicitApprovalsEnabled= */ true,
- /* uploaderMatchesChangeOwner= */ true,
- /* useDiffCache= */ true);
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ true);
}
private void testImplicitApprovalOnGetStatusForFileRenameOnOldPath(
- boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner, boolean useDiffCache)
- throws Exception {
+ boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner) throws Exception {
TestAccount changeOwner = admin;
TestAccount otherCodeOwner =
accountCreator.create(
@@ -826,69 +666,34 @@
}
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- if (useDiffCache) {
- FileCodeOwnerStatus expectedFileCodeOwnerStatus;
- if (implicitApprovalsEnabled && uploaderMatchesChangeOwner) {
- expectedFileCodeOwnerStatus =
- FileCodeOwnerStatus.rename(
- oldPath,
- CodeOwnerStatus.APPROVED,
- String.format(
- "implicitly approved by the patch set uploader %s who is a code owner",
- AccountTemplateUtil.getAccountTemplate(changeOwner.id())),
- newPath,
- CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
- /* reasonNewPath= */ null);
- } else {
- expectedFileCodeOwnerStatus =
- FileCodeOwnerStatus.rename(
- oldPath,
- CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
- newPath,
- CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- }
-
- assertThatCollection(fileCodeOwnerStatuses).containsExactly(expectedFileCodeOwnerStatus);
+ FileCodeOwnerStatus expectedFileCodeOwnerStatus;
+ if (implicitApprovalsEnabled && uploaderMatchesChangeOwner) {
+ expectedFileCodeOwnerStatus =
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "implicitly approved by the patch set uploader %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(changeOwner.id())),
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ /* reasonNewPath= */ null);
} else {
- FileCodeOwnerStatus expectedFileDeletionCodeOwnerStatus;
- if (implicitApprovalsEnabled && uploaderMatchesChangeOwner) {
- expectedFileDeletionCodeOwnerStatus =
- FileCodeOwnerStatus.deletion(
- oldPath,
- CodeOwnerStatus.APPROVED,
- String.format(
- "implicitly approved by the patch set uploader %s who is a code owner",
- AccountTemplateUtil.getAccountTemplate(changeOwner.id())));
- } else {
- expectedFileDeletionCodeOwnerStatus =
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- }
-
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- expectedFileDeletionCodeOwnerStatus,
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ expectedFileCodeOwnerStatus =
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
}
+
+ assertThatCollection(fileCodeOwnerStatuses).containsExactly(expectedFileCodeOwnerStatus);
}
@Test
public void testImplicitApprovalOnGetStatusForFileRenameOnNewPath() throws Exception {
testImplicitApprovalOnGetStatusForFileRenameOnNewPath(
- /* implicitApprovalsEnabled= */ false,
- /* uploaderMatchesChangeOwner= */ true,
- /* useDiffCache= */ false);
- }
-
- @Test
- @GerritConfig(
- name = "experiments.enabled",
- value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
- public void testImplicitApprovalOnGetStatusForFileRenameOnNewPath_useDiffCache()
- throws Exception {
- testImplicitApprovalOnGetStatusForFileRenameOnNewPath(
- /* implicitApprovalsEnabled= */ false,
- /* uploaderMatchesChangeOwner= */ true,
- /* useDiffCache= */ true);
+ /* implicitApprovalsEnabled= */ false, /* uploaderMatchesChangeOwner= */ true);
}
@Test
@@ -896,49 +701,18 @@
public void getStatusForFileRename_noImplicitApprovalOnNewPath_uploaderDoesntMatchChangeOwner()
throws Exception {
testImplicitApprovalOnGetStatusForFileRenameOnNewPath(
- /* implicitApprovalsEnabled= */ true,
- /* uploaderMatchesChangeOwner= */ false,
- /* useDiffCache= */ false);
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- @GerritConfig(
- name = "experiments.enabled",
- value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
- public void
- getStatusForFileRename_noImplicitApprovalOnNewPath_uploaderDoesntMatchChangeOwner_useDiffCache()
- throws Exception {
- testImplicitApprovalOnGetStatusForFileRenameOnNewPath(
- /* implicitApprovalsEnabled= */ true,
- /* uploaderMatchesChangeOwner= */ false,
- /* useDiffCache= */ true);
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ false);
}
@Test
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
public void getStatusForFileRename_withImplicitApprovalOnNewPath() throws Exception {
testImplicitApprovalOnGetStatusForFileRenameOnNewPath(
- /* implicitApprovalsEnabled= */ true,
- /* uploaderMatchesChangeOwner= */ true,
- /* useDiffCache= */ false);
- }
-
- @Test
- @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- @GerritConfig(
- name = "experiments.enabled",
- value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
- public void getStatusForFileRename_withImplicitApprovalOnNewPath_useDiffCache() throws Exception {
- testImplicitApprovalOnGetStatusForFileRenameOnNewPath(
- /* implicitApprovalsEnabled= */ true,
- /* uploaderMatchesChangeOwner= */ true,
- /* useDiffCache= */ true);
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ true);
}
private void testImplicitApprovalOnGetStatusForFileRenameOnNewPath(
- boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner, boolean useDiffCache)
- throws Exception {
+ boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner) throws Exception {
TestAccount changeOwner = admin;
TestAccount otherCodeOwner =
accountCreator.create(
@@ -957,49 +731,28 @@
}
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- if (useDiffCache) {
- FileCodeOwnerStatus expectedFileCodeOwnerStatus;
- if (implicitApprovalsEnabled && uploaderMatchesChangeOwner) {
- expectedFileCodeOwnerStatus =
- FileCodeOwnerStatus.rename(
- oldPath,
- CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
- /* reasonOldPath= */ null,
- newPath,
- CodeOwnerStatus.APPROVED,
- String.format(
- "implicitly approved by the patch set uploader %s who is a code owner",
- AccountTemplateUtil.getAccountTemplate(changeOwner.id())));
- } else {
- expectedFileCodeOwnerStatus =
- FileCodeOwnerStatus.rename(
- oldPath,
- CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
- newPath,
- CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- }
-
- assertThatCollection(fileCodeOwnerStatuses).containsExactly(expectedFileCodeOwnerStatus);
+ FileCodeOwnerStatus expectedFileCodeOwnerStatus;
+ if (implicitApprovalsEnabled && uploaderMatchesChangeOwner) {
+ expectedFileCodeOwnerStatus =
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ /* reasonOldPath= */ null,
+ newPath,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "implicitly approved by the patch set uploader %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(changeOwner.id())));
} else {
- FileCodeOwnerStatus expectedAddtitionFileCodeOwnerStatus;
- if (implicitApprovalsEnabled && uploaderMatchesChangeOwner) {
- expectedAddtitionFileCodeOwnerStatus =
- FileCodeOwnerStatus.addition(
- newPath,
- CodeOwnerStatus.APPROVED,
- String.format(
- "implicitly approved by the patch set uploader %s who is a code owner",
- AccountTemplateUtil.getAccountTemplate(changeOwner.id())));
- } else {
- expectedAddtitionFileCodeOwnerStatus =
- FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- }
-
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
- expectedAddtitionFileCodeOwnerStatus);
+ expectedFileCodeOwnerStatus =
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
+ newPath,
+ CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
}
+
+ assertThatCollection(fileCodeOwnerStatuses).containsExactly(expectedFileCodeOwnerStatus);
}
@Test
diff --git a/resources/Documentation/metrics.md b/resources/Documentation/metrics.md
index 05994b0..7c15669 100644
--- a/resources/Documentation/metrics.md
+++ b/resources/Documentation/metrics.md
@@ -12,8 +12,6 @@
added as a reviewer.
** `post_type':
Whether the change message was posted synchronously or asynchronously.
-* `compute_changed_files`:
- Latency for computing changed files.
* `compute_file_status`:
Latency for computing the file status for one file.
* `compute_file_statuses`:
@@ -27,8 +25,6 @@
* `extend_change_message_on_post_review`:
Latency for extending the change message with the owned path when a code owner
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`:
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index f08a932..6e3d739 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -653,6 +653,17 @@
"reviewer <GERRIT_ACCOUNT_1000096> is a code owner"
]
}
+ },
+ {
+ "change_type": "RENAMED",
+ "old_path_status" {
+ "path": "user-introduction.txt",
+ "status": "INSUFFICIENT_REVIEWERS"
+ },
+ "new_path_status" {
+ "path": "docs/user-intro.md",
+ "status": "APPROVED"
+ }
}
],
"accounts": {
@@ -1008,7 +1019,7 @@
| Field Name | | Description |
| ------------- | -------- | ----------- |
-| `change_type` | optional | The type of the file modification. Can be `ADDED`, `MODIFIED`, `DELETED`, `RENAMED` or `COPIED`. Not set if `MODIFIED`. Renamed files might appear as separate addition and deletion or with type=RENAMED. Copied files might appear as addition or with type=COPIED.
+| `change_type` | optional | The type of the file modification. Can be `ADDED`, `MODIFIED`, `DELETED`, `RENAMED` or `COPIED`. Not set if `MODIFIED`.
| `old_path_status` | optional | The code owner status for the old path as [PathCodeOwnerStatusInfo](#path-code-owner-status-info) entity. Only set if `change_type` is `DELETED` or `RENAMED`.
| `new_path_status` | optional | The code owner status for the new path as [PathCodeOwnerStatusInfo](#path-code-owner-status-info) entity. Not set if `change_type` is `DELETED`.
