Merge changes Ie703498f,I04530aed,I489c26ac,I99349b7d,I02a1e3cb, ...
* changes:
Make all callers of ChangedFiles go through getOrCompute
ChangeFiles: Allow using getFromDiffCache for initial commits
Drop possibility to pass in RevWalk into CodeOwnerBackend and ChangedFiles
BackendConfig: Remove @VisibleForTesting from getDefaultBackend()
config.md: Remove line-break that appears in the middle of a sentence
Allow to configure the path expression syntax that should be used
diff --git a/java/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressions.java b/java/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressions.java
index d2acaed..2637eda 100644
--- a/java/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressions.java
+++ b/java/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressions.java
@@ -14,10 +14,12 @@
package com.google.gerrit.plugins.codeowners.acceptance.testsuite;
+import com.google.gerrit.plugins.codeowners.backend.AbstractFileBasedCodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.FindOwnersGlobMatcher;
import com.google.gerrit.plugins.codeowners.backend.GlobMatcher;
import com.google.gerrit.plugins.codeowners.backend.PathExpressionMatcher;
+import com.google.gerrit.plugins.codeowners.backend.PathExpressions;
import com.google.gerrit.plugins.codeowners.backend.SimplePathExpressionMatcher;
import com.google.gerrit.plugins.codeowners.backend.config.BackendConfig;
import com.google.inject.Inject;
@@ -83,8 +85,20 @@
private PathExpressionMatcher getPathExpressionMatcher() {
CodeOwnerBackend defaultBackend = backendConfig.getDefaultBackend();
+ if (defaultBackend instanceof AbstractFileBasedCodeOwnerBackend) {
+ return ((AbstractFileBasedCodeOwnerBackend) defaultBackend)
+ .getDefaultPathExpressions()
+ .map(PathExpressions::getMatcher)
+ .orElseThrow(
+ () ->
+ new IllegalStateException(
+ String.format(
+ "code owner backend %s doesn't support path expressions",
+ defaultBackend.getClass().getName())));
+ }
+
return defaultBackend
- .getPathExpressionMatcher()
+ .getPathExpressionMatcher(/* branchNameKey= */ null)
.orElseThrow(
() ->
new IllegalStateException(
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackend.java b/java/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackend.java
index 15aed2d..a6fff53 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackend.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackend.java
@@ -16,9 +16,11 @@
import static java.util.Objects.requireNonNull;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Throwables;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Project;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
@@ -77,9 +79,7 @@
@Override
public final Optional<CodeOwnerConfig> getCodeOwnerConfig(
- CodeOwnerConfig.Key codeOwnerConfigKey,
- @Nullable RevWalk revWalk,
- @Nullable ObjectId revision) {
+ CodeOwnerConfig.Key codeOwnerConfigKey, @Nullable ObjectId revision) {
String fileName =
codeOwnerConfigKey.fileName().orElse(getFileName(codeOwnerConfigKey.project()));
@@ -96,33 +96,21 @@
return Optional.empty();
}
- return loadCodeOwnerConfigFile(codeOwnerConfigKey, fileName, revWalk, revision)
+ return loadCodeOwnerConfigFile(codeOwnerConfigKey, fileName, revision)
.getLoadedCodeOwnerConfig();
}
private CodeOwnerConfigFile loadCodeOwnerConfigFile(
- CodeOwnerConfig.Key codeOwnerConfigKey,
- String fileName,
- @Nullable RevWalk revWalk,
- @Nullable ObjectId revision) {
+ CodeOwnerConfig.Key codeOwnerConfigKey, String fileName, @Nullable ObjectId revision) {
try (Repository repository = repoManager.openRepository(codeOwnerConfigKey.project())) {
if (revision == null) {
return codeOwnerConfigFileFactory.loadCurrent(
fileName, codeOwnerConfigParser, repository, codeOwnerConfigKey);
}
- boolean closeRevWalk = false;
- if (revWalk == null) {
- closeRevWalk = true;
- revWalk = new RevWalk(repository);
- }
- try {
+ try (RevWalk revWalk = new RevWalk(repository)) {
return codeOwnerConfigFileFactory.load(
fileName, codeOwnerConfigParser, revWalk, revision, codeOwnerConfigKey);
- } finally {
- if (closeRevWalk) {
- revWalk.close();
- }
}
} catch (IOException e) {
throw new CodeOwnersInternalServerErrorException(
@@ -226,6 +214,28 @@
}
}
+ @Override
+ public final Optional<PathExpressionMatcher> getPathExpressionMatcher(
+ BranchNameKey branchNameKey) {
+ Optional<PathExpressions> pathExpressions =
+ codeOwnersPluginConfiguration
+ .getProjectConfig(branchNameKey.project())
+ .getPathExpressions(branchNameKey.branch());
+ boolean hasConfiguredPathExpressions = pathExpressions.isPresent();
+ if (!hasConfiguredPathExpressions) {
+ pathExpressions = getDefaultPathExpressions();
+ }
+ logger.atFine().log(
+ "using %s path expression syntax %s for project/branch %s",
+ (hasConfiguredPathExpressions ? "configured" : "default"),
+ pathExpressions.map(PathExpressions::name).orElse("<none>"),
+ branchNameKey);
+ return pathExpressions.map(PathExpressions::getMatcher);
+ }
+
+ @VisibleForTesting
+ public abstract Optional<PathExpressions> getDefaultPathExpressions();
+
private Optional<CodeOwnerConfig> upsertCodeOwnerConfigInSourceBranch(
@Nullable IdentifiedUser currentUser,
CodeOwnerConfig.Key codeOwnerConfigKey,
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
index 36a9751..fb72365 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
@@ -39,7 +39,6 @@
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;
@@ -63,16 +62,16 @@
/**
* Class to get/compute the files that have been changed in a revision.
*
- * <p>The {@link #getFromDiffCache(Project.NameKey, ObjectId)} method is retrieving the file diff
- * from the diff cache and has rename detection enabled.
+ * <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 {@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>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>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>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.
@@ -113,104 +112,100 @@
* Returns the changed files for the given revision.
*
* <p>By default the changed files are computed on access (see {@link #compute(Project.NameKey,
- * ObjectId)}).
+ * 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)}).
+ * #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)
- throws IOException, PatchListNotAvailableException, DiffNotAvailableException {
+ public ImmutableList<ChangedFile> getOrCompute(
+ Project.NameKey project, ObjectId revision, MergeCommitStrategy mergeCommitStrategy)
+ throws IOException, DiffNotAvailableException {
+ requireNonNull(project, "project");
+ requireNonNull(revision, "revision");
+ requireNonNull(mergeCommitStrategy, "mergeCommitStrategy");
+
if (experimentFeatures.isFeatureEnabled(CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)) {
- return getFromDiffCache(project, revision);
+ return getFromDiffCache(project, revision, mergeCommitStrategy);
}
- return compute(project, revision);
+ return compute(project, revision, mergeCommitStrategy);
}
/**
- * Computes the files that have been changed in the given revision.
+ * Returns the changed files for the given revision.
*
- * <p>The diff is computed against the parent commit.
+ * <p>Uses the configured merge commit strategy.
*
- * <p>Rename detection is disabled.
+ * @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
- * @throws PatchListNotAvailableException thrown if getting the patch list for a merge commit
- * against the auto merge failed
+ * @see #getOrCompute(Project.NameKey, ObjectId, MergeCommitStrategy)
*/
- public ImmutableList<ChangedFile> compute(RevisionResource revisionResource)
- throws IOException, PatchListNotAvailableException {
+ public ImmutableList<ChangedFile> getOrCompute(RevisionResource revisionResource)
+ throws IOException, DiffNotAvailableException {
requireNonNull(revisionResource, "revisionResource");
- return compute(revisionResource.getProject(), revisionResource.getPatchSet().commitId());
+ return getOrCompute(revisionResource.getProject(), revisionResource.getPatchSet().commitId());
}
/**
- * Computes the files that have been changed in the given revision.
+ * Computed the changed files for the given revision.
*
- * <p>The diff is computed against the parent commit.
- *
- * <p>Rename detection is disabled.
+ * <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
- * @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
- * @throws PatchListNotAvailableException thrown if getting the patch list for a merge commit
- * against the auto merge failed
+ * @param mergeCommitStrategy the merge commit strategy that should be used to compute the changed
+ * files for merge commits
+ * @return the changed files
*/
- public ImmutableList<ChangedFile> compute(Project.NameKey project, ObjectId revision)
- throws IOException, PatchListNotAvailableException {
+ private ImmutableList<ChangedFile> compute(
+ Project.NameKey project, ObjectId revision, MergeCommitStrategy mergeCommitStrategy)
+ throws IOException {
requireNonNull(project, "project");
requireNonNull(revision, "revision");
-
- try (Repository repository = repoManager.openRepository(project);
- RevWalk revWalk = new RevWalk(repository)) {
- RevCommit revCommit = revWalk.parseCommit(revision);
- return compute(project, repository.getConfig(), revWalk, revCommit);
- }
- }
-
- public ImmutableList<ChangedFile> compute(
- Project.NameKey project, Config repoConfig, RevWalk revWalk, RevCommit revCommit)
- throws IOException {
- return compute(
- project,
- repoConfig,
- revWalk,
- revCommit,
- codeOwnersPluginConfiguration.getProjectConfig(project).getMergeCommitStrategy());
- }
-
- public ImmutableList<ChangedFile> compute(
- Project.NameKey project,
- Config repoConfig,
- RevWalk revWalk,
- RevCommit revCommit,
- MergeCommitStrategy mergeCommitStrategy)
- throws IOException {
- requireNonNull(project, "project");
- requireNonNull(repoConfig, "repoConfig");
- requireNonNull(revWalk, "revWalk");
- requireNonNull(revCommit, "revCommit");
requireNonNull(mergeCommitStrategy, "mergeCommitStrategy");
logger.atFine().log(
- "computing changed files for revision %s in project %s", revCommit.name(), project);
+ "computing changed files for revision %s in project %s", revision.name(), project);
- if (revCommit.getParentCount() > 1
- && MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION.equals(mergeCommitStrategy)) {
- RevCommit autoMergeCommit = getAutoMergeCommit(project, revCommit);
- return compute(repoConfig, revWalk, revCommit, autoMergeCommit);
+ 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);
}
-
- RevCommit baseCommit = revCommit.getParentCount() > 0 ? revCommit.getParent(0) : null;
- return compute(repoConfig, revWalk, revCommit, baseCommit);
}
private RevCommit getAutoMergeCommit(Project.NameKey project, RevCommit mergeCommit)
@@ -273,19 +268,14 @@
* Gets the changed files from the diff cache.
*
* <p>Rename detection is enabled.
- *
- * @throws IllegalStateException thrown if invoked for an initial revision
*/
@VisibleForTesting
- ImmutableList<ChangedFile> getFromDiffCache(Project.NameKey project, ObjectId revision)
+ ImmutableList<ChangedFile> getFromDiffCache(
+ Project.NameKey project, ObjectId revision, MergeCommitStrategy mergeCommitStrategy)
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();
+ requireNonNull(mergeCommitStrategy, "mergeCommitStrategy");
try (Timer0.Context ctx = codeOwnerMetrics.getChangedFiles.start()) {
Map<String, FileDiffOutput> fileDiffOutputs;
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 8266681..d11767a 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -48,7 +48,6 @@
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;
import com.google.gerrit.server.permissions.ProjectPermission;
@@ -162,7 +161,7 @@
ownedPaths = ownedPaths.limit(limit);
}
return ownedPaths.collect(toImmutableList());
- } catch (IOException | PatchListNotAvailableException | DiffNotAvailableException e) {
+ } catch (IOException | DiffNotAvailableException e) {
throw new CodeOwnersInternalServerErrorException(
String.format(
"failed to compute owned paths of patch set %s for account %d",
@@ -178,8 +177,7 @@
* @return whether the given change has sufficient code owner approvals to be submittable
*/
public boolean isSubmittable(ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, PatchListNotAvailableException,
- DiffNotAvailableException {
+ throws ResourceConflictException, IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
logger.atFine().log(
"checking if change %d in project %s is submittable",
@@ -225,8 +223,7 @@
*/
public ImmutableSet<FileCodeOwnerStatus> getFileStatusesAsSet(
ChangeNotes changeNotes, int start, int limit)
- throws ResourceConflictException, IOException, PatchListNotAvailableException,
- DiffNotAvailableException {
+ throws ResourceConflictException, IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatuses.start()) {
logger.atFine().log(
@@ -279,8 +276,7 @@
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, PatchListNotAvailableException,
- DiffNotAvailableException {
+ throws ResourceConflictException, IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
try (Timer0.Context ctx = codeOwnerMetrics.prepareFileStatusComputation.start()) {
logger.atFine().log(
@@ -400,8 +396,7 @@
@VisibleForTesting
public Stream<FileCodeOwnerStatus> getFileStatusesForAccount(
ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId)
- throws ResourceConflictException, IOException, PatchListNotAvailableException,
- DiffNotAvailableException {
+ throws ResourceConflictException, IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
requireNonNull(patchSet, "patchSet");
requireNonNull(accountId, "accountId");
@@ -470,7 +465,7 @@
private Stream<FileCodeOwnerStatus> getAllPathsAsApproved(
ChangeNotes changeNotes, PatchSet patchSet, String reason)
- throws IOException, PatchListNotAvailableException, DiffNotAvailableException {
+ throws IOException, DiffNotAvailableException {
logger.atFine().log("all paths are approved (reason = %s)", reason);
return changedFiles.getOrCompute(changeNotes.getProjectName(), patchSet.commitId()).stream()
.map(
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerBackend.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerBackend.java
index 3775f89..22d91bc 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerBackend.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerBackend.java
@@ -15,13 +15,13 @@
package com.google.gerrit.plugins.codeowners.backend;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.restapi.NotImplementedException;
import com.google.gerrit.server.IdentifiedUser;
import java.nio.file.Path;
import java.util.Optional;
import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.revwalk.RevWalk;
/**
* Interface for code owner backends.
@@ -51,25 +51,8 @@
* {@code null} the code owner config is loaded from the current revision of the branch
* @return code owner config for the given key if it exists, otherwise {@link Optional#empty()}
*/
- default Optional<CodeOwnerConfig> getCodeOwnerConfig(
- CodeOwnerConfig.Key codeOwnerConfigKey, @Nullable ObjectId revision) {
- return getCodeOwnerConfig(codeOwnerConfigKey, /* revWalk= */ null, revision);
- }
-
- /**
- * Gets the code owner config for the given key if it exists.
- *
- * @param codeOwnerConfigKey the code owner config key for which the code owner config should be
- * returned
- * @param revWalk optional rev walk, if given this rev walk is used to load the given revision
- * @param revision the branch revision from which the code owner config should be loaded, if
- * {@code null} the code owner config is loaded from the current revision of the branch
- * @return code owner config for the given key if it exists, otherwise {@link Optional#empty()}
- */
Optional<CodeOwnerConfig> getCodeOwnerConfig(
- CodeOwnerConfig.Key codeOwnerConfigKey,
- @Nullable RevWalk revWalk,
- @Nullable ObjectId revision);
+ CodeOwnerConfig.Key codeOwnerConfigKey, @Nullable ObjectId revision);
/**
* Returns the absolute file path of the specified code owner config.
@@ -111,8 +94,11 @@
* <p>May return {@link Optional#empty()} if path expressions are not supported by the code owner
* backend. It this case all {@link CodeOwnerSet}s that have path expressions are ignored and will
* not have any effect.
+ *
+ * @param branchNameKey project and branch for which the path expression matcher should be
+ * returned
*/
- Optional<PathExpressionMatcher> getPathExpressionMatcher();
+ Optional<PathExpressionMatcher> getPathExpressionMatcher(BranchNameKey branchNameKey);
/**
* Replaces the old email in the given code owner config file content with the new email.
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExperimentFeaturesConstants.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExperimentFeaturesConstants.java
index a0e82ad..01bf7d6 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExperimentFeaturesConstants.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExperimentFeaturesConstants.java
@@ -24,7 +24,8 @@
* 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)
+ * 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";
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
index 8272bb6..fcd207a 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
@@ -131,7 +131,7 @@
.getProjectConfig(codeOwnerConfigKey.project())
.getBackend(codeOwnerConfigKey.branchNameKey().branch());
return codeOwnerBackend
- .getPathExpressionMatcher()
+ .getPathExpressionMatcher(codeOwnerConfigKey.branchNameKey())
.orElse((pathExpression, relativePath) -> false);
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathExpressions.java b/java/com/google/gerrit/plugins/codeowners/backend/PathExpressions.java
new file mode 100644
index 0000000..d378a7e
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/PathExpressions.java
@@ -0,0 +1,62 @@
+// 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;
+
+import com.google.gerrit.common.Nullable;
+import java.util.Locale;
+import java.util.Optional;
+
+/** Enum listing the supported options for path expressions syntaxes. */
+public enum PathExpressions {
+ /** Simple path expressions as implemented by {@link SimplePathExpressionMatcher}. */
+ SIMPLE(SimplePathExpressionMatcher.INSTANCE),
+
+ /** Plain glob path expressions as implemented by {@link GlobMatcher}. */
+ GLOB(GlobMatcher.INSTANCE),
+
+ /**
+ * Find-owners compatible glob path expressions as implemented by {@link FindOwnersGlobMatcher}.
+ */
+ FIND_OWNERS_GLOB(FindOwnersGlobMatcher.INSTANCE);
+
+ private final PathExpressionMatcher matcher;
+
+ private PathExpressions(PathExpressionMatcher matcher) {
+ this.matcher = matcher;
+ }
+
+ /** Gets the path expression matcher. */
+ public PathExpressionMatcher getMatcher() {
+ return matcher;
+ }
+
+ /**
+ * Tries to parse a string as a {@link PathExpressions} enum.
+ *
+ * @param value the string value to be parsed
+ * @return the parsed {@link PathExpressions} enum, {@link Optional#empty()} if the given value
+ * couldn't be parsed as {@link PathExpressions} enum
+ */
+ public static Optional<PathExpressions> tryParse(@Nullable String value) {
+ if (value == null) {
+ return Optional.empty();
+ }
+ try {
+ return Optional.of(PathExpressions.valueOf(value.toUpperCase(Locale.US)));
+ } catch (IllegalArgumentException e) {
+ return Optional.empty();
+ }
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/BackendConfig.java b/java/com/google/gerrit/plugins/codeowners/backend/config/BackendConfig.java
index cbeb83d..05c9f0e 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/BackendConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/BackendConfig.java
@@ -18,6 +18,7 @@
import static java.util.Objects.requireNonNull;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.BranchNameKey;
@@ -26,6 +27,7 @@
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackendId;
+import com.google.gerrit.plugins.codeowners.backend.PathExpressions;
import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.gerrit.server.git.validators.CommitValidationMessage;
import com.google.gerrit.server.git.validators.ValidationMessage;
@@ -57,6 +59,7 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@VisibleForTesting public static final String KEY_BACKEND = "backend";
+ @VisibleForTesting public static final String KEY_PATH_EXPRESSIONS = "pathExpressions";
private final String pluginName;
private final DynamicMap<CodeOwnerBackend> codeOwnerBackends;
@@ -64,6 +67,9 @@
/** The name of the configured code owners default backend. */
private final String defaultBackendName;
+ /** The configured default path expressions. */
+ private final Optional<PathExpressions> defaultPathExpressions;
+
@Inject
BackendConfig(
@PluginName String pluginName,
@@ -76,6 +82,19 @@
pluginConfigFactory
.getFromGerritConfig(pluginName)
.getString(KEY_BACKEND, CodeOwnerBackendId.FIND_OWNERS.getBackendId());
+
+ String defaultPathExpressionsName =
+ pluginConfigFactory
+ .getFromGerritConfig(pluginName)
+ .getString(KEY_PATH_EXPRESSIONS, /* defaultValue= */ null);
+ this.defaultPathExpressions = PathExpressions.tryParse(defaultPathExpressionsName);
+ if (!Strings.isNullOrEmpty(defaultPathExpressionsName)
+ && !this.defaultPathExpressions.isPresent()) {
+ logger.atWarning().log(
+ "Path expressions '%s' that are configured in gerrit.config"
+ + " (parameter plugin.%s.%s) not found.",
+ defaultPathExpressionsName, pluginName, KEY_PATH_EXPRESSIONS);
+ }
}
/**
@@ -105,6 +124,18 @@
}
}
+ String pathExpressionsName =
+ projectLevelConfig.getString(SECTION_CODE_OWNERS, null, KEY_PATH_EXPRESSIONS);
+ if (!Strings.isNullOrEmpty(pathExpressionsName)
+ && !PathExpressions.tryParse(pathExpressionsName).isPresent()) {
+ validationMessages.add(
+ new CommitValidationMessage(
+ String.format(
+ "Path expressions '%s' that are configured in %s (parameter %s.%s) not found.",
+ pathExpressionsName, fileName, SECTION_CODE_OWNERS, KEY_PATH_EXPRESSIONS),
+ ValidationMessage.Type.ERROR));
+ }
+
for (String subsection : projectLevelConfig.getSubsections(SECTION_CODE_OWNERS)) {
backendName = projectLevelConfig.getString(SECTION_CODE_OWNERS, subsection, KEY_BACKEND);
if (backendName != null) {
@@ -117,6 +148,22 @@
ValidationMessage.Type.ERROR));
}
}
+
+ pathExpressionsName =
+ projectLevelConfig.getString(SECTION_CODE_OWNERS, subsection, KEY_PATH_EXPRESSIONS);
+ if (!Strings.isNullOrEmpty(pathExpressionsName)
+ && !PathExpressions.tryParse(pathExpressionsName).isPresent()) {
+ validationMessages.add(
+ new CommitValidationMessage(
+ String.format(
+ "Path expressions '%s' that are configured in %s (parameter %s.%s.%s) not found.",
+ pathExpressionsName,
+ fileName,
+ SECTION_CODE_OWNERS,
+ subsection,
+ KEY_PATH_EXPRESSIONS),
+ ValidationMessage.Type.ERROR));
+ }
}
return ImmutableList.copyOf(validationMessages);
@@ -211,7 +258,6 @@
}
/** Gets the default code owner backend. */
- @VisibleForTesting
public CodeOwnerBackend getDefaultBackend() {
return lookupBackend(defaultBackendName)
.orElseThrow(
@@ -233,4 +279,93 @@
// plugin name.
return Optional.ofNullable(codeOwnerBackends.get("gerrit", backendName));
}
+
+ /**
+ * Gets the path expressions that are configured for the given branch.
+ *
+ * <p>The path expressions configuration is evaluated in the following order:
+ *
+ * <ul>
+ * <li>path expressions for branch by full name (with inheritance)
+ * <li>path expressions for branch by short name (with inheritance)
+ * </ul>
+ *
+ * @param pluginConfig the plugin config from which the path expressions should be read.
+ * @param branch the project and branch for which the configured path expressions should be read
+ * @return the path expressions that are configured for the given branch, {@link Optional#empty()}
+ * if there is no branch-specific path expressions configuration
+ */
+ Optional<PathExpressions> getPathExpressionsForBranch(Config pluginConfig, BranchNameKey branch) {
+ requireNonNull(pluginConfig, "pluginConfig");
+ requireNonNull(branch, "branch");
+
+ // check for branch specific path expressions by full branch name
+ Optional<PathExpressions> pathExpressions =
+ getPathExpressionsForBranch(pluginConfig, branch.project(), branch.branch());
+ if (!pathExpressions.isPresent()) {
+ // check for branch specific path expressions by short branch name
+ pathExpressions =
+ getPathExpressionsForBranch(pluginConfig, branch.project(), branch.shortName());
+ }
+ return pathExpressions;
+ }
+
+ private Optional<PathExpressions> getPathExpressionsForBranch(
+ Config pluginConfig, Project.NameKey project, String branch) {
+ String pathExpressionsName =
+ pluginConfig.getString(SECTION_CODE_OWNERS, branch, KEY_PATH_EXPRESSIONS);
+ if (Strings.isNullOrEmpty(pathExpressionsName)) {
+ return Optional.empty();
+ }
+
+ Optional<PathExpressions> pathExpressions = PathExpressions.tryParse(pathExpressionsName);
+ if (!pathExpressions.isPresent()) {
+ logger.atWarning().log(
+ "Path expressions '%s' that are configured for project %s in"
+ + " %s.config (parameter %s.%s.%s) not found. Falling back to default path"
+ + " expressions.",
+ pathExpressionsName,
+ project,
+ pluginName,
+ SECTION_CODE_OWNERS,
+ branch,
+ KEY_PATH_EXPRESSIONS);
+ }
+ return pathExpressions;
+ }
+
+ /**
+ * Gets the path expressions that are configured for the given project.
+ *
+ * @param pluginConfig the plugin config from which the path expressions should be read.
+ * @param project the project for which the configured path expressions should be read
+ * @return the path expressions that are configured for the given project, {@link
+ * Optional#empty()} if there is no project-specific path expression configuration
+ */
+ Optional<PathExpressions> getPathExpressionsForProject(
+ Config pluginConfig, Project.NameKey project) {
+ requireNonNull(pluginConfig, "pluginConfig");
+ requireNonNull(project, "project");
+
+ String pathExpressionsName =
+ pluginConfig.getString(SECTION_CODE_OWNERS, null, KEY_PATH_EXPRESSIONS);
+ if (Strings.isNullOrEmpty(pathExpressionsName)) {
+ return Optional.empty();
+ }
+
+ Optional<PathExpressions> pathExpressions = PathExpressions.tryParse(pathExpressionsName);
+ if (!pathExpressions.isPresent()) {
+ logger.atWarning().log(
+ "Path expressions '%s' that are configured for project %s in"
+ + " %s.config (parameter %s.%s) not found. Falling back to default path"
+ + " expressions.",
+ pathExpressionsName, project, pluginName, SECTION_CODE_OWNERS, KEY_PATH_EXPRESSIONS);
+ }
+ return pathExpressions;
+ }
+
+ /** Gets the default path expressions. */
+ public Optional<PathExpressions> getDefaultPathExpressions() {
+ return defaultPathExpressions;
+ }
}
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 c6181b5..a92e0eb 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
@@ -26,6 +26,7 @@
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.CommitValidationMessage;
import com.google.gerrit.server.git.validators.ValidationMessage;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.project.ProjectLevelConfig;
import com.google.gerrit.server.project.ProjectState;
@@ -97,7 +98,7 @@
exceptionMessage(fileName, cfg.getRevision()), validationMessages);
}
return ImmutableList.of();
- } catch (IOException | ConfigInvalidException e) {
+ } catch (IOException | DiffNotAvailableException | ConfigInvalidException e) {
String errorMessage =
String.format(
"failed to validate file %s for revision %s in ref %s of project %s",
@@ -124,12 +125,10 @@
* @param fileName the name of the file
*/
private boolean isFileChanged(CommitReceivedEvent receiveEvent, String fileName)
- throws IOException {
+ throws IOException, DiffNotAvailableException {
return changedFiles
- .compute(
+ .getOrCompute(
receiveEvent.project.getNameKey(),
- receiveEvent.repoConfig,
- receiveEvent.revWalk,
receiveEvent.commit,
MergeCommitStrategy.ALL_CHANGED_FILES)
.stream()
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
index 0500d7c..dbf69c1 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
@@ -33,6 +33,7 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException;
import com.google.gerrit.plugins.codeowners.backend.EnableImplicitApprovals;
import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners;
+import com.google.gerrit.plugins.codeowners.backend.PathExpressions;
import com.google.gerrit.plugins.codeowners.common.CodeOwnerConfigValidationPolicy;
import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy;
import com.google.gerrit.server.account.Emails;
@@ -87,6 +88,8 @@
@Nullable private Boolean isDisabled;
private Map<String, CodeOwnerBackend> backendByBranch = new HashMap<>();
@Nullable private CodeOwnerBackend backend;
+ private Map<String, Optional<PathExpressions>> pathExpressionsByBranch = new HashMap<>();
+ @Nullable private Optional<PathExpressions> pathExpressions;
@Nullable private Boolean implicitApprovalsEnabled;
@Nullable private RequiredApproval requiredApproval;
@Nullable private ImmutableSortedSet<RequiredApproval> overrideApprovals;
@@ -461,6 +464,74 @@
}
/**
+ * Returns the configured {@link PathExpressions} for the given branch.
+ *
+ * <p>The path expression configuration is evaluated in the following order:
+ *
+ * <ul>
+ * <li>path expression configuration for branch (with inheritance, first by full branch name,
+ * then by short branch name)
+ * <li>path expressions configuration for project (with inheritance)
+ * </ul>
+ *
+ * <p>The first path expressions configuration that exists counts and the evaluation is stopped.
+ *
+ * @param branchName the branch for which the configured path expressions should be returned
+ * @return the {@link PathExpressions} that should be used for the branch, {@link
+ * Optional#empty()} if no path expressions are configured for the branch
+ */
+ public Optional<PathExpressions> getPathExpressions(String branchName) {
+ requireNonNull(branchName, "branchName");
+
+ BranchNameKey branchNameKey = BranchNameKey.create(projectName, branchName);
+ return pathExpressionsByBranch.computeIfAbsent(
+ branchNameKey.branch(),
+ b -> {
+ Optional<PathExpressions> pathExpressions =
+ backendConfig.getPathExpressionsForBranch(
+ pluginConfig, BranchNameKey.create(projectName, branchName));
+ if (pathExpressions.isPresent()) {
+ return pathExpressions;
+ }
+ return getPathExpressions();
+ });
+ }
+
+ /**
+ * Returns the configured {@link PathExpressions}.
+ *
+ * <p>The path expression configuration is evaluated in the following order:
+ *
+ * <ul>
+ * <li>path expression configuration for project (with inheritance)
+ * <li>default path expressions (globally configured path expressions)
+ * </ul>
+ *
+ * <p>The first path expression configuration that exists counts and the evaluation is stopped.
+ *
+ * @return the {@link PathExpressions} that should be used, {@link Optional#empty()} if no path
+ * expressions are configured
+ */
+ public Optional<PathExpressions> getPathExpressions() {
+ if (pathExpressions == null) {
+ pathExpressions = readPathExpressions();
+ }
+ return pathExpressions;
+ }
+
+ private Optional<PathExpressions> readPathExpressions() {
+ // check if project specific path expressions are configured
+ Optional<PathExpressions> pathExpressions =
+ backendConfig.getPathExpressionsForProject(pluginConfig, projectName);
+ if (pathExpressions.isPresent()) {
+ return pathExpressions;
+ }
+
+ // fall back to the default path expressions
+ return backendConfig.getDefaultPathExpressions();
+ }
+
+ /**
* Checks whether implicit code owner approvals are enabled.
*
* <p>If enabled, an implict code owner approval from the change owner is assumed if the last
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersBackend.java b/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersBackend.java
index 0e923d7..513bd12 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersBackend.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersBackend.java
@@ -18,8 +18,7 @@
import com.google.gerrit.plugins.codeowners.backend.AbstractFileBasedCodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigFile;
-import com.google.gerrit.plugins.codeowners.backend.FindOwnersGlobMatcher;
-import com.google.gerrit.plugins.codeowners.backend.PathExpressionMatcher;
+import com.google.gerrit.plugins.codeowners.backend.PathExpressions;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -63,8 +62,8 @@
}
@Override
- public Optional<PathExpressionMatcher> getPathExpressionMatcher() {
- return Optional.of(FindOwnersGlobMatcher.INSTANCE);
+ public Optional<PathExpressions> getDefaultPathExpressions() {
+ return Optional.of(PathExpressions.FIND_OWNERS_GLOB);
}
@Override
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/proto/ProtoBackend.java b/java/com/google/gerrit/plugins/codeowners/backend/proto/ProtoBackend.java
index b273ab2..ccb59df 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/proto/ProtoBackend.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/proto/ProtoBackend.java
@@ -18,8 +18,7 @@
import com.google.gerrit.plugins.codeowners.backend.AbstractFileBasedCodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigFile;
-import com.google.gerrit.plugins.codeowners.backend.PathExpressionMatcher;
-import com.google.gerrit.plugins.codeowners.backend.SimplePathExpressionMatcher;
+import com.google.gerrit.plugins.codeowners.backend.PathExpressions;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -67,7 +66,7 @@
}
@Override
- public Optional<PathExpressionMatcher> getPathExpressionMatcher() {
- return Optional.of(SimplePathExpressionMatcher.INSTANCE);
+ public Optional<PathExpressions> getDefaultPathExpressions() {
+ return Optional.of(PathExpressions.SIMPLE);
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
index 1eaaed6..1b931ba 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFiles.java
@@ -41,7 +41,6 @@
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.validation.CodeOwnerConfigValidator;
import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.validators.CommitValidationMessage;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -55,8 +54,6 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
-import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.revwalk.RevWalk;
/**
* REST endpoint that checks/validates the code owner config files in a project.
@@ -75,7 +72,6 @@
private final Provider<CurrentUser> currentUser;
private final PermissionBackend permissionBackend;
- private final GitRepositoryManager repoManager;
private final Provider<ListBranches> listBranches;
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final CodeOwnerConfigScanner.Factory codeOwnerConfigScannerFactory;
@@ -85,14 +81,12 @@
public CheckCodeOwnerConfigFiles(
Provider<CurrentUser> currentUser,
PermissionBackend permissionBackend,
- GitRepositoryManager repoManager,
Provider<ListBranches> listBranches,
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
CodeOwnerConfigScanner.Factory codeOwnerConfigScannerFactory,
CodeOwnerConfigValidator codeOwnerConfigValidator) {
this.currentUser = currentUser;
this.permissionBackend = permissionBackend;
- this.repoManager = repoManager;
this.listBranches = listBranches;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.codeOwnerConfigScannerFactory = codeOwnerConfigScannerFactory;
@@ -126,25 +120,22 @@
validateInput(projectResource.getNameKey(), branches, input);
- try (Repository repo = repoManager.openRepository(projectResource.getNameKey());
- RevWalk revWalk = new RevWalk(repo)) {
- ImmutableMap.Builder<String, Map<String, List<ConsistencyProblemInfo>>>
- resultsByBranchBuilder = ImmutableMap.builder();
- branches.stream()
- .filter(branchNameKey -> shouldValidateBranch(input, branchNameKey))
- .filter(
- branchNameKey ->
- validateDisabledBranches(input)
- || !codeOwnersPluginConfiguration
- .getProjectConfig(branchNameKey.project())
- .isDisabled(branchNameKey.branch()))
- .forEach(
- branchNameKey ->
- resultsByBranchBuilder.put(
- branchNameKey.branch(),
- checkBranch(revWalk, input.path, branchNameKey, input.verbosity)));
- return Response.ok(resultsByBranchBuilder.build());
- }
+ ImmutableMap.Builder<String, Map<String, List<ConsistencyProblemInfo>>> resultsByBranchBuilder =
+ ImmutableMap.builder();
+ branches.stream()
+ .filter(branchNameKey -> shouldValidateBranch(input, branchNameKey))
+ .filter(
+ branchNameKey ->
+ validateDisabledBranches(input)
+ || !codeOwnersPluginConfiguration
+ .getProjectConfig(branchNameKey.project())
+ .isDisabled(branchNameKey.branch()))
+ .forEach(
+ branchNameKey ->
+ resultsByBranchBuilder.put(
+ branchNameKey.branch(),
+ checkBranch(input.path, branchNameKey, input.verbosity)));
+ return Response.ok(resultsByBranchBuilder.build());
}
private ImmutableSet<BranchNameKey> branches(ProjectResource projectResource)
@@ -156,7 +147,6 @@
}
private Map<String, List<ConsistencyProblemInfo>> checkBranch(
- RevWalk revWalk,
String pathGlob,
BranchNameKey branchNameKey,
@Nullable ConsistencyProblemInfo.Status verbosity) {
@@ -177,7 +167,7 @@
problemsByPath.putAll(
codeOwnerBackend.getFilePath(codeOwnerConfig.key()).toString(),
checkCodeOwnerConfig(
- branchNameKey, revWalk, codeOwnerBackend, codeOwnerConfig, verbosity));
+ branchNameKey, codeOwnerBackend, codeOwnerConfig, verbosity));
return true;
},
(codeOwnerConfigFilePath, configInvalidException) -> {
@@ -193,17 +183,12 @@
private ImmutableList<ConsistencyProblemInfo> checkCodeOwnerConfig(
BranchNameKey branchNameKey,
- RevWalk revWalk,
CodeOwnerBackend codeOwnerBackend,
CodeOwnerConfig codeOwnerConfig,
@Nullable ConsistencyProblemInfo.Status verbosity) {
return codeOwnerConfigValidator
.validateCodeOwnerConfig(
- branchNameKey,
- revWalk,
- currentUser.get().asIdentifiedUser(),
- codeOwnerBackend,
- codeOwnerConfig)
+ branchNameKey, currentUser.get().asIdentifiedUser(), codeOwnerBackend, codeOwnerConfig)
.map(
commitValidationMessage ->
createConsistencyProblemInfo(commitValidationMessage, verbosity))
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFilesInRevision.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFilesInRevision.java
index db939c4..a53b19b 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFilesInRevision.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFilesInRevision.java
@@ -29,7 +29,7 @@
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
@@ -76,7 +76,7 @@
@Override
public Response<Map<String, List<ConsistencyProblemInfo>>> apply(
RevisionResource revisionResource, CheckCodeOwnerConfigFilesInRevisionInput input)
- throws IOException, PatchListNotAvailableException {
+ throws IOException, DiffNotAvailableException {
logger.atFine().log(
"checking code owner config files for revision %d of change %d (path = %s)",
revisionResource.getPatchSet().number(),
@@ -95,7 +95,7 @@
RevWalk rw = new RevWalk(repository)) {
RevCommit commit = rw.parseCommit(revisionResource.getPatchSet().commitId());
return Response.ok(
- changedFiles.compute(revisionResource.getProject(), commit).stream()
+ changedFiles.getOrCompute(revisionResource.getProject(), commit).stream()
// filter out deletions (files without new path)
.filter(changedFile -> changedFile.newPath().isPresent())
// filter out non code owner config files
@@ -122,7 +122,6 @@
codeOwnerBackend,
revisionResource.getChange().getDest(),
changedFile,
- rw,
commit)
.map(
commitValidationMessage ->
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java
index ad0b1ad..e19192b 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java
@@ -28,6 +28,7 @@
import com.google.gerrit.plugins.codeowners.restapi.CodeOwnersInChangeCollection.PathResource;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -68,7 +69,8 @@
@Override
public PathResource parse(RevisionResource revisionResource, IdString id)
- throws RestApiException, IOException, PatchListNotAvailableException {
+ throws RestApiException, IOException, PatchListNotAvailableException,
+ DiffNotAvailableException {
// Check if the file exists in the revision only after creating the path resource. This way we
// get a more specific error response for invalid paths ('400 Bad Request' instead of a '404 Not
// Found').
@@ -100,8 +102,9 @@
private void checkThatFileExists(
RevisionResource revisionResource, PathResource pathResource, IdString id)
- throws RestApiException, IOException, PatchListNotAvailableException {
- if (!changedFiles.compute(revisionResource).stream()
+ throws RestApiException, IOException, PatchListNotAvailableException,
+ DiffNotAvailableException {
+ if (!changedFiles.getOrCompute(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 6292194..1666568 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -66,6 +66,7 @@
import com.google.gerrit.server.logging.TraceContext;
import com.google.gerrit.server.logging.TraceContext.TraceTimer;
import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.ProjectPermission;
@@ -82,7 +83,6 @@
import java.util.Optional;
import java.util.stream.Stream;
import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
@@ -204,8 +204,6 @@
validationResult =
validateCodeOwnerConfig(
receiveEvent.getBranchNameKey(),
- receiveEvent.repoConfig,
- receiveEvent.revWalk,
receiveEvent.commit,
receiveEvent.user,
codeOwnerConfigValidationPolicy.isForced(),
@@ -294,8 +292,6 @@
validationResult =
validateCodeOwnerConfig(
branchNameKey,
- repository.getConfig(),
- revWalk,
commit,
patchSetUploader,
codeOwnerConfigValidationPolicy.isForced(),
@@ -337,7 +333,6 @@
*
* @param branchNameKey the project and branch that contains the provided commit or for which the
* commit is being pushed
- * @param revWalk the rev walk that should be used to load revCommit
* @param revCommit the commit for which newly added and modified code owner configs should be
* validated
* @param user user for which the code owner visibility checks should be performed
@@ -348,8 +343,6 @@
*/
private Optional<ValidationResult> validateCodeOwnerConfig(
BranchNameKey branchNameKey,
- Config repoConfig,
- RevWalk revWalk,
RevCommit revCommit,
IdentifiedUser user,
boolean force,
@@ -417,12 +410,8 @@
// MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION is configured.
ImmutableList<ChangedFile> modifiedCodeOwnerConfigFiles =
changedFiles
- .compute(
- branchNameKey.project(),
- repoConfig,
- revWalk,
- revCommit,
- MergeCommitStrategy.ALL_CHANGED_FILES)
+ .getOrCompute(
+ branchNameKey.project(), revCommit, MergeCommitStrategy.ALL_CHANGED_FILES)
.stream()
// filter out deletions (files without new path)
.filter(changedFile -> changedFile.newPath().isPresent())
@@ -448,12 +437,7 @@
.flatMap(
changedFile ->
validateCodeOwnerConfig(
- user,
- codeOwnerBackend,
- branchNameKey,
- changedFile,
- revWalk,
- revCommit))));
+ user, codeOwnerBackend, branchNameKey, changedFile, revCommit))));
} catch (InvalidPluginConfigurationException e) {
// If the code-owners plugin configuration is invalid we cannot get the code owners backend
// and hence we are not able to detect and validate code owner config files. Instead of
@@ -473,7 +457,7 @@
"code-owners plugin configuration is invalid,"
+ " cannot validate code owner config files",
ValidationMessage.Type.WARNING)));
- } catch (IOException e) {
+ } catch (IOException | DiffNotAvailableException e) {
String errorMessage =
String.format(
"failed to validate code owner config files in revision %s"
@@ -491,7 +475,6 @@
* @param codeOwnerBackend the code owner backend from which the code owner config can be loaded
* @param branchNameKey the project and branch of the code owner config
* @param changedFile the changed file that represents the code owner config
- * @param revWalk the rev walk that should be used to load revCommit
* @param revCommit the commit from which the code owner config should be loaded
* @return a stream of validation messages that describe issues with the code owner config, an
* empty stream if there are no issues
@@ -501,13 +484,11 @@
CodeOwnerBackend codeOwnerBackend,
BranchNameKey branchNameKey,
ChangedFile changedFile,
- RevWalk revWalk,
RevCommit revCommit) {
requireNonNull(user, "user");
requireNonNull(codeOwnerBackend, "codeOwnerBackend");
requireNonNull(branchNameKey, "branchNameKey");
requireNonNull(changedFile, "changedFile");
- requireNonNull(revWalk, "revWalk");
requireNonNull(revCommit, "revCommit");
if (!changedFile.newPath().isPresent()) {
@@ -524,7 +505,7 @@
createCodeOwnerConfigKey(branchNameKey, changedFile.newPath().get());
codeOwnerConfig =
codeOwnerBackend
- .getCodeOwnerConfig(codeOwnerConfigKey, revWalk, revCommit)
+ .getCodeOwnerConfig(codeOwnerConfigKey, revCommit)
// We already know that the path exists, so either the code owner config is
// successfully loaded (this case) or the loading fails with an exception because the
// code owner config is not parseable (catch block below), but it cannot happen that
@@ -553,7 +534,7 @@
new CommitValidationMessage(
invalidCodeOwnerConfigException.get().getMessage(),
getValidationMessageTypeForParsingError(
- codeOwnerBackend, branchNameKey, changedFile, revWalk, revCommit)));
+ codeOwnerBackend, branchNameKey, changedFile, revCommit)));
}
// The code owner config was successfully loaded and parsed.
@@ -564,15 +545,14 @@
Optional<CodeOwnerConfig> baseCodeOwnerConfig;
try {
baseCodeOwnerConfig =
- getBaseCodeOwnerConfig(codeOwnerBackend, branchNameKey, changedFile, revWalk, revCommit);
+ getBaseCodeOwnerConfig(codeOwnerBackend, branchNameKey, changedFile, revCommit);
} catch (CodeOwnersInternalServerErrorException codeOwnersInternalServerErrorException) {
if (getInvalidCodeOwnerConfigCause(codeOwnersInternalServerErrorException).isPresent()) {
// The base code owner config is non-parseable. Since the update makes the code owner
// config parseable, it is a good update even if the code owner config still contains
// issues. Hence in this case we downgrade all validation errors in the new version to
// warnings so that the update is not blocked.
- return validateCodeOwnerConfig(
- branchNameKey, revWalk, user, codeOwnerBackend, codeOwnerConfig)
+ return validateCodeOwnerConfig(branchNameKey, user, codeOwnerBackend, codeOwnerConfig)
.map(CodeOwnerConfigValidator::downgradeErrorToWarning);
}
@@ -583,14 +563,9 @@
// Validate the parsed code owner config.
if (baseCodeOwnerConfig.isPresent()) {
return validateCodeOwnerConfig(
- branchNameKey,
- revWalk,
- user,
- codeOwnerBackend,
- codeOwnerConfig,
- baseCodeOwnerConfig.get());
+ branchNameKey, user, codeOwnerBackend, codeOwnerConfig, baseCodeOwnerConfig.get());
}
- return validateCodeOwnerConfig(branchNameKey, revWalk, user, codeOwnerBackend, codeOwnerConfig);
+ return validateCodeOwnerConfig(branchNameKey, user, codeOwnerBackend, codeOwnerConfig);
}
/**
@@ -621,7 +596,6 @@
* @param branchNameKey the project and branch of the base code owner config
* @param changedFile the changed file of the code owner config that contains the path of the base
* code owner config as old path
- * @param revWalk rev walk that should be used to load the base code owner config
* @param revCommit the commit of the code owner config for which the base code owner config
* should be loaded
* @return the loaded base code owner config, {@link Optional#empty()} if no base code owner
@@ -631,15 +605,13 @@
CodeOwnerBackend codeOwnerBackend,
BranchNameKey branchNameKey,
ChangedFile changedFile,
- RevWalk revWalk,
RevCommit revCommit) {
if (changedFile.oldPath().isPresent()) {
Optional<ObjectId> parentRevision = getParentRevision(branchNameKey.project(), revCommit);
if (parentRevision.isPresent()) {
CodeOwnerConfig.Key baseCodeOwnerConfigKey =
createCodeOwnerConfigKey(branchNameKey, changedFile.oldPath().get());
- return codeOwnerBackend.getCodeOwnerConfig(
- baseCodeOwnerConfigKey, revWalk, parentRevision.get());
+ return codeOwnerBackend.getCodeOwnerConfig(baseCodeOwnerConfigKey, parentRevision.get());
}
}
return Optional.empty();
@@ -665,7 +637,6 @@
* @param codeOwnerBackend the code owner backend from which the code owner config can be loaded
* @param branchNameKey the project and branch of the code owner config
* @param changedFile the changed file that represents the code owner config
- * @param revWalk rev walk that should be used to load the code owner config
* @param revCommit the commit from which the code owner config should be loaded
* @return the {@link com.google.gerrit.server.git.validators.ValidationMessage.Type} (ERROR or
* WARNING) that should be used for parsing error of a code owner config file
@@ -674,7 +645,6 @@
CodeOwnerBackend codeOwnerBackend,
BranchNameKey branchNameKey,
ChangedFile changedFile,
- RevWalk revWalk,
RevCommit revCommit) {
//
if (changedFile.oldPath().isPresent()) {
@@ -693,7 +663,7 @@
// there.
CodeOwnerConfig.Key baseCodeOwnerConfigKey =
createCodeOwnerConfigKey(branchNameKey, changedFile.oldPath().get());
- codeOwnerBackend.getCodeOwnerConfig(baseCodeOwnerConfigKey, revWalk, parentRevision);
+ codeOwnerBackend.getCodeOwnerConfig(baseCodeOwnerConfigKey, parentRevision);
// The code owner config at the parent revision is parseable. This means the parsing error
// is introduced by the new commit and we should block uploading it, which we achieve by
// setting the validation message type to fatal.
@@ -757,7 +727,6 @@
* they are not newly introduced by the given code owner config).
*
* @param branchNameKey the branch and the project
- * @param revWalk rev walk that should be used to load the code owner configs
* @param user user for which the code owner visibility checks should be performed
* @param codeOwnerBackend the code owner backend from which the code owner configs were loaded
* @param codeOwnerConfig the code owner config that should be validated
@@ -767,7 +736,6 @@
*/
private Stream<CommitValidationMessage> validateCodeOwnerConfig(
BranchNameKey branchNameKey,
- RevWalk revWalk,
IdentifiedUser user,
CodeOwnerBackend codeOwnerBackend,
CodeOwnerConfig codeOwnerConfig,
@@ -776,9 +744,9 @@
requireNonNull(baseCodeOwnerConfig, "baseCodeOwnerConfig");
ImmutableSet<CommitValidationMessage> issuesInBaseVersion =
- validateCodeOwnerConfig(branchNameKey, revWalk, user, codeOwnerBackend, baseCodeOwnerConfig)
+ validateCodeOwnerConfig(branchNameKey, user, codeOwnerBackend, baseCodeOwnerConfig)
.collect(toImmutableSet());
- return validateCodeOwnerConfig(branchNameKey, revWalk, user, codeOwnerBackend, codeOwnerConfig)
+ return validateCodeOwnerConfig(branchNameKey, user, codeOwnerBackend, codeOwnerConfig)
.map(
commitValidationMessage ->
issuesInBaseVersion.contains(commitValidationMessage)
@@ -790,7 +758,6 @@
* Validates the given code owner config and returns validation issues as stream.
*
* @param branchNameKey the branch and the project
- * @param revWalk rev walk that should be used to load the code owner configs from {@code project}
* @param user user for which the code owner visibility checks should be performed
* @param codeOwnerBackend the code owner backend from which the code owner config was loaded
* @param codeOwnerConfig the code owner config that should be validated
@@ -799,7 +766,6 @@
*/
public Stream<CommitValidationMessage> validateCodeOwnerConfig(
BranchNameKey branchNameKey,
- RevWalk revWalk,
IdentifiedUser user,
CodeOwnerBackend codeOwnerBackend,
CodeOwnerConfig codeOwnerConfig) {
@@ -811,10 +777,7 @@
codeOwnerBackend.getFilePath(codeOwnerConfig.key()),
codeOwnerConfig),
validateImports(
- branchNameKey,
- revWalk,
- codeOwnerBackend.getFilePath(codeOwnerConfig.key()),
- codeOwnerConfig));
+ branchNameKey, codeOwnerBackend.getFilePath(codeOwnerConfig.key()), codeOwnerConfig));
}
/**
@@ -891,7 +854,6 @@
* Validates the imports of the given code owner config.
*
* @param branchNameKey the branch and the project
- * @param revWalk rev walk that should be used to load the code owner configs from {@code project}
* @param codeOwnerConfigFilePath the path of the code owner config file which contains the code
* owner config
* @param codeOwnerConfig the code owner config for which the imports should be validated
@@ -899,19 +861,19 @@
* if there are no issues
*/
private Stream<CommitValidationMessage> validateImports(
- BranchNameKey branchNameKey,
- RevWalk revWalk,
- Path codeOwnerConfigFilePath,
- CodeOwnerConfig codeOwnerConfig) {
+ BranchNameKey branchNameKey, Path codeOwnerConfigFilePath, CodeOwnerConfig codeOwnerConfig) {
try {
- RevCommit codeOwnerConfigRevision = revWalk.parseCommit(codeOwnerConfig.revision());
+ RevCommit codeOwnerConfigRevision;
+ try (Repository repo = repoManager.openRepository(branchNameKey.project());
+ RevWalk revWalk = new RevWalk(repo)) {
+ codeOwnerConfigRevision = revWalk.parseCommit(codeOwnerConfig.revision());
+ }
return Streams.concat(
codeOwnerConfig.imports().stream()
.map(
codeOwnerConfigReference ->
validateCodeOwnerConfigReference(
branchNameKey,
- revWalk,
codeOwnerConfigFilePath,
codeOwnerConfig.key(),
codeOwnerConfigRevision,
@@ -923,7 +885,6 @@
codeOwnerConfigReference ->
validateCodeOwnerConfigReference(
branchNameKey,
- revWalk,
codeOwnerConfigFilePath,
codeOwnerConfig.key(),
codeOwnerConfigRevision,
@@ -941,7 +902,6 @@
* Validates a code owner config reference.
*
* @param branchNameKey the branch and the project
- * @param revWalk rev walk that should be used to load the code owner configs from {@code project}
* @param codeOwnerConfigFilePath the path of the code owner config file which contains the code
* owner config reference
* @param keyOfImportingCodeOwnerConfig key of the importing code owner config
@@ -954,7 +914,6 @@
*/
private Optional<CommitValidationMessage> validateCodeOwnerConfigReference(
BranchNameKey branchNameKey,
- RevWalk revWalk,
Path codeOwnerConfigFilePath,
CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig,
RevCommit codeOwnerConfigRevision,
@@ -1040,8 +999,7 @@
// from it could fail with MissingObjectException.
Optional<CodeOwnerConfig> importedCodeOwnerConfig =
keyOfImportedCodeOwnerConfig.project().equals(branchNameKey.project())
- ? codeOwnerBackend.getCodeOwnerConfig(
- keyOfImportedCodeOwnerConfig, revWalk, revision.get())
+ ? codeOwnerBackend.getCodeOwnerConfig(keyOfImportedCodeOwnerConfig, revision.get())
: codeOwnerBackend.getCodeOwnerConfig(keyOfImportedCodeOwnerConfig, revision.get());
if (!importedCodeOwnerConfig.isPresent()) {
return nonResolvableImport(
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorErrorHandlingIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorErrorHandlingIT.java
index e1936b1..a1c34ba 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorErrorHandlingIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorErrorHandlingIT.java
@@ -19,6 +19,7 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.registration.DynamicMap;
@@ -35,7 +36,6 @@
import java.nio.file.Path;
import java.util.Optional;
import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.Before;
import org.junit.Test;
@@ -124,7 +124,7 @@
@Override
public Optional<CodeOwnerConfig> getCodeOwnerConfig(
- CodeOwnerConfig.Key codeOwnerConfigKey, RevWalk revWalk, ObjectId revision) {
+ CodeOwnerConfig.Key codeOwnerConfigKey, ObjectId revision) {
return Optional.empty();
}
@@ -142,7 +142,7 @@
}
@Override
- public Optional<PathExpressionMatcher> getPathExpressionMatcher() {
+ public Optional<PathExpressionMatcher> getPathExpressionMatcher(BranchNameKey branchNameKey) {
return Optional.empty();
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
index 4724629..432057b 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -2079,9 +2079,7 @@
.create();
// Create a change that merges the other branch into master. The code owner config files in the
- // created merge commit will be validated. This only works if CodeOwnerConfigValidator uses the
- // same RevWalk instance that inserted the new merge commit. If it doesn't, the create change
- // call below would fail with a MissingObjectException.
+ // created merge commit will be validated.
ChangeInput changeInput = new ChangeInput();
changeInput.project = project.get();
changeInput.branch = "master";
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
index 3b1258e..17ba5c4 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
@@ -20,6 +20,7 @@
import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
import static com.google.gerrit.plugins.codeowners.testing.RequiredApprovalSubject.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static com.google.gerrit.truth.OptionalSubject.assertThat;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
@@ -35,6 +36,7 @@
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackendId;
import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners;
+import com.google.gerrit.plugins.codeowners.backend.PathExpressions;
import com.google.gerrit.plugins.codeowners.backend.config.BackendConfig;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig;
@@ -242,6 +244,86 @@
}
@Test
+ public void configurePathExpressionsForProject() throws Exception {
+ fetchRefsMetaConfig();
+
+ Config cfg = new Config();
+ cfg.setString(
+ CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ BackendConfig.KEY_PATH_EXPRESSIONS,
+ PathExpressions.GLOB.name());
+ setCodeOwnersConfig(cfg);
+
+ PushResult r = pushRefsMetaConfig();
+ assertThat(r.getRemoteUpdate(RefNames.REFS_CONFIG).getStatus()).isEqualTo(Status.OK);
+ assertThat(codeOwnersPluginConfiguration.getProjectConfig(project).getPathExpressions("master"))
+ .value()
+ .isEqualTo(PathExpressions.GLOB);
+ }
+
+ @Test
+ public void configurePathExpressionsForBranch() throws Exception {
+ fetchRefsMetaConfig();
+
+ Config cfg = new Config();
+ cfg.setString(
+ CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS,
+ "master",
+ BackendConfig.KEY_PATH_EXPRESSIONS,
+ PathExpressions.GLOB.name());
+ setCodeOwnersConfig(cfg);
+
+ PushResult r = pushRefsMetaConfig();
+ assertThat(r.getRemoteUpdate(RefNames.REFS_CONFIG).getStatus()).isEqualTo(Status.OK);
+ assertThat(codeOwnersPluginConfiguration.getProjectConfig(project).getPathExpressions("master"))
+ .value()
+ .isEqualTo(PathExpressions.GLOB);
+ }
+
+ @Test
+ public void cannotConfigureInvalidPathExpressionsForProject() throws Exception {
+ fetchRefsMetaConfig();
+
+ Config cfg = new Config();
+ cfg.setString(
+ CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ BackendConfig.KEY_PATH_EXPRESSIONS,
+ "INVALID");
+ setCodeOwnersConfig(cfg);
+
+ PushResult r = pushRefsMetaConfig();
+ assertThat(r.getRemoteUpdate(RefNames.REFS_CONFIG).getStatus())
+ .isEqualTo(Status.REJECTED_OTHER_REASON);
+ assertThat(r.getMessages())
+ .contains(
+ "Path expressions 'INVALID' that are configured in code-owners.config"
+ + " (parameter codeOwners.pathExpressions) not found.");
+ }
+
+ @Test
+ public void cannotConfigureInvalidPathExpressionsForBranch() throws Exception {
+ fetchRefsMetaConfig();
+
+ Config cfg = new Config();
+ cfg.setString(
+ CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS,
+ "master",
+ BackendConfig.KEY_PATH_EXPRESSIONS,
+ "INVALID");
+ setCodeOwnersConfig(cfg);
+
+ PushResult r = pushRefsMetaConfig();
+ assertThat(r.getRemoteUpdate(RefNames.REFS_CONFIG).getStatus())
+ .isEqualTo(Status.REJECTED_OTHER_REASON);
+ assertThat(r.getMessages())
+ .contains(
+ "Path expressions 'INVALID' that are configured in code-owners.config"
+ + " (parameter codeOwners.master.pathExpressions) not found.");
+ }
+
+ @Test
public void configureRequiredApproval() throws Exception {
fetchRefsMetaConfig();
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressionsTest.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressionsTest.java
index cd155ba..b3d15d6 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressionsTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressionsTest.java
@@ -19,6 +19,7 @@
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.registration.PrivateInternals_DynamicMapImpl;
@@ -35,7 +36,6 @@
import java.nio.file.Path;
import java.util.Optional;
import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.Before;
import org.junit.Test;
@@ -200,7 +200,7 @@
@Override
public Optional<CodeOwnerConfig> getCodeOwnerConfig(
- CodeOwnerConfig.Key codeOwnerConfigKey, RevWalk revWalk, ObjectId revision) {
+ CodeOwnerConfig.Key codeOwnerConfigKey, ObjectId revision) {
throw new UnsupportedOperationException();
}
@@ -218,7 +218,7 @@
}
@Override
- public Optional<PathExpressionMatcher> getPathExpressionMatcher() {
+ public Optional<PathExpressionMatcher> getPathExpressionMatcher(BranchNameKey branchNameKey) {
return Optional.ofNullable(pathExpressionMatcher);
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackendTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackendTest.java
index 1bbce45..7ec9295 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackendTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackendTest.java
@@ -21,6 +21,7 @@
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.testing.backend.TestCodeOwnerConfigStorage;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
@@ -487,4 +488,13 @@
.isEqualTo(
Paths.get(codeOwnerConfigKey.folderPath() + codeOwnerConfigKey.fileName().get()));
}
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.pathExpressions", value = "GLOB")
+ public void getConfiguredPathExpressionMatcher() throws Exception {
+ com.google.gerrit.truth.OptionalSubject.assertThat(
+ codeOwnerBackend.getPathExpressionMatcher(BranchNameKey.create(project, "master")))
+ .value()
+ .isInstanceOf(GlobMatcher.class);
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/BUILD b/javatests/com/google/gerrit/plugins/codeowners/backend/BUILD
index f21c5c1..5db2a85 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/BUILD
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/BUILD
@@ -36,6 +36,7 @@
"//java/com/google/gerrit/exceptions",
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/testing:gerrit-test-util",
+ "//java/com/google/gerrit/truth",
"//lib:guava",
"//lib:jgit",
"//lib:jgit-junit",
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
index 4534df2..fc8f176 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
@@ -68,24 +68,26 @@
public void cannotComputeForNullRevisionResource() throws Exception {
NullPointerException npe =
assertThrows(
- NullPointerException.class, () -> changedFiles.compute(/* revisionResource= */ null));
+ NullPointerException.class,
+ () -> changedFiles.getOrCompute(/* revisionResource= */ null));
assertThat(npe).hasMessageThat().isEqualTo("revisionResource");
}
@Test
- public void cannotComputeForNullProject() throws Exception {
+ public void cannotGetOrComputeForNullProject() throws Exception {
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> changedFiles.compute(/* project= */ null, ObjectId.zeroId()));
+ () -> changedFiles.getOrCompute(/* project= */ null, ObjectId.zeroId()));
assertThat(npe).hasMessageThat().isEqualTo("project");
}
@Test
- public void cannotComputeForNullRevision() throws Exception {
+ public void cannotGetOrComputeForNullRevision() throws Exception {
NullPointerException npe =
assertThrows(
- NullPointerException.class, () -> changedFiles.compute(project, /* revision= */ null));
+ NullPointerException.class,
+ () -> changedFiles.getOrCompute(project, /* revision= */ null));
assertThat(npe).hasMessageThat().isEqualTo("revision");
}
@@ -96,7 +98,7 @@
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.compute(getRevisionResource(changeId));
+ changedFiles.getOrCompute(getRevisionResource(changeId));
assertThat(changedFilesSet).hasSize(1);
ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
assertThat(changedFile).hasNewPath().value().isEqualTo(Paths.get(path));
@@ -115,7 +117,7 @@
.getChangeId();
ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.compute(getRevisionResource(changeId));
+ changedFiles.getOrCompute(getRevisionResource(changeId));
assertThat(changedFilesSet).hasSize(1);
ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
assertThat(changedFile).hasNewPath().value().isEqualTo(Paths.get(path));
@@ -130,7 +132,7 @@
String changeId = createChangeWithFileDeletion(path);
ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.compute(getRevisionResource(changeId));
+ changedFiles.getOrCompute(getRevisionResource(changeId));
assertThat(changedFilesSet).hasSize(1);
ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
assertThat(changedFile).hasNewPath().isEmpty();
@@ -151,7 +153,7 @@
// ChangedFiles uses a DiffFormatter without rename detection (because rename detection requires
// loading the file contents which is too expensive).
ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.compute(getRevisionResource(changeId));
+ changedFiles.getOrCompute(getRevisionResource(changeId));
assertThat(changedFilesSet).hasSize(2);
ChangedFileSubject changedFile1 = assertThatCollection(changedFilesSet).element(0);
changedFile1.hasNewPath().value().isEqualTo(Paths.get(newPath));
@@ -174,7 +176,7 @@
String changeId = r.getChangeId();
ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.compute(getRevisionResource(changeId));
+ changedFiles.getOrCompute(getRevisionResource(changeId));
assertThat(changedFilesSet).hasSize(1);
ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
assertThat(changedFile).hasNewPath().value().isEqualTo(Paths.get(path));
@@ -254,7 +256,7 @@
.create();
ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.compute(getRevisionResource(Integer.toString(mergeChange.get())));
+ changedFiles.getOrCompute(getRevisionResource(Integer.toString(mergeChange.get())));
if (MergeCommitStrategy.ALL_CHANGED_FILES.equals(mergeCommitStrategy)) {
assertThat(changedFilesSet).comparingElementsUsing(hasPath()).containsExactly(file1, file2);
@@ -327,7 +329,7 @@
.create();
ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.compute(getRevisionResource(Integer.toString(mergeChange.get())));
+ changedFiles.getOrCompute(getRevisionResource(Integer.toString(mergeChange.get())));
ImmutableSet<String> oldPaths =
changedFilesSet.stream()
.map(changedFile -> JgitPath.of(changedFile.oldPath().get()).get())
@@ -359,7 +361,7 @@
.getChangeId();
ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.compute(getRevisionResource(changeId));
+ changedFiles.getOrCompute(getRevisionResource(changeId));
assertThat(changedFilesSet)
.comparingElementsUsing(hasPath())
.containsExactly(file4, file3, file5, file1, file2)
@@ -470,7 +472,7 @@
.create();
ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.compute(getRevisionResource(Integer.toString(mergeChange.get())));
+ changedFiles.getOrCompute(getRevisionResource(Integer.toString(mergeChange.get())));
if (MergeCommitStrategy.ALL_CHANGED_FILES.equals(mergeCommitStrategy)) {
assertThat(changedFilesSet)
@@ -491,7 +493,9 @@
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> changedFiles.getFromDiffCache(/* project= */ null, ObjectId.zeroId()));
+ () ->
+ changedFiles.getFromDiffCache(
+ /* project= */ null, ObjectId.zeroId(), MergeCommitStrategy.ALL_CHANGED_FILES));
assertThat(npe).hasMessageThat().isEqualTo("project");
}
@@ -500,17 +504,31 @@
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> changedFiles.getFromDiffCache(project, /* revision= */ null));
+ () ->
+ changedFiles.getFromDiffCache(
+ project, /* revision= */ null, MergeCommitStrategy.ALL_CHANGED_FILES));
assertThat(npe).hasMessageThat().isEqualTo("revision");
}
@Test
+ public void cannotGetFromDiffCacheForNullMergeCommitStrategy() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () ->
+ changedFiles.getFromDiffCache(
+ project, ObjectId.zeroId(), /* mergeCommitStrategy= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("mergeCommitStrategy");
+ }
+
+ @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);
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.getFromDiffCache(project, commit, MergeCommitStrategy.ALL_CHANGED_FILES);
assertThat(changedFilesSet).hasSize(1);
ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
assertThat(changedFile).hasNewPath().value().isEqualTo(Paths.get(path));
@@ -528,7 +546,8 @@
createChange("Change Modifying A File", JgitPath.of(path).get(), "new file content")
.getCommit();
- ImmutableList<ChangedFile> changedFilesSet = changedFiles.getFromDiffCache(project, commit);
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.getFromDiffCache(project, commit, MergeCommitStrategy.ALL_CHANGED_FILES);
assertThat(changedFilesSet).hasSize(1);
ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
assertThat(changedFile).hasNewPath().value().isEqualTo(Paths.get(path));
@@ -544,7 +563,9 @@
ImmutableList<ChangedFile> changedFilesSet =
changedFiles.getFromDiffCache(
- project, getRevisionResource(changeId).getPatchSet().commitId());
+ project,
+ getRevisionResource(changeId).getPatchSet().commitId(),
+ MergeCommitStrategy.ALL_CHANGED_FILES);
assertThat(changedFilesSet).hasSize(1);
ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
assertThat(changedFile).hasNewPath().isEmpty();
@@ -563,7 +584,9 @@
ImmutableList<ChangedFile> changedFilesSet =
changedFiles.getFromDiffCache(
- project, getRevisionResource(changeId).getPatchSet().commitId());
+ project,
+ getRevisionResource(changeId).getPatchSet().commitId(),
+ MergeCommitStrategy.ALL_CHANGED_FILES);
ChangedFileSubject changedFile = assertThatCollection(changedFilesSet).onlyElement();
changedFile.hasNewPath().value().isEqualTo(Paths.get(newPath));
changedFile.hasOldPath().value().isEqualTo(Paths.get(oldPath));
@@ -579,10 +602,14 @@
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");
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.getFromDiffCache(project, commit, MergeCommitStrategy.ALL_CHANGED_FILES);
+ 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
@@ -659,7 +686,8 @@
ImmutableList<ChangedFile> changedFilesSet =
changedFiles.getFromDiffCache(
project,
- getRevisionResource(Integer.toString(mergeChange.get())).getPatchSet().commitId());
+ getRevisionResource(Integer.toString(mergeChange.get())).getPatchSet().commitId(),
+ mergeCommitStrategy);
if (MergeCommitStrategy.ALL_CHANGED_FILES.equals(mergeCommitStrategy)) {
assertThat(changedFilesSet).comparingElementsUsing(hasPath()).containsExactly(file1, file2);
@@ -675,7 +703,8 @@
public void
getFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution_allChangedFiles()
throws Exception {
- testGetFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution();
+ testGetFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution(
+ MergeCommitStrategy.ALL_CHANGED_FILES);
}
@Test
@@ -685,11 +714,12 @@
public void
getFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution_filesWithConflictResolution()
throws Exception {
- testGetFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution();
+ testGetFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution(
+ MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION);
}
- private void testGetFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution()
- throws Exception {
+ private void testGetFromFileDiffCacheForMergeChangeThatContainsADeletedFileAsConflictResolution(
+ MergeCommitStrategy mergeCommitStrategy) throws Exception {
setAsRootCodeOwners(admin);
String file = "foo/a.txt";
@@ -735,7 +765,8 @@
ImmutableList<ChangedFile> changedFilesSet =
changedFiles.getFromDiffCache(
project,
- getRevisionResource(Integer.toString(mergeChange.get())).getPatchSet().commitId());
+ getRevisionResource(Integer.toString(mergeChange.get())).getPatchSet().commitId(),
+ mergeCommitStrategy);
ImmutableSet<String> oldPaths =
changedFilesSet.stream()
.map(changedFile -> JgitPath.of(changedFile.oldPath().get()).get())
@@ -766,7 +797,8 @@
"file content"))
.getCommit();
- ImmutableList<ChangedFile> changedFilesSet = changedFiles.getFromDiffCache(project, commit);
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.getFromDiffCache(project, commit, MergeCommitStrategy.ALL_CHANGED_FILES);
assertThat(changedFilesSet)
.comparingElementsUsing(hasPath())
.containsExactly(file4, file3, file5, file1, file2)
@@ -880,7 +912,8 @@
ImmutableList<ChangedFile> changedFilesSet =
changedFiles.getFromDiffCache(
project,
- getRevisionResource(Integer.toString(mergeChange.get())).getPatchSet().commitId());
+ getRevisionResource(Integer.toString(mergeChange.get())).getPatchSet().commitId(),
+ mergeCommitStrategy);
if (MergeCommitStrategy.ALL_CHANGED_FILES.equals(mergeCommitStrategy)) {
assertThat(changedFilesSet)
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerBackendIdTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerBackendIdTest.java
index 3f5ee09..a5e56f2 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerBackendIdTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerBackendIdTest.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersBackend;
@@ -26,7 +27,6 @@
import java.nio.file.Path;
import java.util.Optional;
import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.Test;
/** Tests for {@link com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackendId}. */
@@ -50,9 +50,7 @@
private static class TestCodeOwnerBackend implements CodeOwnerBackend {
@Override
public Optional<CodeOwnerConfig> getCodeOwnerConfig(
- CodeOwnerConfig.Key codeOwnerConfigKey,
- @Nullable RevWalk revWalk,
- @Nullable ObjectId revision) {
+ CodeOwnerConfig.Key codeOwnerConfigKey, @Nullable ObjectId revision) {
throw new UnsupportedOperationException("not implemented");
}
@@ -75,7 +73,7 @@
}
@Override
- public Optional<PathExpressionMatcher> getPathExpressionMatcher() {
+ public Optional<PathExpressionMatcher> getPathExpressionMatcher(BranchNameKey branchNameKey) {
return Optional.empty();
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
index 92f7f5b..5deb1d6 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
@@ -49,7 +49,6 @@
import java.util.Optional;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
-import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.Before;
import org.junit.Test;
@@ -2226,9 +2225,7 @@
@Override
public Optional<CodeOwnerConfig> getCodeOwnerConfig(
- CodeOwnerConfig.Key codeOwnerConfigKey,
- @Nullable RevWalk revWalk,
- @Nullable ObjectId revision) {
+ CodeOwnerConfig.Key codeOwnerConfigKey, @Nullable ObjectId revision) {
throw new UnsupportedOperationException("not implemented");
}
@@ -2241,7 +2238,7 @@
}
@Override
- public Optional<PathExpressionMatcher> getPathExpressionMatcher() {
+ public Optional<PathExpressionMatcher> getPathExpressionMatcher(BranchNameKey branchNameKey) {
return Optional.ofNullable(pathExpressionMatcher);
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/config/BackendConfigTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/config/BackendConfigTest.java
index 10ef51c..dcc9cb3 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/BackendConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/BackendConfigTest.java
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.plugins.codeowners.backend.config.BackendConfig.KEY_BACKEND;
+import static com.google.gerrit.plugins.codeowners.backend.config.BackendConfig.KEY_PATH_EXPRESSIONS;
import static com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static com.google.gerrit.truth.OptionalSubject.assertThat;
@@ -26,6 +27,7 @@
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackendId;
+import com.google.gerrit.plugins.codeowners.backend.PathExpressions;
import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersBackend;
import com.google.gerrit.plugins.codeowners.backend.proto.ProtoBackend;
import com.google.gerrit.server.git.validators.CommitValidationMessage;
@@ -186,6 +188,106 @@
}
@Test
+ public void cannotGetPathExpressionsForBranchWithNullPluginConfig() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () ->
+ backendConfig.getPathExpressionsForBranch(
+ null, BranchNameKey.create(project, "master")));
+ assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
+ }
+
+ @Test
+ public void cannotGetPathExpressionsForBranchForNullBranch() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> backendConfig.getPathExpressionsForBranch(new Config(), null));
+ assertThat(npe).hasMessageThat().isEqualTo("branch");
+ }
+
+ @Test
+ public void getPathExpressionsForBranchWhenPathExpressionsAreNotSet() throws Exception {
+ assertThat(
+ backendConfig.getPathExpressionsForBranch(
+ new Config(), BranchNameKey.create(project, "master")))
+ .isEmpty();
+ }
+
+ @Test
+ public void getPathExpressionsForBranch() throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_CODE_OWNERS,
+ "refs/heads/master",
+ KEY_PATH_EXPRESSIONS,
+ PathExpressions.GLOB.name());
+ assertThat(
+ backendConfig.getPathExpressionsForBranch(cfg, BranchNameKey.create(project, "master")))
+ .value()
+ .isEqualTo(PathExpressions.GLOB);
+ }
+
+ @Test
+ public void getPathExpressionsForBranchShortName() throws Exception {
+ Config cfg = new Config();
+ cfg.setString(SECTION_CODE_OWNERS, "master", KEY_PATH_EXPRESSIONS, PathExpressions.GLOB.name());
+ assertThat(
+ backendConfig.getPathExpressionsForBranch(cfg, BranchNameKey.create(project, "master")))
+ .value()
+ .isEqualTo(PathExpressions.GLOB);
+ }
+
+ @Test
+ public void getPathExpressionsForBranchIfConfigIsInvalid() throws Exception {
+ Config cfg = new Config();
+ cfg.setString(SECTION_CODE_OWNERS, "master", KEY_PATH_EXPRESSIONS, "INVALID");
+ assertThat(
+ backendConfig.getPathExpressionsForBranch(cfg, BranchNameKey.create(project, "master")))
+ .isEmpty();
+ }
+
+ @Test
+ public void cannotGetPathExpressionsForProjectWithNullPluginConfig() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> backendConfig.getPathExpressionsForProject(null, project));
+ assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
+ }
+
+ @Test
+ public void cannotGetPathExpressionsForProjectForNullProject() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> backendConfig.getPathExpressionsForProject(new Config(), null));
+ assertThat(npe).hasMessageThat().isEqualTo("project");
+ }
+
+ @Test
+ public void getPathExpressionsForProjectWhenBackendIsNotSet() throws Exception {
+ assertThat(backendConfig.getPathExpressionsForProject(new Config(), project)).isEmpty();
+ }
+
+ @Test
+ public void getPathExpressionsForProject() throws Exception {
+ Config cfg = new Config();
+ cfg.setString(SECTION_CODE_OWNERS, null, KEY_PATH_EXPRESSIONS, PathExpressions.GLOB.name());
+ assertThat(backendConfig.getPathExpressionsForProject(cfg, project))
+ .value()
+ .isEqualTo(PathExpressions.GLOB);
+ }
+
+ @Test
+ public void getPathExpressionsForProjectIfConfigIsInvalid() throws Exception {
+ Config cfg = new Config();
+ cfg.setString(SECTION_CODE_OWNERS, null, KEY_PATH_EXPRESSIONS, "INVALID");
+ assertThat(backendConfig.getPathExpressionsForProject(cfg, project)).isEmpty();
+ }
+
+ @Test
public void cannotValidateProjectLevelConfigWithNullFileName() throws Exception {
NullPointerException npe =
assertThrows(
@@ -204,6 +306,23 @@
}
@Test
+ public void getDefaultPathExpressions() throws Exception {
+ assertThat(backendConfig.getDefaultPathExpressions()).isEmpty();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.pathExpressions", value = "GLOB")
+ public void getConfiguredDefaultPathExpressions() throws Exception {
+ assertThat(backendConfig.getDefaultPathExpressions()).value().isEqualTo(PathExpressions.GLOB);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.pathExpressions", value = "INVALID")
+ public void getDefaultPathExpressionsIfConfigIsInvalid() throws Exception {
+ assertThat(backendConfig.getDefaultPathExpressions()).isEmpty();
+ }
+
+ @Test
public void validateEmptyProjectLevelConfig() throws Exception {
ImmutableList<CommitValidationMessage> commitValidationMessage =
backendConfig.validateProjectLevelConfig("code-owners.config", new Config());
@@ -215,13 +334,15 @@
Config cfg = new Config();
cfg.setString(
SECTION_CODE_OWNERS, null, KEY_BACKEND, CodeOwnerBackendId.FIND_OWNERS.getBackendId());
+ cfg.setString(SECTION_CODE_OWNERS, null, KEY_PATH_EXPRESSIONS, PathExpressions.GLOB.name());
ImmutableList<CommitValidationMessage> commitValidationMessage =
backendConfig.validateProjectLevelConfig("code-owners.config", cfg);
assertThat(commitValidationMessage).isEmpty();
}
@Test
- public void validateInvalidProjectLevelConfig_invalidProjectConfiguration() throws Exception {
+ public void validateInvalidProjectLevelConfig_invalidProjectLevelBackendConfiguration()
+ throws Exception {
Config cfg = new Config();
cfg.setString(SECTION_CODE_OWNERS, null, KEY_BACKEND, "INVALID");
ImmutableList<CommitValidationMessage> commitValidationMessages =
@@ -237,7 +358,25 @@
}
@Test
- public void validateInvalidProjectLevelConfig_invalidBranchConfiguration() throws Exception {
+ public void validateInvalidProjectLevelConfig_invalidProjectLevelPathExpressionsConfiguration()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(SECTION_CODE_OWNERS, null, KEY_PATH_EXPRESSIONS, "INVALID");
+ ImmutableList<CommitValidationMessage> commitValidationMessages =
+ backendConfig.validateProjectLevelConfig("code-owners.config", cfg);
+ assertThat(commitValidationMessages).hasSize(1);
+ CommitValidationMessage commitValidationMessage =
+ Iterables.getOnlyElement(commitValidationMessages);
+ assertThat(commitValidationMessage.getType()).isEqualTo(ValidationMessage.Type.ERROR);
+ assertThat(commitValidationMessage.getMessage())
+ .isEqualTo(
+ "Path expressions 'INVALID' that are configured in code-owners.config (parameter"
+ + " codeOwners.pathExpressions) not found.");
+ }
+
+ @Test
+ public void validateInvalidProjectLevelConfig_invalidBranchLevelBackendConfiguration()
+ throws Exception {
Config cfg = new Config();
cfg.setString(SECTION_CODE_OWNERS, "someBranch", KEY_BACKEND, "INVALID");
ImmutableList<CommitValidationMessage> commitValidationMessages =
@@ -251,4 +390,21 @@
"Code owner backend 'INVALID' that is configured in code-owners.config (parameter"
+ " codeOwners.someBranch.backend) not found.");
}
+
+ @Test
+ public void validateInvalidProjectLevelConfig_invalidBranchLevelPathExpressionsConfiguration()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(SECTION_CODE_OWNERS, "someBranch", KEY_PATH_EXPRESSIONS, "INVALID");
+ ImmutableList<CommitValidationMessage> commitValidationMessages =
+ backendConfig.validateProjectLevelConfig("code-owners.config", cfg);
+ assertThat(commitValidationMessages).hasSize(1);
+ CommitValidationMessage commitValidationMessage =
+ Iterables.getOnlyElement(commitValidationMessages);
+ assertThat(commitValidationMessage.getType()).isEqualTo(ValidationMessage.Type.ERROR);
+ assertThat(commitValidationMessage.getMessage())
+ .isEqualTo(
+ "Path expressions 'INVALID' that are configured in code-owners.config (parameter"
+ + " codeOwners.someBranch.pathExpressions) not found.");
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidatorTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidatorTest.java
index e05323c..d712ba4 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidatorTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidatorTest.java
@@ -24,7 +24,9 @@
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.git.validators.CommitValidationException;
import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Before;
import org.junit.Test;
@@ -52,29 +54,33 @@
/* subsection= */ null,
GeneralConfig.KEY_FALLBACK_CODE_OWNERS,
FallbackCodeOwners.ALL_USERS);
- RevCommit commit =
- testRepo
- .commit()
- .add("code-owners.config", cfg.toText())
- .add("project.config", "INVALID")
- .create();
- CommitReceivedEvent receiveEvent = new CommitReceivedEvent();
- receiveEvent.project =
- projectCache.get(project).orElseThrow(illegalState(project)).getProject();
- receiveEvent.refName = RefNames.REFS_CONFIG;
- receiveEvent.commit = commit;
- receiveEvent.revWalk = testRepo.getRevWalk();
- receiveEvent.repoConfig = new Config();
- CommitValidationException exception =
- assertThrows(
- CommitValidationException.class,
- () -> codeOwnersPluginConfigValidator.onCommitReceived(receiveEvent));
- assertThat(exception)
- .hasMessageThat()
- .isEqualTo(
- String.format(
- "failed to validate file code-owners.config for revision %s in ref %s of project %s",
- commit.getName(), RefNames.REFS_CONFIG, project));
- assertThat(exception).hasCauseThat().isInstanceOf(ConfigInvalidException.class);
+ try (TestRepository<Repository> testRepo =
+ new TestRepository<>(repoManager.openRepository(project))) {
+ RevCommit commit =
+ testRepo
+ .commit()
+ .add("code-owners.config", cfg.toText())
+ .add("project.config", "INVALID")
+ .create();
+
+ CommitReceivedEvent receiveEvent = new CommitReceivedEvent();
+ receiveEvent.project =
+ projectCache.get(project).orElseThrow(illegalState(project)).getProject();
+ receiveEvent.refName = RefNames.REFS_CONFIG;
+ receiveEvent.commit = commit;
+ receiveEvent.revWalk = testRepo.getRevWalk();
+ receiveEvent.repoConfig = new Config();
+ CommitValidationException exception =
+ assertThrows(
+ CommitValidationException.class,
+ () -> codeOwnersPluginConfigValidator.onCommitReceived(receiveEvent));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "failed to validate file code-owners.config for revision %s in ref %s of project %s",
+ commit.getName(), RefNames.REFS_CONFIG, project));
+ assertThat(exception).hasCauseThat().isInstanceOf(ConfigInvalidException.class);
+ }
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshotTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshotTest.java
index f155572..1dbe420 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshotTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshotTest.java
@@ -29,6 +29,7 @@
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.entities.RefNames;
@@ -42,6 +43,7 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigUpdate;
import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners;
import com.google.gerrit.plugins.codeowners.backend.PathExpressionMatcher;
+import com.google.gerrit.plugins.codeowners.backend.PathExpressions;
import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersBackend;
import com.google.gerrit.plugins.codeowners.common.CodeOwnerConfigValidationPolicy;
import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy;
@@ -52,7 +54,6 @@
import java.nio.file.Path;
import java.util.Optional;
import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.Before;
import org.junit.Test;
@@ -1032,6 +1033,154 @@
}
@Test
+ public void cannotGetPathExpressionsForNullBranch() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> cfgSnapshot().getPathExpressions(/* branchName= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("branchName");
+ }
+
+ @Test
+ public void getPathExpressionsForNonExistingBranch() throws Exception {
+ assertThat(cfgSnapshot().getPathExpressions("non-existing")).isEmpty();
+ }
+
+ @Test
+ public void getPathExpressionsWhenNoPathExpressionsAreConfigured() throws Exception {
+ assertThat(cfgSnapshot().getPathExpressions("master")).isEmpty();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.pathExpressions", value = "GLOB")
+ public void getConfiguredPathExpressions() throws Exception {
+ assertThat(cfgSnapshot().getPathExpressions("master")).value().isEqualTo(PathExpressions.GLOB);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.pathExpressions",
+ value = "non-existing-path-expressions")
+ public void getPathExpressionsIfNonExistingPathExpressionsAreConfigured() throws Exception {
+ assertThat(cfgSnapshot().getPathExpressions("master")).isEmpty();
+ }
+
+ @Test
+ public void getPathExpressionsConfiguredOnProjectLevel() throws Exception {
+ configurePathExpressions(project, PathExpressions.GLOB.name());
+ assertThat(cfgSnapshot().getPathExpressions("master")).value().isEqualTo(PathExpressions.GLOB);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.backend", value = "GLOB")
+ public void pathExpressionsConfiguredOnProjectLevelOverrideDefaultPathExpressions()
+ throws Exception {
+ configurePathExpressions(project, PathExpressions.SIMPLE.name());
+ assertThat(cfgSnapshot().getPathExpressions("master"))
+ .value()
+ .isEqualTo(PathExpressions.SIMPLE);
+ }
+
+ @Test
+ public void pathExpressionsAreInheritedFromParentProject() throws Exception {
+ configurePathExpressions(allProjects, PathExpressions.GLOB.name());
+ assertThat(cfgSnapshot().getPathExpressions("master")).value().isEqualTo(PathExpressions.GLOB);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.pathExpressions", value = "GLOB")
+ public void inheritedPathExpressionsOverrideDefaultPathExpressions() throws Exception {
+ configurePathExpressions(allProjects, PathExpressions.SIMPLE.name());
+ assertThat(cfgSnapshot().getPathExpressions("master"))
+ .value()
+ .isEqualTo(PathExpressions.SIMPLE);
+ }
+
+ @Test
+ public void projectLevelPathExpressionsOverrideInheritedPathExpressions() throws Exception {
+ configurePathExpressions(allProjects, PathExpressions.GLOB.name());
+ configurePathExpressions(project, PathExpressions.SIMPLE.name());
+ assertThat(cfgSnapshot().getPathExpressions("master"))
+ .value()
+ .isEqualTo(PathExpressions.SIMPLE);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.pathExpressions", value = "GLOB")
+ public void
+ pathExpressionsAreReadFromGlobalConfigIfNonExistingPathExpressionsAreConfiguredOnProjectLevel()
+ throws Exception {
+ configurePathExpressions(project, "non-existing-path-expressions");
+ assertThat(cfgSnapshot().getPathExpressions("master")).value().isEqualTo(PathExpressions.GLOB);
+ }
+
+ @Test
+ public void projectLevelPathExpressionsForOtherProjectHasNoEffect() throws Exception {
+ Project.NameKey otherProject = projectOperations.newProject().create();
+ configurePathExpressions(otherProject, PathExpressions.GLOB.name());
+ assertThat(cfgSnapshot().getPathExpressions("master")).isEmpty();
+ }
+
+ @Test
+ public void getPathExpressionsConfiguredOnBranchLevel() throws Exception {
+ configurePathExpressions(project, "refs/heads/master", PathExpressions.GLOB.name());
+ assertThat(cfgSnapshot().getPathExpressions("master")).value().isEqualTo(PathExpressions.GLOB);
+ }
+
+ @Test
+ public void getPathExpressionsConfiguredOnBranchLevelShortName() throws Exception {
+ configurePathExpressions(project, "master", PathExpressions.GLOB.name());
+ assertThat(cfgSnapshot().getPathExpressions("master")).value().isEqualTo(PathExpressions.GLOB);
+ }
+
+ @Test
+ public void
+ branchLevelPathExpressionsOnFullNameTakePrecedenceOverBranchLevelPathExpressionsOnShortName()
+ throws Exception {
+ configurePathExpressions(project, "master", PathExpressions.GLOB.name());
+ configurePathExpressions(project, "refs/heads/master", PathExpressions.SIMPLE.name());
+ assertThat(cfgSnapshot().getPathExpressions("master"))
+ .value()
+ .isEqualTo(PathExpressions.SIMPLE);
+ }
+
+ @Test
+ public void branchLevelPathExpressionsOverridesProjectLevelPathExpressions() throws Exception {
+ configurePathExpressions(project, PathExpressions.GLOB.name());
+ configurePathExpressions(project, "master", PathExpressions.SIMPLE.name());
+ assertThat(cfgSnapshot().getPathExpressions("master"))
+ .value()
+ .isEqualTo(PathExpressions.SIMPLE);
+ }
+
+ @Test
+ public void
+ pathExpressionsAreReadFromProjectIfNonExistingPathExpressionsAreConfiguredOnBranchLevel()
+ throws Exception {
+ updateCodeOwnersConfig(
+ project,
+ codeOwnersConfig -> {
+ codeOwnersConfig.setString(
+ CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ BackendConfig.KEY_PATH_EXPRESSIONS,
+ PathExpressions.GLOB.name());
+ codeOwnersConfig.setString(
+ CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS,
+ "master",
+ BackendConfig.KEY_PATH_EXPRESSIONS,
+ "non-existing-path-expressions");
+ });
+ assertThat(cfgSnapshot().getPathExpressions("master")).value().isEqualTo(PathExpressions.GLOB);
+ }
+
+ @Test
+ public void branchLevelPathExpressionsForOtherBranchHaveNoEffect() throws Exception {
+ configurePathExpressions(project, "foo", PathExpressions.GLOB.name());
+ assertThat(cfgSnapshot().getPathExpressions("master")).isEmpty();
+ }
+
+ @Test
public void getDefaultRequiredApprovalWhenNoRequiredApprovalIsConfigured() throws Exception {
RequiredApproval requiredApproval = cfgSnapshot().getRequiredApproval();
assertThat(requiredApproval).hasLabelNameThat().isEqualTo(RequiredApprovalConfig.DEFAULT_LABEL);
@@ -1858,6 +2007,17 @@
setCodeOwnersConfig(project, branch, BackendConfig.KEY_BACKEND, backendName);
}
+ private void configurePathExpressions(Project.NameKey project, String pathExpressionsName)
+ throws Exception {
+ configurePathExpressions(project, /* branch= */ null, pathExpressionsName);
+ }
+
+ private void configurePathExpressions(
+ Project.NameKey project, @Nullable String branch, String pathExpressionsName)
+ throws Exception {
+ setCodeOwnersConfig(project, branch, BackendConfig.KEY_PATH_EXPRESSIONS, pathExpressionsName);
+ }
+
private void configureRequiredApproval(Project.NameKey project, String requiredApproval)
throws Exception {
setCodeOwnersConfig(
@@ -1980,9 +2140,7 @@
@Override
public Optional<CodeOwnerConfig> getCodeOwnerConfig(
- CodeOwnerConfig.Key codeOwnerConfigKey,
- @Nullable RevWalk revWalk,
- @Nullable ObjectId revision) {
+ CodeOwnerConfig.Key codeOwnerConfigKey, @Nullable ObjectId revision) {
throw new UnsupportedOperationException("not implemented");
}
@@ -2005,7 +2163,7 @@
}
@Override
- public Optional<PathExpressionMatcher> getPathExpressionMatcher() {
+ public Optional<PathExpressionMatcher> getPathExpressionMatcher(BranchNameKey branchNameKey) {
return Optional.empty();
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersBackendTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersBackendTest.java
index ff6f184..4b26b73 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersBackendTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersBackendTest.java
@@ -16,6 +16,7 @@
import static com.google.gerrit.truth.OptionalSubject.assertThat;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.plugins.codeowners.backend.AbstractFileBasedCodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.AbstractFileBasedCodeOwnerBackendTest;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigParser;
@@ -41,7 +42,7 @@
@Test
public void getPathExpressionMatcher() throws Exception {
- assertThat(codeOwnerBackend.getPathExpressionMatcher())
+ assertThat(codeOwnerBackend.getPathExpressionMatcher(BranchNameKey.create(project, "master")))
.value()
.isInstanceOf(FindOwnersGlobMatcher.class);
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/proto/ProtoBackendTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/proto/ProtoBackendTest.java
index d231cdf..01d7091 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/proto/ProtoBackendTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/proto/ProtoBackendTest.java
@@ -2,6 +2,7 @@
import static com.google.gerrit.truth.OptionalSubject.assertThat;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.plugins.codeowners.backend.AbstractFileBasedCodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.AbstractFileBasedCodeOwnerBackendTest;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigParser;
@@ -27,7 +28,7 @@
@Test
public void getPathExpressionMatcher() throws Exception {
- assertThat(codeOwnerBackend.getPathExpressionMatcher())
+ assertThat(codeOwnerBackend.getPathExpressionMatcher(BranchNameKey.create(project, "master")))
.value()
.isInstanceOf(SimplePathExpressionMatcher.class);
}
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index 8ac1897..099645b 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -148,6 +148,25 @@
syntax so that the existing code owner config files can no longer be
parsed.
+<a id="pluginCodeOwnersPathExpressions">plugin.@PLUGIN@.pathExpressions</a>
+: The path expression syntax that is used in
+ [code owner config files](user-guide.html#codeOwnerConfigFiles).\
+ Can be overridden per project by setting
+ [codeOwners.pathExpressions](#codeOwnersPathExpressions) in
+ `@PLUGIN@.config`.\
+ The supported path expression syntaxes are listed and explained at the
+ [Path Expressions](path-expressions.html) page.\
+ By default unset which means that the path expression syntax is derived
+ from the configured [code owner backend](#pluginCodeOwnersBackend).\
+ \
+ **NOTE:** Be careful with changing this parameter as it affects how path
+ expressions in existing
+ [code owner config files](user-guide.html#codeOwnerConfigFiles) are
+ interpreted. E.g. by changing the path expression syntax existing path
+ expressions may now match different files, or existing path expressions
+ may no longer be valid and fail to parse, in which case they would not
+ match anything any more.
+
<a id="pluginCodeOwnersFileExtension">plugin.@PLUGIN@.fileExtension</a>
: The file extension that should be used for code owner config files.\
Allows to use a different code owner configuration in a fork. E.g. if
@@ -621,7 +640,7 @@
The supported code owner backends are listed at the
[Backends](backends.html) page.\
If not set, the global setting
- [plugin.@PLUGIN@.backend](#pluginCodeOwnersBackend) in `gerrit.config`\
+ [plugin.@PLUGIN@.backend](#pluginCodeOwnersBackend) in `gerrit.config`
is used.\
\
**NOTE:** Be careful with changing this parameter as it invalidates all
@@ -650,6 +669,51 @@
syntax so that the existing code owner config files can no longer be
parsed.
+<a id="codeOwnersPathExpressions">codeOwners.pathExpressions</a>
+: The path expression syntax that is used in`
+ [code owner config files](user-guide.html#codeOwnerConfigFiles).\
+ Overrides the global setting
+ [plugin.@PLUGIN@.pathExpressions](#pluginCodeOwnersPathExpressions) in
+ `gerrit.config` and the `codeOwners.pathExpressions` setting from parent
+ projects.\
+ Can be overridden per branch by setting
+ [codeOwners.\<branch\>.pathExpressions](#codeOwnersBranchPathExpressions).\
+ The supported path expression syntaxes are listed and explained at the
+ [Path Expressions](path-expressions.html) page.\
+ If not set, the global setting
+ [plugin.@PLUGIN@.pathExpressions](#pluginCodeOwnersPathExpressions) in
+ `gerrit.config` is used.\
+ \
+ **NOTE:** Be careful with changing this parameter as it affects how path
+ expressions in existing
+ [code owner config files](user-guide.html#codeOwnerConfigFiles) are
+ interpreted. E.g. by changing the path expression syntax existing path
+ expressions may now match different files, or existing path expressions
+ may no longer be valid and fail to parse, in which case they would not
+ match anything any more.
+
+<a id="codeOwnersBranchPathExpressions">codeOwners.\<branch\>.pathExpressions</a>
+: The path expression syntax that is used in
+ [code owner config files](user-guide.html#codeOwnerConfigFiles) that are
+ contained in this branch.\
+ The branch can be the short or full name. If both configurations exist
+ the one for the full name takes precedence.\
+ Overrides the per repository setting
+ [codeOwners.pathExpressions](#codeOwnersPathExpressions) and the
+ `codeOwners.<branch>.pathExpressions` setting from parent projects.\
+ The path expression syntax that is used in
+ [code owner config files](user-guide.html#codeOwnerConfigFiles).\
+ If not set, the project level configuration
+ [codeOwners.pathExpressions](#codeOwnersPathExpressions) is used.\
+ \
+ **NOTE:** Be careful with changing this parameter as it affects how path
+ expressions in existing
+ [code owner config files](user-guide.html#codeOwnerConfigFiles) are
+ interpreted. E.g. by changing the path expression syntax existing path
+ expressions may now match different files, or existing path expressions
+ may no longer be valid and fail to parse, in which case they would not
+ match anything any more.
+
<a id="codeOwnersFileExtension">codeOwners.fileExtension</a>
: The file extension that should be used for the code owner config files
in this project.\
diff --git a/resources/Documentation/path-expressions.md b/resources/Documentation/path-expressions.md
index a8fc387..ea306cf 100644
--- a/resources/Documentation/path-expressions.md
+++ b/resources/Documentation/path-expressions.md
@@ -5,15 +5,29 @@
[per-file](backend-find-owners.html#perFile) rule for the
[find-owners](backend-find-owners.html) backend).
-Which syntax is used depends on the used code owner backend:
+The following path expression syntaxes are supported:
+
+* `GLOB`:
+ Uses [globs](#globs) to match paths.
+* `FIND_OWNERS_GLOB`:
+ Uses [globs](#globs) to match paths, but each glob is automatically prefixed
+ with `{**/,}` so that subfolders are always matched, e.g. `*.md` matches all
+ md files in all subfolders, rather then only md files in the current folder
+ (also see the [caveat](#findOwnersCaveat) section below).
+* `SIMPLE`:
+ Uses [simple path expressions](#simplePathExpressions) to match paths.
+
+Which syntax is used by default depends on the used code owner backend:
* [find-owners](backend-find-owners.html) backend:
- uses [globs](#globs), but each glob is automatically prefixed with `{**/,}`
- so that subfolders are always matched, e.g. `*.md` matches all md files in all
- subfolders, rather then only md files in the current folder (also see the
- [caveat](#findOwnersCaveat) section below)
+ Uses `FIND_OWNERS_GLOB` as path expression syntax.
* [proto](backend-proto.html) backend:
- uses [simple path expressions](#simplePathExpressions)
+ Uses `SIMPLE` as path expression syntax.
+
+The default path expression syntax that is derived from the backend can be
+overriden by [global configuration](config.html#pluginCodeOwnersPathExpressions),
+on [project-level](config.html#codeOwnersPathExpressions) or on
+[branch-level](config.html#codeOwnersBranchPathExpressions).
## <a id="globs">Globs
