ChangedFiles: Rename 'getFromDiffCache' methods to 'get' Earlier ChangedFiles offered 2 alternatives for geting the changed files, one that computed the changed files and one that retrieved them from the diff cache. The first alternative was dropped in change I811c28f64 and now the changed files are always retrieved from the diff cache. Back then when the 2 alternatives existed it was important that they could be distinguished by the method, but now that only 1 way to get the changed files exists the "FromDiffCache" part of the method names that exposes an implementation detail is no longer needed. Dropping the "FromDiffCache" part from the method names will allow us to change the implementation later, e.g. to get the changed files in another way than retrieving them from the diff cache. Change-Id: I454e4896a7295432306c6bd620e805edf9ae245a 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 bdcf002..07b42bc 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
@@ -47,8 +47,8 @@ /** * Class to get the files that have been changed in a revision. * - * <p>The {@link #getFromDiffCache(Project.NameKey, ObjectId, MergeCommitStrategy)} method is - * retrieving the file diff from the diff cache and has rename detection enabled. + * <p>The {@link #get(Project.NameKey, ObjectId, MergeCommitStrategy)} method is retrieving the file + * diff from the diff cache and has rename detection enabled. * * <p>The {@link com.google.gerrit.server.patch.PatchListCache} is deprecated, and hence it not * being used here. @@ -73,7 +73,7 @@ } /** - * Gets the changed files from the diff cache. + * Gets the changed files. * * <p>Rename detection is enabled. * @@ -83,7 +83,7 @@ * files for merge commits * @return the files that have been changed in the given revision, sorted alphabetically by path */ - public ImmutableList<ChangedFile> getFromDiffCache( + public ImmutableList<ChangedFile> get( Project.NameKey project, ObjectId revision, MergeCommitStrategy mergeCommitStrategy) throws IOException, DiffNotAvailableException { requireNonNull(project, "project"); @@ -101,7 +101,7 @@ // if the merge commit strategy is FILES_WITH_CONFLICT_RESOLUTION. fileDiffOutputs = diffOperations.listModifiedFilesAgainstParent( - project, revision, /* parentNum=*/ 0, DiffOptions.DEFAULTS); + project, revision, /* parentNum= */ 0, DiffOptions.DEFAULTS); } else { checkState(mergeCommitStrategy.equals(MergeCommitStrategy.ALL_CHANGED_FILES)); // Always use parent 1 to do the comparison. @@ -121,7 +121,7 @@ } /** - * Gets the changed files from the diff cache. + * Gets the changed files. * * <p>Rename detection is enabled. * @@ -132,18 +132,18 @@ * @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> getFromDiffCache(Project.NameKey project, ObjectId revision) + public ImmutableList<ChangedFile> get(Project.NameKey project, ObjectId revision) throws IOException, DiffNotAvailableException { requireNonNull(project, "project"); requireNonNull(revision, "revision"); - return getFromDiffCache( + return get( project, revision, codeOwnersPluginConfiguration.getProjectConfig(project).getMergeCommitStrategy()); } /** - * Gets the changed files from the diff cache. + * Gets the changed files. * * <p>Rename detection is enabled. * @@ -152,13 +152,12 @@ * @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 #getFromDiffCache(Project.NameKey, ObjectId, MergeCommitStrategy) + * @see #get(Project.NameKey, ObjectId, MergeCommitStrategy) */ - public ImmutableList<ChangedFile> getFromDiffCache(RevisionResource revisionResource) + public ImmutableList<ChangedFile> get(RevisionResource revisionResource) throws IOException, DiffNotAvailableException { requireNonNull(revisionResource, "revisionResource"); - return getFromDiffCache( - revisionResource.getProject(), revisionResource.getPatchSet().commitId()); + return get(revisionResource.getProject(), revisionResource.getPatchSet().commitId()); } /** @@ -197,7 +196,7 @@ // if the merge commit strategy is FILES_WITH_CONFLICT_RESOLUTION. modifiedFiles = diffOperationsForCommitValidation.loadModifiedFilesAgainstParentIfNecessary( - project, revision, /* parentNum=*/ 0, /* enableRenameDetection= */ true); + project, revision, /* parentNum= */ 0, /* enableRenameDetection= */ true); } else { checkState(mergeCommitStrategy.equals(MergeCommitStrategy.ALL_CHANGED_FILES)); // Always use parent 1 to do the comparison.
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFilesByPatchSetCache.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFilesByPatchSetCache.java index 3d5fc9c..48382d0 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFilesByPatchSetCache.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFilesByPatchSetCache.java
@@ -71,7 +71,7 @@ changeNotes.getChange().getId().get()); PatchSet patchSet = getPatchSet(patchSetId); try { - return changedFiles.getFromDiffCache( + return changedFiles.get( changeNotes.getProjectName(), patchSet.commitId(), codeOwnersConfig.getMergeCommitStrategy());
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java index afaf7ad..e776abd 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -363,8 +363,7 @@ ChangedFilesByPatchSetCache changedFilesByPatchSetCache = changedFilesByPatchSetCacheFactory.create(codeOwnersConfig, changeNotes); return changedFiles - .getFromDiffCache( - changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId()) + .get(changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId()) .stream() .map( changedFile -> @@ -438,9 +437,7 @@ codeOwnersConfig, codeOwnerResolver, changeNotes, accountIds); ChangedFilesByPatchSetCache changedFilesByPatchSetCache = changedFilesByPatchSetCacheFactory.create(codeOwnersConfig, changeNotes); - return changedFiles - .getFromDiffCache(changeNotes.getProjectName(), patchSet.commitId()) - .stream() + return changedFiles.get(changeNotes.getProjectName(), patchSet.commitId()).stream() .map( changedFile -> getFileStatus( @@ -472,7 +469,7 @@ ChangeNotes changeNotes, PatchSet patchSet, String reason) throws IOException, DiffNotAvailableException { logger.atFine().log("all paths are approved (reason = %s)", reason); - return changedFiles.getFromDiffCache(changeNotes.getProjectName(), patchSet.commitId()).stream() + return changedFiles.get(changeNotes.getProjectName(), patchSet.commitId()).stream() .map( changedFile -> FileCodeOwnerStatus.create(
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFilesInRevision.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFilesInRevision.java index d4dcede..930ab0f 100644 --- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFilesInRevision.java +++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwnerConfigFilesInRevision.java
@@ -95,7 +95,7 @@ RevWalk rw = new RevWalk(repository)) { RevCommit commit = rw.parseCommit(revisionResource.getPatchSet().commitId()); return Response.ok( - changedFiles.getFromDiffCache(revisionResource.getProject(), commit).stream() + changedFiles.get(revisionResource.getProject(), commit).stream() // filter out deletions (files without new path) .filter(changedFile -> changedFile.newPath().isPresent()) // filter out non code owner config files
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java index 642d605..53473f2 100644 --- a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java +++ b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java
@@ -69,7 +69,9 @@ @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 @@ -103,7 +105,7 @@ private void checkThatFileExists( RevisionResource revisionResource, PathResource pathResource, IdString id) throws RestApiException, IOException, DiffNotAvailableException { - if (!changedFiles.getFromDiffCache(revisionResource).stream() + if (!changedFiles.get(revisionResource).stream() .anyMatch( changedFile -> // Check whether the path matches any file in the change.
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java b/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java index 7d46516..dafa555 100644 --- a/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java +++ b/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java
@@ -98,7 +98,7 @@ // (MergeCommitStrategy.ALL_CHANGED_FILES) as this is what CodeOwnerConfigValidator does. try { return changedFiles - .getFromDiffCache( + .get( changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId(), MergeCommitStrategy.ALL_CHANGED_FILES)
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java index 844d9e9..aa42837 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
@@ -69,8 +69,7 @@ public void cannotGetFromDiffCacheForNullRevisionResource() throws Exception { NullPointerException npe = assertThrows( - NullPointerException.class, - () -> changedFiles.getFromDiffCache(/* revisionResource= */ null)); + NullPointerException.class, () -> changedFiles.get(/* revisionResource= */ null)); assertThat(npe).hasMessageThat().isEqualTo("revisionResource"); } @@ -79,7 +78,7 @@ NullPointerException npe = assertThrows( NullPointerException.class, - () -> changedFiles.getFromDiffCache(/* project= */ null, ObjectId.zeroId())); + () -> changedFiles.get(/* project= */ null, ObjectId.zeroId())); assertThat(npe).hasMessageThat().isEqualTo("project"); } @@ -87,8 +86,7 @@ public void cannotGetFromDiffCacheForNullRevision_v1() throws Exception { NullPointerException npe = assertThrows( - NullPointerException.class, - () -> changedFiles.getFromDiffCache(project, /* revision= */ null)); + NullPointerException.class, () -> changedFiles.get(project, /* revision= */ null)); assertThat(npe).hasMessageThat().isEqualTo("revision"); } @@ -98,7 +96,7 @@ assertThrows( NullPointerException.class, () -> - changedFiles.getFromDiffCache( + changedFiles.get( /* project= */ null, ObjectId.zeroId(), MergeCommitStrategy.ALL_CHANGED_FILES)); assertThat(npe).hasMessageThat().isEqualTo("project"); } @@ -109,7 +107,7 @@ assertThrows( NullPointerException.class, () -> - changedFiles.getFromDiffCache( + changedFiles.get( project, /* revision= */ null, MergeCommitStrategy.ALL_CHANGED_FILES)); assertThat(npe).hasMessageThat().isEqualTo("revision"); } @@ -119,9 +117,7 @@ NullPointerException npe = assertThrows( NullPointerException.class, - () -> - changedFiles.getFromDiffCache( - project, ObjectId.zeroId(), /* mergeCommitStrategy= */ null)); + () -> changedFiles.get(project, ObjectId.zeroId(), /* mergeCommitStrategy= */ null)); assertThat(npe).hasMessageThat().isEqualTo("mergeCommitStrategy"); } @@ -132,7 +128,7 @@ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getCommit(); ImmutableList<ChangedFile> changedFilesSet = - changedFiles.getFromDiffCache(project, commit, MergeCommitStrategy.ALL_CHANGED_FILES); + changedFiles.get(project, commit, MergeCommitStrategy.ALL_CHANGED_FILES); assertThat(changedFilesSet).hasSize(1); ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet); assertThat(changedFile).hasNewPath().value().isEqualTo(Path.of(path)); @@ -151,7 +147,7 @@ .getCommit(); ImmutableList<ChangedFile> changedFilesSet = - changedFiles.getFromDiffCache(project, commit, MergeCommitStrategy.ALL_CHANGED_FILES); + changedFiles.get(project, commit, MergeCommitStrategy.ALL_CHANGED_FILES); assertThat(changedFilesSet).hasSize(1); ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet); assertThat(changedFile).hasNewPath().value().isEqualTo(Path.of(path)); @@ -166,7 +162,7 @@ TestChange change = createChangeWithFileDeletion(path); ImmutableList<ChangedFile> changedFilesSet = - changedFiles.getFromDiffCache( + changedFiles.get( project, getRevisionResource(change.id()).getPatchSet().commitId(), MergeCommitStrategy.ALL_CHANGED_FILES); @@ -187,7 +183,7 @@ gApi.changes().id(change.id()).current().files(); ImmutableList<ChangedFile> changedFilesSet = - changedFiles.getFromDiffCache( + changedFiles.get( project, getRevisionResource(change.id()).getPatchSet().commitId(), MergeCommitStrategy.ALL_CHANGED_FILES); @@ -207,7 +203,7 @@ assertThat(commit.getParents()).isEmpty(); ImmutableList<ChangedFile> changedFilesSet = - changedFiles.getFromDiffCache(project, commit, MergeCommitStrategy.ALL_CHANGED_FILES); + changedFiles.get(project, commit, MergeCommitStrategy.ALL_CHANGED_FILES); assertThat(changedFilesSet).hasSize(1); ChangedFile changedFile = Iterables.getOnlyElement(changedFilesSet); assertThat(changedFile).hasNewPath().value().isEqualTo(Path.of(path)); @@ -297,7 +293,7 @@ .create(); ImmutableList<ChangedFile> changedFilesSet = - changedFiles.getFromDiffCache( + changedFiles.get( project, getRevisionResource(mergeChange).getPatchSet().commitId(), mergeCommitStrategy); @@ -389,7 +385,7 @@ .create(); ImmutableList<ChangedFile> changedFilesSet = - changedFiles.getFromDiffCache( + changedFiles.get( project, getRevisionResource(mergeChange).getPatchSet().commitId(), mergeCommitStrategy); @@ -424,7 +420,7 @@ .getCommit(); ImmutableList<ChangedFile> changedFilesSet = - changedFiles.getFromDiffCache(project, commit, MergeCommitStrategy.ALL_CHANGED_FILES); + changedFiles.get(project, commit, MergeCommitStrategy.ALL_CHANGED_FILES); assertThat(changedFilesSet) .comparingElementsUsing(hasPath()) .containsExactly(file4, file3, file5, file1, file2) @@ -540,7 +536,7 @@ .create(); ImmutableList<ChangedFile> changedFilesSet = - changedFiles.getFromDiffCache( + changedFiles.get( project, getRevisionResource(mergeChange).getPatchSet().commitId(), mergeCommitStrategy);