Disable rename detection for the code owners submit rule
The code owners submit rule only checks if all paths (new and old) have
been code owner approved (see CodeOwnerApprovalCheck#isSubmittable). For
this it's OK if for renamed files 2 FileCodeOwnerStatus'es are returned
(one for the new path and one for the old path), hence rename detection
can be disabled here.
When code owner statuses are returned from the GetCodeOwnerStatus REST
endpoint however we must detect renames (see
CodeOwnerApprovalCheck#getFileStatusesAsSet) because our API
documentation for this REST endpoint says that we return a single code
owner status for renamed files. This is important for callers that do
pagination, e.g. if the change screen shows the first 200 files (which
may include renames) than getting the first 200 code owner statuses
should match these files (their new and old paths).
Changing CodeOwnerApprovalCheck#getAllPathsAsApproved to always disable
the rename detection didn't make any tests fail although it affects the
GetCodeOwnerStatus REST endpoint. This was because getAllPathsAsApproved
is only called from the GetCodeOwnerStatus REST endpoint if a change
contains a renamed file and all files of the change are exempted from
requiring code owner approvals because a) the uploader is exempted from
requiring code owner approvals or b) the change is a pure revert and
pure reverts are configured to not require code owner approvals. The new
tests in GetCodeOwnerStatusIT cover this now.
CodeOwnerApprovalCheck has one more caller of ChangedFiles for which we
haven't checked yet whether rename detection can be disabled. At this
place we add a TODO so that we remember to check this later.
Bug: Google b/461456634
Change-Id: I9fc7ca3a808a176324fc9eb2f1d49b4bb4b55cae
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
index 31491fb..30d51aa 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
@@ -47,8 +47,10 @@
/**
* Class to get the files that have been changed in a revision.
*
- * <p>The {@link #get(Project.NameKey, ObjectId, MergeCommitStrategy)} method is retrieving the file
- * diff from the diff cache and has rename detection enabled.
+ * <p>If possible changed files should be retrieved without rename detection, since rename detection
+ * may be expensive.
+ *
+ * <p>The changed files are retrieved from {@link DiffOperations}.
*
* <p>The {@link com.google.gerrit.server.patch.PatchListCache} is deprecated, and hence it not
* being used here.
@@ -121,7 +123,7 @@
* @param revisionResource the revision resource for which the changed files should be retrieved
* @return the files that have been changed in the given revision, sorted alphabetically by path
* @throws IOException thrown if the computation fails due to an I/O error
- * @see #get(Project.NameKey, ObjectId, MergeCommitStrategy)
+ * @see #get(Project.NameKey, ObjectId, MergeCommitStrategy, boolean)
*/
public ImmutableList<ChangedFile> getWithoutRenameDetection(RevisionResource revisionResource)
throws IOException, DiffNotAvailableException {
@@ -133,43 +135,37 @@
/**
* Gets the changed files.
*
- * <p>Rename detection is enabled.
- *
* <p>Uses the configured merge commit strategy.
*
* @param project the project
* @param revision the revision for which the changed files should be retrieved
+ * @param enableRenameDetection whether the rename detection is enabled
* @return the files that have been changed in the given revision, sorted alphabetically by path
* @throws IOException thrown if the computation fails due to an I/O error
*/
- public ImmutableList<ChangedFile> get(Project.NameKey project, ObjectId revision)
+ public ImmutableList<ChangedFile> get(
+ Project.NameKey project, ObjectId revision, boolean enableRenameDetection)
throws IOException, DiffNotAvailableException {
requireNonNull(project, "project");
requireNonNull(revision, "revision");
return get(
project,
revision,
- codeOwnersPluginConfiguration.getProjectConfig(project).getMergeCommitStrategy());
+ codeOwnersPluginConfiguration.getProjectConfig(project).getMergeCommitStrategy(),
+ enableRenameDetection);
}
/**
* Gets the changed files.
*
- * <p>Rename detection is enabled.
- *
* @param project the project
* @param revision the revision for which the changed files should be retrieved
* @param mergeCommitStrategy the merge commit strategy that should be used to compute the changed
* files for merge commits
+ * @param enableRenameDetection whether the rename detection is enabled
* @return the files that have been changed in the given revision, sorted alphabetically by path
*/
public ImmutableList<ChangedFile> get(
- Project.NameKey project, ObjectId revision, MergeCommitStrategy mergeCommitStrategy)
- throws IOException, DiffNotAvailableException {
- return get(project, revision, mergeCommitStrategy, /* enableRenameDetection= */ true);
- }
-
- private ImmutableList<ChangedFile> get(
Project.NameKey project,
ObjectId revision,
MergeCommitStrategy mergeCommitStrategy,
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 4e30b1a..deb0507 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -221,7 +221,14 @@
try {
boolean isSubmittable =
!getFileStatuses(
- codeOwnersConfig, codeOwnerConfigHierarchy, codeOwnerResolver, changeNotes)
+ codeOwnersConfig,
+ codeOwnerConfigHierarchy,
+ codeOwnerResolver,
+ changeNotes,
+ // We only need to know if all new and old paths are approved. For this it's OK if
+ // for renamed files 2 FileCodeOwnerStatus'es are returned (one for the new path
+ // and one for the old path), hence rename detection can be disabled here.
+ /* enabledRenameDetection= */ false)
.anyMatch(
fileStatus ->
(fileStatus.newPathStatus().isPresent()
@@ -255,7 +262,7 @@
* @param start number of file statuses to skip
* @param limit the max number of file statuses that should be returned (0 = unlimited)
* @see #getFileStatuses(CodeOwnersPluginProjectConfigSnapshot, CodeOwnerConfigHierarchy,
- * CodeOwnerResolver, ChangeNotes)
+ * CodeOwnerResolver, ChangeNotes, boolean)
*/
public ImmutableSet<FileCodeOwnerStatus> getFileStatusesAsSet(
ChangeNotes changeNotes, int start, int limit) throws IOException, DiffNotAvailableException {
@@ -272,7 +279,12 @@
codeOwnersConfig,
codeOwnerConfigHierarchyProvider.get(),
codeOwnerResolverProvider.get().enforceVisibility(false),
- changeNotes);
+ changeNotes,
+ // The FileCodeOwnerStatus'es that are computed here are returned in the response of
+ // GetCodeOwnerStatus REST endpoint. Since the GetCodeOwnerStatus REST endpoint is
+ // documented to return a single FileCodeOwnerStatus for renamed files we need to get
+ // the changed files with rename detection enabled.
+ /* enableRenameDetection= */ true);
if (start > 0) {
fileStatuses = fileStatuses.skip(start);
}
@@ -315,7 +327,8 @@
CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig,
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
- ChangeNotes changeNotes)
+ ChangeNotes changeNotes,
+ boolean enableRenameDetection)
throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
try (Timer0.Context ctx = codeOwnerMetrics.prepareFileStatusComputation.start()) {
@@ -335,7 +348,8 @@
changeNotes.getCurrentPatchSet(),
String.format(
"patch set uploader %s is exempted from requiring code owner approvals",
- AccountTemplateUtil.getAccountTemplate(patchSetUploader)));
+ AccountTemplateUtil.getAccountTemplate(patchSetUploader)),
+ enableRenameDetection);
}
boolean arePureRevertsExempted = codeOwnersConfig.arePureRevertsExempted();
@@ -346,7 +360,8 @@
return getAllPathsAsApproved(
changeNotes,
changeNotes.getCurrentPatchSet(),
- "change is a pure revert and is exempted from requiring code owner approvals");
+ "change is a pure revert and is exempted from requiring code owner approvals",
+ enableRenameDetection);
}
BranchNameKey branch = changeNotes.getChange().getDest();
@@ -363,7 +378,10 @@
ChangedFilesByPatchSetCache changedFilesByPatchSetCache =
changedFilesByPatchSetCacheFactory.create(codeOwnersConfig, changeNotes);
return changedFiles
- .get(changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId())
+ .get(
+ changeNotes.getProjectName(),
+ changeNotes.getCurrentPatchSet().commitId(),
+ enableRenameDetection)
.stream()
.map(
changedFile ->
@@ -437,7 +455,10 @@
codeOwnersConfig, codeOwnerResolver, changeNotes, accountIds);
ChangedFilesByPatchSetCache changedFilesByPatchSetCache =
changedFilesByPatchSetCacheFactory.create(codeOwnersConfig, changeNotes);
- return changedFiles.get(changeNotes.getProjectName(), patchSet.commitId()).stream()
+ // TODO: check if rename detection can be disabled here
+ return changedFiles
+ .get(changeNotes.getProjectName(), patchSet.commitId(), /* enableRenameDetection= */ true)
+ .stream()
.map(
changedFile ->
getFileStatus(
@@ -466,10 +487,12 @@
}
private Stream<FileCodeOwnerStatus> getAllPathsAsApproved(
- ChangeNotes changeNotes, PatchSet patchSet, String reason)
+ ChangeNotes changeNotes, PatchSet patchSet, String reason, boolean enableRenameDetection)
throws IOException, DiffNotAvailableException {
logger.atFine().log("all paths are approved (reason = %s)", reason);
- return changedFiles.get(changeNotes.getProjectName(), patchSet.commitId()).stream()
+ return changedFiles
+ .get(changeNotes.getProjectName(), patchSet.commitId(), enableRenameDetection)
+ .stream()
.map(
changedFile ->
FileCodeOwnerStatus.create(
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 059ab95..c8aef14 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
@@ -22,8 +22,12 @@
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
import com.google.gerrit.acceptance.testsuite.change.TestChange;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.extensions.api.changes.ChangeIdentifier;
+import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
import com.google.gerrit.plugins.codeowners.api.CodeOwnerStatusInfo;
@@ -45,6 +49,7 @@
*/
public class GetCodeOwnerStatusIT extends AbstractCodeOwnersIT {
@Inject private RequestScopeOperations requestScopeOperations;
+ @Inject private ChangeOperations changeOperations;
@Test
public void getStatus() throws Exception {
@@ -547,6 +552,125 @@
}
@Test
+ @GerritConfig(name = "plugin.code-owners.exemptedUser", value = "exempted-user@example.com")
+ public void getStatusForRenamedFile_exemptedAccount() throws Exception {
+ TestAccount exemptedUser =
+ accountCreator.create(
+ "exemptedUser", "exempted-user@example.com", "Exempted User", /* displayName= */ null);
+
+ String oldPath = "foo/bar/abc.txt";
+ String newPath = "foo/baz/abc.txt";
+
+ TestChange baseChange =
+ changeOperations
+ .newChange()
+ .project(project)
+ .commitMessage("Change Adding A File")
+ .file(JgitPath.of(oldPath).get())
+ .content("file content")
+ .createAndGet();
+
+ TestChange changeWithRename =
+ changeOperations
+ .newChange()
+ .owner(exemptedUser.id())
+ .project(project)
+ .commitMessage("Change Renaming A File")
+ .childOf()
+ .change(baseChange.id())
+ .file(JgitPath.of(oldPath).get())
+ .renameTo(JgitPath.of(newPath).get())
+ .createAndGet();
+
+ CodeOwnerStatusInfo codeOwnerStatus =
+ changeCodeOwnersApiFactory.change(changeWithRename.id().id()).getCodeOwnerStatus().get();
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ oldPath,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "patch set uploader %s is exempted from requiring code owner approvals",
+ AccountTemplateUtil.getAccountTemplate(exemptedUser.id())),
+ newPath,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "patch set uploader %s is exempted from requiring code owner approvals",
+ AccountTemplateUtil.getAccountTemplate(exemptedUser.id()))));
+ }
+
+ @GerritConfig(name = "plugin.code-owners.exemptPureReverts", value = "true")
+ @Test
+ public void getStatusForRenamedFile_exemptedPureRevert() throws Exception {
+ setAsRootCodeOwners(admin);
+
+ String oldPath = "foo/bar/abc.txt";
+ String newPath = "foo/baz/abc.txt";
+
+ TestChange baseChange =
+ changeOperations
+ .newChange()
+ .project(project)
+ .commitMessage("Change Adding A File")
+ .file(JgitPath.of(oldPath).get())
+ .content("file content")
+ .createAndGet();
+
+ TestChange changeWithRename =
+ changeOperations
+ .newChange()
+ .project(project)
+ .commitMessage("Change Renaming A File")
+ .childOf()
+ .change(baseChange.id())
+ .file(JgitPath.of(oldPath).get())
+ .renameTo(JgitPath.of(newPath).get())
+ .createAndGet();
+
+ // Approve and submit both changes, since a revert change can only be created for a submitted
+ // change.
+ changeOperations
+ .change(baseChange.id())
+ .newVote()
+ .codeReviewApproval()
+ .user(admin.id())
+ .create();
+ changeOperations.change(changeWithRename.id()).newVote().codeReviewApproval().create();
+ changeOperations
+ .change(changeWithRename.id())
+ .newVote()
+ .codeReviewApproval()
+ .user(admin.id())
+ .create();
+ gApi.changes().id(changeWithRename.id()).current().submit();
+
+ // Revert the change
+ ChangeInfo changeInfoOfRevert = gApi.changes().id(changeWithRename.id()).revert().get();
+ TestChange revertChange =
+ changeOperations
+ .change(
+ ChangeIdentifier.byProjectAndNumericChangeId(
+ changeInfoOfRevert.project, changeInfoOfRevert._number))
+ .get();
+
+ CodeOwnerStatusInfo codeOwnerStatus =
+ changeCodeOwnersApiFactory.change(revertChange.id().id()).getCodeOwnerStatus().get();
+ assertThat(codeOwnerStatus)
+ .hasFileCodeOwnerStatusesThat()
+ .comparingElementsUsing(isFileCodeOwnerStatus())
+ .containsExactly(
+ FileCodeOwnerStatus.rename(
+ newPath,
+ CodeOwnerStatus.APPROVED,
+ "change is a pure revert and is exempted from requiring code owner approvals",
+ oldPath,
+ CodeOwnerStatus.APPROVED,
+ "change is a pure revert and is exempted from requiring code owner approvals"));
+ }
+
+ @Test
public void getCodeOwnerStatusIfCodeOwnersFunctionalityIsDisabled() throws Exception {
disableCodeOwnersForProject(project);
String path = "foo/bar.baz";
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
index 5df0afb..02016d8 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
@@ -79,7 +79,9 @@
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> changedFiles.get(/* project= */ null, ObjectId.zeroId()));
+ () ->
+ changedFiles.get(
+ /* project= */ null, ObjectId.zeroId(), /* enableRenameDetection= */ false));
assertThat(npe).hasMessageThat().isEqualTo("project");
}
@@ -87,7 +89,10 @@
public void cannotGetForNullRevision_v1() throws Exception {
NullPointerException npe =
assertThrows(
- NullPointerException.class, () -> changedFiles.get(project, /* revision= */ null));
+ NullPointerException.class,
+ () ->
+ changedFiles.get(
+ project, /* revision= */ null, /* enableRenameDetection= */ false));
assertThat(npe).hasMessageThat().isEqualTo("revision");
}
@@ -98,7 +103,10 @@
NullPointerException.class,
() ->
changedFiles.get(
- /* project= */ null, ObjectId.zeroId(), MergeCommitStrategy.ALL_CHANGED_FILES));
+ /* project= */ null,
+ ObjectId.zeroId(),
+ MergeCommitStrategy.ALL_CHANGED_FILES,
+ /* enableRenameDetection= */ false));
assertThat(npe).hasMessageThat().isEqualTo("project");
}
@@ -109,7 +117,10 @@
NullPointerException.class,
() ->
changedFiles.get(
- project, /* revision= */ null, MergeCommitStrategy.ALL_CHANGED_FILES));
+ project,
+ /* revision= */ null,
+ MergeCommitStrategy.ALL_CHANGED_FILES,
+ /* enableRenameDetection= */ false));
assertThat(npe).hasMessageThat().isEqualTo("revision");
}
@@ -118,7 +129,12 @@
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> changedFiles.get(project, ObjectId.zeroId(), /* mergeCommitStrategy= */ null));
+ () ->
+ changedFiles.get(
+ project,
+ ObjectId.zeroId(),
+ /* mergeCommitStrategy= */ null,
+ /* enableRenameDetection= */ false));
assertThat(npe).hasMessageThat().isEqualTo("mergeCommitStrategy");
}
@@ -129,7 +145,11 @@
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getCommit();
ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.get(project, commit, MergeCommitStrategy.ALL_CHANGED_FILES);
+ changedFiles.get(
+ project,
+ commit,
+ MergeCommitStrategy.ALL_CHANGED_FILES,
+ /* enableRenameDetection= */ false);
assertThat(changedFilesSet).hasSize(1);
ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
assertThat(changedFile).hasNewPath().value().isEqualTo(Path.of(path));
@@ -148,7 +168,11 @@
.getCommit();
ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.get(project, commit, MergeCommitStrategy.ALL_CHANGED_FILES);
+ changedFiles.get(
+ project,
+ commit,
+ MergeCommitStrategy.ALL_CHANGED_FILES,
+ /* enableRenameDetection= */ false);
assertThat(changedFilesSet).hasSize(1);
ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
assertThat(changedFile).hasNewPath().value().isEqualTo(Path.of(path));
@@ -166,7 +190,8 @@
changedFiles.get(
project,
getRevisionResource(change.id()).getPatchSet().commitId(),
- MergeCommitStrategy.ALL_CHANGED_FILES);
+ MergeCommitStrategy.ALL_CHANGED_FILES,
+ /* enableRenameDetection= */ false);
assertThat(changedFilesSet).hasSize(1);
ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
assertThat(changedFile).hasNewPath().isEmpty();
@@ -187,7 +212,8 @@
changedFiles.get(
project,
getRevisionResource(change.id()).getPatchSet().commitId(),
- MergeCommitStrategy.ALL_CHANGED_FILES);
+ MergeCommitStrategy.ALL_CHANGED_FILES,
+ /* enableRenameDetection= */ true);
ChangedFileSubject changedFile = assertThatCollection(changedFilesSet).onlyElement();
changedFile.hasNewPath().value().isEqualTo(Path.of(newPath));
changedFile.hasOldPath().value().isEqualTo(Path.of(oldPath));
@@ -221,7 +247,11 @@
assertThat(commit.getParents()).isEmpty();
ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.get(project, commit, MergeCommitStrategy.ALL_CHANGED_FILES);
+ changedFiles.get(
+ project,
+ commit,
+ MergeCommitStrategy.ALL_CHANGED_FILES,
+ /* enableRenameDetection= */ false);
assertThat(changedFilesSet).hasSize(1);
ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet);
assertThat(changedFile).hasNewPath().value().isEqualTo(Path.of(path));
@@ -314,7 +344,8 @@
changedFiles.get(
project,
getRevisionResource(mergeChange).getPatchSet().commitId(),
- mergeCommitStrategy);
+ mergeCommitStrategy,
+ /* enableRenameDetection= */ false);
if (MergeCommitStrategy.ALL_CHANGED_FILES.equals(mergeCommitStrategy)) {
assertThat(changedFilesSet).comparingElementsUsing(hasPath()).containsExactly(file1, file2);
@@ -406,7 +437,8 @@
changedFiles.get(
project,
getRevisionResource(mergeChange).getPatchSet().commitId(),
- mergeCommitStrategy);
+ mergeCommitStrategy,
+ /* enableRenameDetection= */ false);
ImmutableSet<String> oldPaths =
changedFilesSet.stream()
.map(changedFile -> JgitPath.of(changedFile.oldPath().get()).get())
@@ -438,7 +470,11 @@
.getCommit();
ImmutableList<ChangedFile> changedFilesSet =
- changedFiles.get(project, commit, MergeCommitStrategy.ALL_CHANGED_FILES);
+ changedFiles.get(
+ project,
+ commit,
+ MergeCommitStrategy.ALL_CHANGED_FILES,
+ /* enableRenameDetection= */ false);
assertThat(changedFilesSet)
.comparingElementsUsing(hasPath())
.containsExactly(file4, file3, file5, file1, file2)
@@ -554,7 +590,8 @@
changedFiles.get(
project,
getRevisionResource(mergeChange).getPatchSet().commitId(),
- mergeCommitStrategy);
+ mergeCommitStrategy,
+ /* enableRenameDetection= */ false);
if (MergeCommitStrategy.ALL_CHANGED_FILES.equals(mergeCommitStrategy)) {
assertThat(changedFilesSet)
