Merge changes I3b520220,I09533336
* changes:
Reduce default value for maxPathsInChangeMessages config parameter
Remove sorting of owned paths to avoid that all owned paths are computed
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
index c96af17..b691e21 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
@@ -15,10 +15,10 @@
package com.google.gerrit.plugins.codeowners.backend;
import static com.google.common.base.Preconditions.checkState;
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static java.util.Objects.requireNonNull;
-import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.Project;
@@ -84,12 +84,12 @@
* <p>The diff is computed against the parent commit.
*
* @param revisionResource the revision resource for which the changed files should be computed
- * @return the files that have been changed in the given revision
+ * @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
*/
- public ImmutableSet<ChangedFile> compute(RevisionResource revisionResource)
+ public ImmutableList<ChangedFile> compute(RevisionResource revisionResource)
throws IOException, PatchListNotAvailableException {
requireNonNull(revisionResource, "revisionResource");
return compute(revisionResource.getProject(), revisionResource.getPatchSet().commitId());
@@ -102,12 +102,12 @@
*
* @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
+ * @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
*/
- public ImmutableSet<ChangedFile> compute(Project.NameKey project, ObjectId revision)
+ public ImmutableList<ChangedFile> compute(Project.NameKey project, ObjectId revision)
throws IOException, PatchListNotAvailableException {
requireNonNull(project, "project");
requireNonNull(revision, "revision");
@@ -119,7 +119,7 @@
}
}
- public ImmutableSet<ChangedFile> compute(
+ public ImmutableList<ChangedFile> compute(
Project.NameKey project, Config repoConfig, RevWalk revWalk, RevCommit revCommit)
throws IOException, PatchListNotAvailableException {
return compute(
@@ -130,7 +130,7 @@
codeOwnersPluginConfiguration.getProjectConfig(project).getMergeCommitStrategy());
}
- public ImmutableSet<ChangedFile> compute(
+ public ImmutableList<ChangedFile> compute(
Project.NameKey project,
Config repoConfig,
RevWalk revWalk,
@@ -164,7 +164,7 @@
* @param mergeCommit the merge commit for which the changed files should be computed
* @return the changed files for the given merge commit
*/
- private ImmutableSet<ChangedFile> computeByComparingAgainstAutoMerge(
+ private ImmutableList<ChangedFile> computeByComparingAgainstAutoMerge(
Project.NameKey project, RevCommit mergeCommit) throws PatchListNotAvailableException {
checkState(
mergeCommit.getParentCount() > 1, "expected %s to be a merge commit", mergeCommit.name());
@@ -180,7 +180,7 @@
patchListEntry.getNewName() == null
|| !Patch.isMagic(patchListEntry.getNewName()))
.map(ChangedFile::create)
- .collect(toImmutableSet());
+ .collect(toImmutableList());
}
}
@@ -192,9 +192,9 @@
* @param repoConfig the repository configuration
* @param revWalk the rev walk
* @param revCommit the commit for which the changed files should be computed
- * @return the changed files for the given commit
+ * @return the changed files for the given commit, sorted alphabetically by path
*/
- private ImmutableSet<ChangedFile> computeByComparingAgainstFirstParent(
+ private ImmutableList<ChangedFile> computeByComparingAgainstFirstParent(
Config repoConfig, RevWalk revWalk, RevCommit revCommit) throws IOException {
try (Timer0.Context ctx = codeOwnerMetrics.computeChangedFilesAgainstFirstParent.start()) {
RevCommit baseCommit = revCommit.getParentCount() > 0 ? revCommit.getParent(0) : null;
@@ -209,8 +209,8 @@
diffFormatter.setReader(revWalk.getObjectReader(), repoConfig);
diffFormatter.setDiffComparator(RawTextComparator.DEFAULT);
List<DiffEntry> diffEntries = diffFormatter.scan(baseCommit, revCommit);
- ImmutableSet<ChangedFile> changedFiles =
- diffEntries.stream().map(ChangedFile::create).collect(toImmutableSet());
+ ImmutableList<ChangedFile> changedFiles =
+ diffEntries.stream().map(ChangedFile::create).collect(toImmutableList());
if (changedFiles.size() <= MAX_CHANGED_FILES_TO_LOG) {
logger.atFine().log("changed files = %s", changedFiles);
} else {
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 1c5e87f..5ab28b4 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -17,7 +17,6 @@
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
-import static java.util.Comparator.comparing;
import static java.util.Objects.requireNonNull;
import com.google.common.annotations.VisibleForTesting;
@@ -149,8 +148,7 @@
.map(Optional::get))
.filter(
pathCodeOwnerStatus -> pathCodeOwnerStatus.status() == CodeOwnerStatus.APPROVED)
- .map(PathCodeOwnerStatus::path)
- .sorted(comparing(Path::toString));
+ .map(PathCodeOwnerStatus::path);
if (limit > 0) {
ownedPaths = ownedPaths.limit(limit);
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
index ddeec46..251504a 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
@@ -69,11 +69,12 @@
public static final String KEY_EXEMPTED_USER = "exemptedUser";
public static final String KEY_ENABLE_IMPLICIT_APPROVALS = "enableImplicitApprovals";
public static final String KEY_OVERRIDE_INFO_URL = "overrideInfoUrl";
- public static final int DEFAULT_MAX_PATHS_IN_CHANGE_MESSAGES = 100;
public static final String KEY_REJECT_NON_RESOLVABLE_CODE_OWNERS =
"rejectNonResolvableCodeOwners";
public static final String KEY_REJECT_NON_RESOLVABLE_IMPORTS = "rejectNonResolvableImports";
+ public static final int DEFAULT_MAX_PATHS_IN_CHANGE_MESSAGES = 50;
+
private static final String KEY_ALLOWED_EMAIL_DOMAIN = "allowedEmailDomain";
private final String pluginName;
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/BUILD b/java/com/google/gerrit/plugins/codeowners/testing/BUILD
index cff6196..16af5f4 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/BUILD
+++ b/java/com/google/gerrit/plugins/codeowners/testing/BUILD
@@ -19,5 +19,6 @@
"//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/backend",
"//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/common",
"//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/restapi",
+ "//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/util",
],
)
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/ChangedFileSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/ChangedFileSubject.java
index 069122c..9484c97 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/ChangedFileSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/ChangedFileSubject.java
@@ -18,16 +18,34 @@
import static com.google.gerrit.truth.OptionalSubject.optionals;
import com.google.common.collect.ImmutableList;
+import com.google.common.truth.Correspondence;
import com.google.common.truth.FailureMetadata;
import com.google.common.truth.Subject;
import com.google.gerrit.plugins.codeowners.common.ChangedFile;
+import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.gerrit.truth.ListSubject;
+import com.google.gerrit.truth.NullAwareCorrespondence;
import com.google.gerrit.truth.OptionalSubject;
import java.util.Collection;
/** {@link Subject} for doing assertions on {@link ChangedFile}s. */
public class ChangedFileSubject extends Subject {
/**
+ * Constructs a {@link Correspondence} that maps {@link ChangedFile}s to their paths (new path if
+ * set, otherwise old path).
+ */
+ public static Correspondence<ChangedFile, String> hasPath() {
+ return NullAwareCorrespondence.transforming(
+ changedFile ->
+ JgitPath.of(
+ changedFile.newPath().isPresent()
+ ? changedFile.newPath().get()
+ : changedFile.oldPath().get())
+ .get(),
+ "has path");
+ }
+
+ /**
* Starts fluent chain to do assertions on a {@link ChangedFile}.
*
* @param changedFile the changed file on which assertions should be done
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
index 34e0cb2..2bbc4f4 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
@@ -18,9 +18,12 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.plugins.codeowners.testing.ChangedFileSubject.assertThat;
import static com.google.gerrit.plugins.codeowners.testing.ChangedFileSubject.assertThatCollection;
+import static com.google.gerrit.plugins.codeowners.testing.ChangedFileSubject.hasPath;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static org.junit.Assert.fail;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.PushOneCommit;
@@ -88,7 +91,8 @@
String changeId =
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
- ImmutableSet<ChangedFile> changedFilesSet = changedFiles.compute(getRevisionResource(changeId));
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.compute(getRevisionResource(changeId));
assertThat(changedFilesSet).hasSize(1);
ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
assertThat(changedFile).hasNewPath().value().isEqualTo(Paths.get(path));
@@ -106,7 +110,8 @@
createChange("Change Modifying A File", JgitPath.of(path).get(), "new file content")
.getChangeId();
- ImmutableSet<ChangedFile> changedFilesSet = changedFiles.compute(getRevisionResource(changeId));
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.compute(getRevisionResource(changeId));
assertThat(changedFilesSet).hasSize(1);
ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
assertThat(changedFile).hasNewPath().value().isEqualTo(Paths.get(path));
@@ -120,7 +125,8 @@
String path = "/foo/bar/baz.txt";
String changeId = createChangeWithFileDeletion(path);
- ImmutableSet<ChangedFile> changedFilesSet = changedFiles.compute(getRevisionResource(changeId));
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.compute(getRevisionResource(changeId));
assertThat(changedFilesSet).hasSize(1);
ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
assertThat(changedFile).hasNewPath().isEmpty();
@@ -140,7 +146,8 @@
// A renamed file is reported as addition of new path + deletion of old path. This is because
// ChangedFiles uses a DiffFormatter without rename detection (because rename detection requires
// loading the file contents which is too expensive).
- ImmutableSet<ChangedFile> changedFilesSet = changedFiles.compute(getRevisionResource(changeId));
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.compute(getRevisionResource(changeId));
assertThat(changedFilesSet).hasSize(2);
ChangedFileSubject changedFile1 = assertThatCollection(changedFilesSet).element(0);
changedFile1.hasNewPath().value().isEqualTo(Paths.get(newPath));
@@ -162,7 +169,8 @@
assertThat(r.getCommit().getParents()).isEmpty();
String changeId = r.getChangeId();
- ImmutableSet<ChangedFile> changedFilesSet = changedFiles.compute(getRevisionResource(changeId));
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.compute(getRevisionResource(changeId));
assertThat(changedFilesSet).hasSize(1);
ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
assertThat(changedFile).hasNewPath().value().isEqualTo(Paths.get(path));
@@ -241,17 +249,13 @@
.content("content")
.create();
- ImmutableSet<ChangedFile> changedFilesSet =
+ ImmutableList<ChangedFile> changedFilesSet =
changedFiles.compute(getRevisionResource(Integer.toString(mergeChange.get())));
- ImmutableSet<String> paths =
- changedFilesSet.stream()
- .map(changedFile -> JgitPath.of(changedFile.newPath().get()).get())
- .collect(toImmutableSet());
if (MergeCommitStrategy.ALL_CHANGED_FILES.equals(mergeCommitStrategy)) {
- assertThat(paths).containsExactly(file1, file2);
+ assertThat(changedFilesSet).comparingElementsUsing(hasPath()).containsExactly(file1, file2);
} else if (MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION.equals(mergeCommitStrategy)) {
- assertThat(paths).containsExactly(file1);
+ assertThat(changedFilesSet).comparingElementsUsing(hasPath()).containsExactly(file1);
} else {
fail("expected merge commit strategy: " + mergeCommitStrategy);
}
@@ -318,7 +322,7 @@
.delete()
.create();
- ImmutableSet<ChangedFile> changedFilesSet =
+ ImmutableList<ChangedFile> changedFilesSet =
changedFiles.compute(getRevisionResource(Integer.toString(mergeChange.get())));
ImmutableSet<String> oldPaths =
changedFilesSet.stream()
@@ -327,6 +331,153 @@
assertThat(oldPaths).containsExactly(file);
}
+ @Test
+ public void sortedByPath() throws Exception {
+ String file1 = "foo/bar.baz";
+ String file2 = "foo/baz.bar";
+ String file3 = "bar/foo.baz";
+ String file4 = "bar/baz.foo";
+ String file5 = "baz/foo.bar";
+ String changeId =
+ createChange(
+ "Test Change",
+ ImmutableMap.of(
+ file1,
+ "file content",
+ file2,
+ "file content",
+ file3,
+ "file content",
+ file4,
+ "file content",
+ file5,
+ "file content"))
+ .getChangeId();
+
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.compute(getRevisionResource(changeId));
+ assertThat(changedFilesSet)
+ .comparingElementsUsing(hasPath())
+ .containsExactly(file4, file3, file5, file1, file2)
+ .inOrder();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.mergeCommitStrategy", value = "ALL_CHANGED_FILES")
+ public void sortedByPath_mergeCommitAgainstFirstParent() throws Exception {
+ testSortedByPathForMerge(MergeCommitStrategy.ALL_CHANGED_FILES);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.mergeCommitStrategy",
+ value = "FILES_WITH_CONFLICT_RESOLUTION")
+ public void sortedByPath_mergeCommitAgainstAutoMerge() throws Exception {
+ testSortedByPathForMerge(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION);
+ }
+
+ private void testSortedByPathForMerge(MergeCommitStrategy mergeCommitStrategy) throws Exception {
+ setAsRootCodeOwners(admin);
+
+ String file1 = "foo/bar.baz";
+ String file2 = "foo/baz.bar";
+ String file3 = "bar/foo.baz";
+ String file4 = "bar/baz.foo";
+ String file5 = "baz/foo.bar";
+
+ // Create a base change.
+ Change.Id baseChange =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .file(file1)
+ .content("base content")
+ .file(file3)
+ .content("base content")
+ .file(file5)
+ .content("base content")
+ .create();
+ approveAndSubmit(baseChange);
+
+ // Create another branch
+ String branchName = "foo";
+ BranchInput branchInput = new BranchInput();
+ branchInput.ref = branchName;
+ branchInput.revision = projectOperations.project(project).getHead("master").name();
+ gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput);
+
+ // Create a change in master that touches file1, file3 and file5
+ Change.Id changeInMaster =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .file(file1)
+ .content("master content")
+ .file(file3)
+ .content("master content")
+ .file(file5)
+ .content("master content")
+ .create();
+ approveAndSubmit(changeInMaster);
+
+ // Create a change in the other branch and that touches file1, file3, file5 and creates file2,
+ // file4.
+ Change.Id changeInOtherBranch =
+ changeOperations
+ .newChange()
+ .branch(branchName)
+ .file(file1)
+ .content("other content")
+ .file(file2)
+ .content("content")
+ .file(file3)
+ .content("other content")
+ .file(file4)
+ .content("content")
+ .file(file5)
+ .content("other content")
+ .create();
+ approveAndSubmit(changeInOtherBranch);
+
+ // Create a merge change with a conflict resolution for file1 and file2 with the same content as
+ // in the other branch (no conflict on file2).
+ Change.Id mergeChange =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .mergeOfButBaseOnFirst()
+ .tipOfBranch("master")
+ .and()
+ .tipOfBranch(branchName)
+ .file(file1)
+ .content("merged content")
+ .file(file2)
+ .content("content")
+ .file(file3)
+ .content("merged content")
+ .file(file4)
+ .content("content")
+ .file(file5)
+ .content("merged content")
+ .create();
+
+ ImmutableList<ChangedFile> changedFilesSet =
+ changedFiles.compute(getRevisionResource(Integer.toString(mergeChange.get())));
+
+ if (MergeCommitStrategy.ALL_CHANGED_FILES.equals(mergeCommitStrategy)) {
+ assertThat(changedFilesSet)
+ .comparingElementsUsing(hasPath())
+ .containsExactly(file4, file3, file5, file1, file2)
+ .inOrder();
+ } else if (MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION.equals(mergeCommitStrategy)) {
+ assertThat(changedFilesSet)
+ .comparingElementsUsing(hasPath())
+ .containsExactly(file3, file5, file1);
+ } else {
+ fail("expected merge commit strategy: " + mergeCommitStrategy);
+ }
+ }
+
private void approveAndSubmit(Change.Id changeId) throws Exception {
approve(Integer.toString(changeId.get()));
gApi.changes().id(changeId.get()).current().submit();
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index 8207d33..ffa4018 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -407,7 +407,7 @@
Can be overridden per project by setting
[codeOwners.maxPathsInChangeMessages](#codeOwnersMaxPathsInChangeMessages)
in `@PLUGIN@.config`.\
- By default `100`.
+ By default `50`.
<a id="pluginCodeOwnersMaxCodeOwnerConfigCacheSize">plugin.@PLUGIN@.maxCodeOwnerConfigCacheSize</a>
: When computing code owner file statuses for a change (e.g. to compute
@@ -770,8 +770,7 @@
from parent projects.\
If not set, the global setting
[plugin.@PLUGIN@.maxPathsInChangeMessages](#pluginCodeOwnersMaxPathsInChangeMessages)
- in `gerrit.config` is used.\
- By default `100`.
+ in `gerrit.config` is used.
---
