Disable rename detection when computing changed files
To detect renames Git must compare file contents of deleted and added
files. Loading and comparing file contents is rather expensive and is
likely the reason why we are seeing a high latency for computing the
changed files for 1% of the changes.
By turning off the rename detection we expect that Git can do a pure
tree diff without needing to load file contents, which should be much
faster.
The rename detection in DiffFormatter is off by default, it gets turned
on only if either DiffFormatter#setDetectRenames(true) or
DiffFormatter#setPathFilter(TreeFilter.FOLLOW) is called. We rely on the
default since it's very unlikely that this default will ever change (and
if it does our rename tests will start failing).
Turning off the rename detection affects the Get Code Owner Status REST
endpoint. For renamed files it now returns 2 file code owner statuses,
one for the old path and one for the new path.
Example:
* old:
...
{
"change_type": "RENAMED",
"old_path_status" {
"path": "user-introduction.txt",
"status": "INSUFFICIENT_REVIEWERS"
},
"new_path_status" {
"path": "docs/user-intro.md",
"status": "APPROVED"
}
}
...
* new:
...
{
"change_type": "ADDED",
"new_path_status" {
"path": "docs/user-intro.md",
"status": "APPROVED"
}
},
{
"change_type": "DELETED",
"old_path_status" {
"path": "user-introduction.txt",
"status": "INSUFFICIENT_REVIEWERS"
}
}
...
This change in behaviour requires an update in the frontend, otherwise
the code owner status icon for renamed files in the file list could be
wrong, as it would only considers the status for the new path, but not
for the old path.
The tests with renamed paths have been adapted accordingly. Since paths
are always returned in alphabetical order, the tests know which path
(new or old path) is returned first in the response and hence the
assertions can rely on this order.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I9a1d6ba9d23b2ba2b5fe2c8afcc0ed0598400041
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
index 4fdab65..c96af17 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
@@ -200,10 +200,14 @@
RevCommit baseCommit = revCommit.getParentCount() > 0 ? revCommit.getParent(0) : null;
logger.atFine().log("baseCommit = %s", baseCommit != null ? baseCommit.name() : "n/a");
+ // Detecting renames is expensive (since it requires Git to load and compare file contents of
+ // added and deleted files) and can significantly increase the latency for changes that touch
+ // large files. To avoid this latency we do not enable the rename detection on the
+ // DiffFormater. As a result of this renamed files will be returned as 2 ChangedFile's, one
+ // for the deletion of the old path and one for the addition of the new path.
try (DiffFormatter diffFormatter = new DiffFormatter(DisabledOutputStream.INSTANCE)) {
diffFormatter.setReader(revWalk.getObjectReader(), repoConfig);
diffFormatter.setDiffComparator(RawTextComparator.DEFAULT);
- diffFormatter.setDetectRenames(true);
List<DiffEntry> diffEntries = diffFormatter.scan(baseCommit, revCommit);
ImmutableSet<ChangedFile> changedFiles =
diffEntries.stream().map(ChangedFile::create).collect(toImmutableSet());
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/ChangedFileSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/ChangedFileSubject.java
index 805eccd..069122c 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/ChangedFileSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/ChangedFileSubject.java
@@ -17,10 +17,13 @@
import static com.google.common.truth.Truth.assertAbout;
import static com.google.gerrit.truth.OptionalSubject.optionals;
+import com.google.common.collect.ImmutableList;
import com.google.common.truth.FailureMetadata;
import com.google.common.truth.Subject;
import com.google.gerrit.plugins.codeowners.common.ChangedFile;
+import com.google.gerrit.truth.ListSubject;
import com.google.gerrit.truth.OptionalSubject;
+import java.util.Collection;
/** {@link Subject} for doing assertions on {@link ChangedFile}s. */
public class ChangedFileSubject extends Subject {
@@ -34,6 +37,12 @@
return assertAbout(changedFiles()).that(changedFile);
}
+ /** Starts fluent chain to do assertions on a collection of {@link ChangedFile}s. */
+ public static ListSubject<ChangedFileSubject, ChangedFile> assertThatCollection(
+ Collection<ChangedFile> changedFiles) {
+ return ListSubject.assertThat(ImmutableList.copyOf(changedFiles), changedFiles());
+ }
+
/** Creates subject factory for mapping {@link ChangedFile}s to {@link ChangedFileSubject}s. */
public static Subject.Factory<ChangedFileSubject, ChangedFile> changedFiles() {
return ChangedFileSubject::new;
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/FileCodeOwnerStatusSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/FileCodeOwnerStatusSubject.java
index 03948f1..3a81908 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/FileCodeOwnerStatusSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/FileCodeOwnerStatusSubject.java
@@ -20,11 +20,13 @@
import static com.google.gerrit.plugins.codeowners.testing.PathCodeOwnerStatusSubject.pathCodeOwnerStatuses;
import static com.google.gerrit.truth.OptionalSubject.optionals;
+import com.google.common.collect.ImmutableList;
import com.google.common.truth.FailureMetadata;
import com.google.common.truth.Subject;
import com.google.gerrit.plugins.codeowners.backend.FileCodeOwnerStatus;
import com.google.gerrit.truth.ListSubject;
import com.google.gerrit.truth.OptionalSubject;
+import java.util.Collection;
import java.util.stream.Stream;
/** {@link Subject} for doing assertions on {@link FileCodeOwnerStatus}es. */
@@ -39,6 +41,13 @@
fileCodeOwnerStatuses.collect(toImmutableList()), fileCodeOwnerStatuses());
}
+ /** Starts fluent chain to do assertions on a collection of {@link FileCodeOwnerStatus}es. */
+ public static ListSubject<FileCodeOwnerStatusSubject, FileCodeOwnerStatus> assertThatCollection(
+ Collection<FileCodeOwnerStatus> fileCodeOwnerStatuses) {
+ return ListSubject.assertThat(
+ ImmutableList.copyOf(fileCodeOwnerStatuses), fileCodeOwnerStatuses());
+ }
+
private static Factory<FileCodeOwnerStatusSubject, FileCodeOwnerStatus> fileCodeOwnerStatuses() {
return FileCodeOwnerStatusSubject::new;
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
index 85a0f04..531f118 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
@@ -102,18 +102,22 @@
CodeOwnerStatusInfo codeOwnerStatus =
changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
- FileCodeOwnerStatusInfoSubject fileCodeOwnerStatusInfoSubject =
- assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().onlyElement();
- fileCodeOwnerStatusInfoSubject.hasChangeTypeThat().isEqualTo(ChangeType.RENAMED);
- fileCodeOwnerStatusInfoSubject.hasNewPathStatusThat().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusInfoSubject
- .hasNewPathStatusThat()
+ assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().hasSize(2);
+ FileCodeOwnerStatusInfoSubject fileCodeOwnerStatusInfoSubject1 =
+ assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().element(0);
+ fileCodeOwnerStatusInfoSubject1.hasChangeTypeThat().isEqualTo(ChangeType.DELETED);
+ fileCodeOwnerStatusInfoSubject1.hasOldPathStatusThat().value().hasPathThat().isEqualTo(oldPath);
+ fileCodeOwnerStatusInfoSubject1
+ .hasOldPathStatusThat()
.value()
.hasStatusThat()
.isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusInfoSubject.hasOldPathStatusThat().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusInfoSubject
- .hasOldPathStatusThat()
+ FileCodeOwnerStatusInfoSubject fileCodeOwnerStatusInfoSubject2 =
+ assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().element(1);
+ fileCodeOwnerStatusInfoSubject2.hasChangeTypeThat().isEqualTo(ChangeType.ADDED);
+ fileCodeOwnerStatusInfoSubject2.hasNewPathStatusThat().value().hasPathThat().isEqualTo(newPath);
+ fileCodeOwnerStatusInfoSubject2
+ .hasNewPathStatusThat()
.value()
.hasStatusThat()
.isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
@@ -122,38 +126,46 @@
gApi.changes().id(changeId).addReviewer(user.email());
codeOwnerStatus = changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
- fileCodeOwnerStatusInfoSubject =
- assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().onlyElement();
- fileCodeOwnerStatusInfoSubject.hasChangeTypeThat().isEqualTo(ChangeType.RENAMED);
- fileCodeOwnerStatusInfoSubject.hasNewPathStatusThat().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusInfoSubject
- .hasNewPathStatusThat()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusInfoSubject.hasOldPathStatusThat().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusInfoSubject
+ assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().hasSize(2);
+ fileCodeOwnerStatusInfoSubject1 =
+ assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().element(0);
+ fileCodeOwnerStatusInfoSubject1.hasChangeTypeThat().isEqualTo(ChangeType.DELETED);
+ fileCodeOwnerStatusInfoSubject1.hasOldPathStatusThat().value().hasPathThat().isEqualTo(oldPath);
+ fileCodeOwnerStatusInfoSubject1
.hasOldPathStatusThat()
.value()
.hasStatusThat()
.isEqualTo(CodeOwnerStatus.PENDING);
+ fileCodeOwnerStatusInfoSubject2 =
+ assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().element(1);
+ fileCodeOwnerStatusInfoSubject2.hasChangeTypeThat().isEqualTo(ChangeType.ADDED);
+ fileCodeOwnerStatusInfoSubject2.hasNewPathStatusThat().value().hasPathThat().isEqualTo(newPath);
+ fileCodeOwnerStatusInfoSubject2
+ .hasNewPathStatusThat()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
// Add a reviewer that is a code owner of the new path.
gApi.changes().id(changeId).addReviewer(user2.email());
codeOwnerStatus = changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
- fileCodeOwnerStatusInfoSubject =
- assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().onlyElement();
- fileCodeOwnerStatusInfoSubject.hasChangeTypeThat().isEqualTo(ChangeType.RENAMED);
- fileCodeOwnerStatusInfoSubject.hasNewPathStatusThat().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusInfoSubject
- .hasNewPathStatusThat()
+ assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().hasSize(2);
+ fileCodeOwnerStatusInfoSubject1 =
+ assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().element(0);
+ fileCodeOwnerStatusInfoSubject1.hasChangeTypeThat().isEqualTo(ChangeType.DELETED);
+ fileCodeOwnerStatusInfoSubject1.hasOldPathStatusThat().value().hasPathThat().isEqualTo(oldPath);
+ fileCodeOwnerStatusInfoSubject1
+ .hasOldPathStatusThat()
.value()
.hasStatusThat()
.isEqualTo(CodeOwnerStatus.PENDING);
- fileCodeOwnerStatusInfoSubject.hasOldPathStatusThat().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusInfoSubject
- .hasOldPathStatusThat()
+ fileCodeOwnerStatusInfoSubject2 =
+ assertThat(codeOwnerStatus).hasFileCodeOwnerStatusesThat().element(1);
+ fileCodeOwnerStatusInfoSubject2.hasChangeTypeThat().isEqualTo(ChangeType.ADDED);
+ fileCodeOwnerStatusInfoSubject2.hasNewPathStatusThat().value().hasPathThat().isEqualTo(newPath);
+ fileCodeOwnerStatusInfoSubject2
+ .hasNewPathStatusThat()
.value()
.hasStatusThat()
.isEqualTo(CodeOwnerStatus.PENDING);
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
index 3cf1aaf..34e0cb2 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
@@ -17,6 +17,7 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
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.testing.GerritJUnit.assertThrows;
import static org.junit.Assert.fail;
@@ -35,6 +36,7 @@
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.common.ChangedFile;
import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy;
+import com.google.gerrit.plugins.codeowners.testing.ChangedFileSubject;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.RevisionResource;
@@ -135,13 +137,21 @@
gApi.changes().id(changeId).current().files();
+ // A renamed file is reported as addition of new path + deletion of old path. This is because
+ // ChangedFiles uses a DiffFormatter without rename detection (because rename detection requires
+ // loading the file contents which is too expensive).
ImmutableSet<ChangedFile> changedFilesSet = changedFiles.compute(getRevisionResource(changeId));
- assertThat(changedFilesSet).hasSize(1);
- ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
- assertThat(changedFile).hasNewPath().value().isEqualTo(Paths.get(newPath));
- assertThat(changedFile).hasOldPath().value().isEqualTo(Paths.get(oldPath));
- assertThat(changedFile).isRename();
- assertThat(changedFile).isNoDeletion();
+ assertThat(changedFilesSet).hasSize(2);
+ ChangedFileSubject changedFile1 = assertThatCollection(changedFilesSet).element(0);
+ changedFile1.hasNewPath().value().isEqualTo(Paths.get(newPath));
+ changedFile1.hasOldPath().isEmpty();
+ changedFile1.isNoRename();
+ changedFile1.isNoDeletion();
+ ChangedFileSubject changedFile2 = assertThatCollection(changedFilesSet).element(1);
+ changedFile2.hasNewPath().isEmpty();
+ changedFile2.hasOldPath().value().isEqualTo(Paths.get(oldPath));
+ changedFile2.isNoRename();
+ changedFile2.isDeletion();
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index f2c55e4..d23f647 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -18,6 +18,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
import static com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject.assertThat;
+import static com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject.assertThatCollection;
import static com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject.assertThatStream;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
@@ -187,24 +188,29 @@
requestScopeOperations.setApiUser(user2.id());
recommend(changeId);
- Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatStream(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusSubject
+ ImmutableList<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)).collect(toImmutableList());
+ assertThatCollection(fileCodeOwnerStatuses).hasSize(2);
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject1 =
+ assertThatCollection(fileCodeOwnerStatuses).element(0);
+ fileCodeOwnerStatusSubject1.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
+ fileCodeOwnerStatusSubject1
.hasNewPathStatus()
.value()
.hasStatusThat()
.isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusSubject
+ fileCodeOwnerStatusSubject1.hasChangedFile().isNoRename();
+ fileCodeOwnerStatusSubject1.hasChangedFile().isNoDeletion();
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject2 =
+ assertThatCollection(fileCodeOwnerStatuses).element(1);
+ fileCodeOwnerStatusSubject2.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
+ fileCodeOwnerStatusSubject2
.hasOldPathStatus()
.value()
.hasStatusThat()
.isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasChangedFile().isRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ fileCodeOwnerStatusSubject2.hasChangedFile().isNoRename();
+ fileCodeOwnerStatusSubject2.hasChangedFile().isDeletion();
}
@Test
@@ -321,24 +327,29 @@
requestScopeOperations.setApiUser(user2.id());
recommend(changeId);
- Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatStream(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusSubject
+ ImmutableList<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)).collect(toImmutableList());
+ assertThatCollection(fileCodeOwnerStatuses).hasSize(2);
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject1 =
+ assertThatCollection(fileCodeOwnerStatuses).element(0);
+ fileCodeOwnerStatusSubject1.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
+ fileCodeOwnerStatusSubject1
.hasOldPathStatus()
.value()
.hasStatusThat()
.isEqualTo(CodeOwnerStatus.PENDING);
- fileCodeOwnerStatusSubject.hasChangedFile().isRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ fileCodeOwnerStatusSubject1.hasChangedFile().isNoRename();
+ fileCodeOwnerStatusSubject1.hasChangedFile().isDeletion();
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject2 =
+ assertThatCollection(fileCodeOwnerStatuses).element(1);
+ fileCodeOwnerStatusSubject2.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
+ fileCodeOwnerStatusSubject2
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ fileCodeOwnerStatusSubject2.hasChangedFile().isNoRename();
+ fileCodeOwnerStatusSubject2.hasChangedFile().isNoDeletion();
}
@Test
@@ -358,24 +369,29 @@
requestScopeOperations.setApiUser(user2.id());
recommend(changeId);
- Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatStream(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.PENDING);
- fileCodeOwnerStatusSubject.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusSubject
+ ImmutableList<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)).collect(toImmutableList());
+ assertThatCollection(fileCodeOwnerStatuses).hasSize(2);
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject1 =
+ assertThatCollection(fileCodeOwnerStatuses).element(0);
+ fileCodeOwnerStatusSubject1.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
+ fileCodeOwnerStatusSubject1
.hasOldPathStatus()
.value()
.hasStatusThat()
.isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasChangedFile().isRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ fileCodeOwnerStatusSubject1.hasChangedFile().isNoRename();
+ fileCodeOwnerStatusSubject1.hasChangedFile().isDeletion();
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject2 =
+ assertThatCollection(fileCodeOwnerStatuses).element(1);
+ fileCodeOwnerStatusSubject2.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
+ fileCodeOwnerStatusSubject2
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.PENDING);
+ fileCodeOwnerStatusSubject2.hasChangedFile().isNoRename();
+ fileCodeOwnerStatusSubject2.hasChangedFile().isNoDeletion();
}
@Test
@@ -473,24 +489,29 @@
requestScopeOperations.setApiUser(user.id());
recommend(changeId);
- Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatStream(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusSubject
+ ImmutableList<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)).collect(toImmutableList());
+ assertThatCollection(fileCodeOwnerStatuses).hasSize(2);
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject1 =
+ assertThatCollection(fileCodeOwnerStatuses).element(0);
+ fileCodeOwnerStatusSubject1.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
+ fileCodeOwnerStatusSubject1
.hasOldPathStatus()
.value()
.hasStatusThat()
.isEqualTo(CodeOwnerStatus.APPROVED);
- fileCodeOwnerStatusSubject.hasChangedFile().isRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ fileCodeOwnerStatusSubject1.hasChangedFile().isNoRename();
+ fileCodeOwnerStatusSubject1.hasChangedFile().isDeletion();
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject2 =
+ assertThatCollection(fileCodeOwnerStatuses).element(1);
+ fileCodeOwnerStatusSubject2.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
+ fileCodeOwnerStatusSubject2
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ fileCodeOwnerStatusSubject2.hasChangedFile().isNoRename();
+ fileCodeOwnerStatusSubject2.hasChangedFile().isNoDeletion();
}
@Test
@@ -506,24 +527,29 @@
requestScopeOperations.setApiUser(user.id());
recommend(changeId);
- Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatStream(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
- fileCodeOwnerStatusSubject.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusSubject
+ ImmutableList<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)).collect(toImmutableList());
+ assertThatCollection(fileCodeOwnerStatuses).hasSize(2);
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject1 =
+ assertThatCollection(fileCodeOwnerStatuses).element(0);
+ fileCodeOwnerStatusSubject1.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
+ fileCodeOwnerStatusSubject1
.hasOldPathStatus()
.value()
.hasStatusThat()
.isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasChangedFile().isRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ fileCodeOwnerStatusSubject1.hasChangedFile().isNoRename();
+ fileCodeOwnerStatusSubject1.hasChangedFile().isDeletion();
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject2 =
+ assertThatCollection(fileCodeOwnerStatuses).element(1);
+ fileCodeOwnerStatusSubject2.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
+ fileCodeOwnerStatusSubject2
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ fileCodeOwnerStatusSubject2.hasChangedFile().isNoRename();
+ fileCodeOwnerStatusSubject2.hasChangedFile().isNoDeletion();
}
@Test
@@ -672,18 +698,13 @@
String changeId = createChangeWithFileRename(oldPath, newPath);
amendChange(user, changeId);
- Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatStream(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusSubject
- .hasNewPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusSubject
+ ImmutableList<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)).collect(toImmutableList());
+ assertThatCollection(fileCodeOwnerStatuses).hasSize(2);
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject1 =
+ assertThatCollection(fileCodeOwnerStatuses).element(0);
+ fileCodeOwnerStatusSubject1.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
+ fileCodeOwnerStatusSubject1
.hasOldPathStatus()
.value()
.hasStatusThat()
@@ -691,8 +712,18 @@
implicitApprovalsEnabled
? CodeOwnerStatus.APPROVED
: CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasChangedFile().isRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ fileCodeOwnerStatusSubject1.hasChangedFile().isNoRename();
+ fileCodeOwnerStatusSubject1.hasChangedFile().isDeletion();
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject2 =
+ assertThatCollection(fileCodeOwnerStatuses).element(1);
+ fileCodeOwnerStatusSubject2.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
+ fileCodeOwnerStatusSubject2
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ fileCodeOwnerStatusSubject2.hasChangedFile().isNoRename();
+ fileCodeOwnerStatusSubject2.hasChangedFile().isNoDeletion();
}
@Test
@@ -719,12 +750,23 @@
String changeId = createChangeWithFileRename(oldPath, newPath);
amendChange(user, changeId);
- Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
- FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
- assertThatStream(fileCodeOwnerStatuses).onlyElement();
- fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
- fileCodeOwnerStatusSubject
+ ImmutableList<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)).collect(toImmutableList());
+ assertThatCollection(fileCodeOwnerStatuses).hasSize(2);
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject1 =
+ assertThatCollection(fileCodeOwnerStatuses).element(0);
+ fileCodeOwnerStatusSubject1.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
+ fileCodeOwnerStatusSubject1
+ .hasOldPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ fileCodeOwnerStatusSubject1.hasChangedFile().isNoRename();
+ fileCodeOwnerStatusSubject1.hasChangedFile().isDeletion();
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject2 =
+ assertThatCollection(fileCodeOwnerStatuses).element(1);
+ fileCodeOwnerStatusSubject2.hasNewPathStatus().value().hasPathThat().isEqualTo(newPath);
+ fileCodeOwnerStatusSubject2
.hasNewPathStatus()
.value()
.hasStatusThat()
@@ -732,14 +774,8 @@
implicitApprovalsEnabled
? CodeOwnerStatus.APPROVED
: CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasOldPathStatus().value().hasPathThat().isEqualTo(oldPath);
- fileCodeOwnerStatusSubject
- .hasOldPathStatus()
- .value()
- .hasStatusThat()
- .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
- fileCodeOwnerStatusSubject.hasChangedFile().isRename();
- fileCodeOwnerStatusSubject.hasChangedFile().isNoDeletion();
+ fileCodeOwnerStatusSubject2.hasChangedFile().isNoRename();
+ fileCodeOwnerStatusSubject2.hasChangedFile().isNoDeletion();
}
@Test
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index c0e608b..29d5fd8 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -615,17 +615,6 @@
"path": "docs/todo.txt",
"status": "PENDING"
}
- },
- {
- "change_type": "RENAMED",
- "old_path_status" {
- "path": "user-introduction.txt",
- "status": "INSUFFICIENT_REVIEWERS"
- },
- "new_path_status" {
- "path": "docs/user-intro.md",
- "status": "APPROVED"
- }
}
]
}
@@ -949,7 +938,7 @@
| Field Name | | Description |
| ------------- | -------- | ----------- |
-| `change_type` | optional | The type of the file modification. Can be `ADDED`, `MODIFIED`, `DELETED`, `RENAMED` or `COPIED`. Not set if `MODIFIED`.
+| `change_type` | optional | The type of the file modification. Can be `ADDED`, `MODIFIED`, `DELETED`, `RENAMED` or `COPIED`. Not set if `MODIFIED`. Renamed files might appear as separate addition and deletion or with type=RENAMED. Copied files might appear as addition or with type=COPIED.
| `old_path_status` | optional | The code owner status for the old path as [PathCodeOwnerStatusInfo](#path-code-owner-status-info) entity. Only set if `change_type` is `DELETED` or `RENAMED`.
| `new_path_status` | optional | The code owner status for the new path as [PathCodeOwnerStatusInfo](#path-code-owner-status-info) entity. Not set if `change_type` is `DELETED`.
