CodeOwnerApprovalCheck: Fix exception when no old path is set for deletion
For deleted files we expect that that the old path is present and that
no new path is set. However for deleted files in merge commits that
resolved a conflict between a file modification in the destination
branch and a deletion in the source branch, the new path was present and
the old path was not set when the comparison was done against the
AutoMerge. This caused an IllegalStateException in
CodeOwnerApprovalCheck saying "old path must be present for
deletion/rename".
The reason for this was that PatchListEntry sets the old path as new
name and the old name is unset (see PatchListEntry constructor), which
is inconsistent with DiffEntry. PatchListEntry is only used when the
comparison is done against the AutoMerge which is why we didn't catch
this bug with the existing tests for file deletions. To fix this we now
get the old path from the new name field in PatchListEntry when we are
constructing a ChangedFile instance from a PatchListEntry for a file
deletion.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I99fd443317f360456bfe3199ac2819b7b19ff0d7
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFile.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFile.java
index 3ead395..82655cf 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFile.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFile.java
@@ -137,6 +137,15 @@
public static ChangedFile create(PatchListEntry patchListEntry) {
requireNonNull(patchListEntry, "patchListEntry");
+ if (patchListEntry.getChangeType() == Patch.ChangeType.DELETED) {
+ // For deletions PatchListEntry sets the old path as new name and the old name is unset (see
+ // PatchListEntry constructor). This means to get the old path we need to read the new name.
+ return new AutoValue_ChangedFile(
+ Optional.empty(),
+ convertPathFromPatchListEntry(patchListEntry.getNewName()),
+ CHANGE_TYPE.get(patchListEntry.getChangeType()));
+ }
+
return new AutoValue_ChangedFile(
convertPathFromPatchListEntry(patchListEntry.getNewName()),
convertPathFromPatchListEntry(patchListEntry.getOldName()),
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java
index 95fef12..a05799d 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java
@@ -316,8 +316,14 @@
private void setupPatchListEntry(
@Nullable String newPath, @Nullable String oldPath, Patch.ChangeType changeType) {
- when(patchListEntry.getNewName()).thenReturn(newPath);
- when(patchListEntry.getOldName()).thenReturn(oldPath);
- when(patchListEntry.getChangeType()).thenReturn(changeType);
+ if (Patch.ChangeType.DELETED == changeType) {
+ // for deletions PatchListEntry sets the oldPath as new name
+ when(patchListEntry.getNewName()).thenReturn(oldPath);
+ when(patchListEntry.getChangeType()).thenReturn(changeType);
+ } else {
+ when(patchListEntry.getNewName()).thenReturn(newPath);
+ when(patchListEntry.getOldName()).thenReturn(oldPath);
+ when(patchListEntry.getChangeType()).thenReturn(changeType);
+ }
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
index 8694f92..19d5fdd 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
@@ -22,6 +22,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
+import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.PushOneCommit.Result;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.config.GerritConfig;
@@ -243,6 +244,74 @@
}
}
+ @Test
+ @GerritConfig(name = "plugin.code-owners.mergeCommitStrategy", value = "ALL_CHANGED_FILES")
+ public void computeForMergeChangeThatContainsADeletedFileAsConflictResolution_allChangedFiles()
+ throws Exception {
+ testComputeForMergeChangeThatContainsADeletedFileAsConflictResolution();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.mergeCommitStrategy",
+ value = "FILES_WITH_CONFLICT_RESOLUTION")
+ public void
+ computeForMergeChangeThatContainsADeletedFileAsConflictResolution_filesWithConflictResolution()
+ throws Exception {
+ testComputeForMergeChangeThatContainsADeletedFileAsConflictResolution();
+ }
+
+ private void testComputeForMergeChangeThatContainsADeletedFileAsConflictResolution()
+ throws Exception {
+ String file = "foo/a.txt";
+
+ // Create a base change.
+ Change.Id baseChange =
+ changeOperations.newChange().branch("master").file(file).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.
+ Change.Id changeInMaster =
+ changeOperations.newChange().branch("master").file(file).content("master content").create();
+ approveAndSubmit(changeInMaster);
+
+ // Create a change in the other branch and that deleted file1.
+ PushOneCommit push =
+ pushFactory.create(admin.newIdent(), testRepo, "Change Deleting A File", file, "");
+ Result r = push.rm("refs/for/master");
+ r.assertOkStatus();
+ approveAndSubmit(r.getChange().getId());
+
+ // Create a merge change with resolving the conflict on file between the edit in master and the
+ // deletion in the other branch by deleting the file.
+ Change.Id mergeChange =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .mergeOf()
+ .tipOfBranch("master")
+ .and()
+ .tipOfBranch(branchName)
+ .file(file)
+ .delete()
+ .create();
+
+ ImmutableSet<ChangedFile> changedFilesSet =
+ changedFiles.compute(getRevisionResource(Integer.toString(mergeChange.get())));
+ ImmutableSet<String> oldPaths =
+ changedFilesSet.stream()
+ .map(changedFile -> JgitPath.of(changedFile.oldPath().get()).get())
+ .collect(toImmutableSet());
+ assertThat(oldPaths).containsExactly(file);
+ }
+
private void approveAndSubmit(Change.Id changeId) throws Exception {
approve(Integer.toString(changeId.get()));
gApi.changes().id(changeId.get()).current().submit();
