Remove sorting of owned paths to avoid that all owned paths are computed
Change Id33722dc0 added a limit to the stream that computes the owned
paths. However this limit has no real effect while the stream gets
sorted. This is because in order to sort all results have to be
available and hence be computed. Only afterwards the limit is applied.
This means the limit only reduces the number of returned owned path, but
doesn't prevent that all owned paths are computed.
To avoid the computation of all owned paths we remove the sorting on the
stream. Instead the sorting should happen before the stream computation
is done, which means we must sort the changed files which are the input
for this computation.
Fortunately these changed files are already sorted. So all we should do
is, document this fact in the ChangedFiles API and return an
ImmutableList instead of an ImmutableSet. Also we add tests to verify
that ChangedFiles returns sorted ChangeFile's.
The integration tests for owned paths already verify that they are
returned in a sorted order.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I0953333649a8c9ada3043e4169ac6abcf5eb3f9a
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/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();
