Fix FilesInCommitCollection when parent = 0
Change Ia3a97599f8 refactored FileInfoJson to have two implementations:
the current one, and another using the new diff cache redesign. The
previous refactoring also modified FilesInCommitCollection to use the
new FileInfoJson API.
The previous implementation allowed passing a value '0' for parentNum to
FilesInCommitCollection, which was not handled in this refactoring. This
change fixes this case in FileInfoJsonOldImpl and adds tests for
FilesInCommitCollection.
Passing a value '0' was not mentioned in the docs, although it was
handled in the code. I modified the docs to include this case.
Change-Id: I65d505e574acbaf64c68f5cc7ec622ae3f2ab923
diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt
index a3592b1..677fc6d 100644
--- a/Documentation/rest-api-projects.txt
+++ b/Documentation/rest-api-projects.txt
@@ -2704,7 +2704,10 @@
The integer-valued request parameter `parent` changes the response to return a
list of the files which are different in this commit compared to the given
parent commit. This is useful for supporting review of merge commits. The value
-is the 1-based index of the parent's position in the commit object.
+is the 1-based index of the parent's position in the commit object. If the
+value 0 is used for `parent`, the default base commit will be used, which is
+the only parent for commits having one parent or the auto-merge commit
+otherwise.
[[dashboard-endpoints]]
== Dashboard Endpoints
diff --git a/java/com/google/gerrit/extensions/api/projects/CommitApi.java b/java/com/google/gerrit/extensions/api/projects/CommitApi.java
index a53fc74..b0cc9da 100644
--- a/java/com/google/gerrit/extensions/api/projects/CommitApi.java
+++ b/java/com/google/gerrit/extensions/api/projects/CommitApi.java
@@ -18,8 +18,10 @@
import com.google.gerrit.extensions.api.changes.CherryPickInput;
import com.google.gerrit.extensions.api.changes.IncludedInInfo;
import com.google.gerrit.extensions.common.CommitInfo;
+import com.google.gerrit.extensions.common.FileInfo;
import com.google.gerrit.extensions.restapi.NotImplementedException;
import com.google.gerrit.extensions.restapi.RestApiException;
+import java.util.Map;
public interface CommitApi {
CommitInfo get() throws RestApiException;
@@ -28,6 +30,9 @@
IncludedInInfo includedIn() throws RestApiException;
+ /** List files in a specific commit against the parent commit. */
+ Map<String, FileInfo> files(int parentNum) throws RestApiException;
+
/** A default implementation for source compatibility when adding new methods to the interface. */
class NotImplemented implements CommitApi {
@Override
@@ -44,5 +49,10 @@
public IncludedInInfo includedIn() throws RestApiException {
throw new NotImplementedException();
}
+
+ @Override
+ public Map<String, FileInfo> files(int parentNum) throws RestApiException {
+ throw new NotImplementedException();
+ }
}
}
diff --git a/java/com/google/gerrit/server/api/projects/CommitApiImpl.java b/java/com/google/gerrit/server/api/projects/CommitApiImpl.java
index 5c7921a..e055a00 100644
--- a/java/com/google/gerrit/server/api/projects/CommitApiImpl.java
+++ b/java/com/google/gerrit/server/api/projects/CommitApiImpl.java
@@ -22,13 +22,16 @@
import com.google.gerrit.extensions.api.changes.IncludedInInfo;
import com.google.gerrit.extensions.api.projects.CommitApi;
import com.google.gerrit.extensions.common.CommitInfo;
+import com.google.gerrit.extensions.common.FileInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.project.CommitResource;
import com.google.gerrit.server.restapi.change.CherryPickCommit;
import com.google.gerrit.server.restapi.project.CommitIncludedIn;
+import com.google.gerrit.server.restapi.project.FilesInCommitCollection;
import com.google.gerrit.server.restapi.project.GetCommit;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import java.util.Map;
public class CommitApiImpl implements CommitApi {
public interface Factory {
@@ -40,6 +43,7 @@
private final CherryPickCommit cherryPickCommit;
private final CommitIncludedIn includedIn;
private final CommitResource commitResource;
+ private final FilesInCommitCollection.ListFiles listFiles;
@Inject
CommitApiImpl(
@@ -47,11 +51,13 @@
GetCommit getCommit,
CherryPickCommit cherryPickCommit,
CommitIncludedIn includedIn,
+ FilesInCommitCollection.ListFiles listFiles,
@Assisted CommitResource commitResource) {
this.changes = changes;
this.getCommit = getCommit;
this.cherryPickCommit = cherryPickCommit;
this.includedIn = includedIn;
+ this.listFiles = listFiles;
this.commitResource = commitResource;
}
@@ -81,4 +87,13 @@
throw asRestApiException("Could not extract IncludedIn data", e);
}
}
+
+ @Override
+ public Map<String, FileInfo> files(int parentNum) throws RestApiException {
+ try {
+ return listFiles.setParent(parentNum).apply(commitResource).value();
+ } catch (Exception e) {
+ throw asRestApiException("Cannot retrieve files", e);
+ }
+ }
}
diff --git a/java/com/google/gerrit/server/change/FileInfoJson.java b/java/com/google/gerrit/server/change/FileInfoJson.java
index 03ce318..ad6f9c7 100644
--- a/java/com/google/gerrit/server/change/FileInfoJson.java
+++ b/java/com/google/gerrit/server/change/FileInfoJson.java
@@ -69,7 +69,8 @@
/**
* Computes the list of modified files for a given project and commit against its parent. For
* merge commits, callers can use 0, 1, 2, etc... to choose a specific parent. The first parent is
- * 0.
+ * 0. A value of -1 for parent can be passed to use the default base commit, which is the only
+ * parent for commits having only one parent, or the auto-merge otherwise.
*
* @param project a project identifying a repository.
* @param objectId a commit SHA-1 identifying a patchset commit.
diff --git a/java/com/google/gerrit/server/change/FileInfoJsonOldImpl.java b/java/com/google/gerrit/server/change/FileInfoJsonOldImpl.java
index 2ac7a87..55d162a 100644
--- a/java/com/google/gerrit/server/change/FileInfoJsonOldImpl.java
+++ b/java/com/google/gerrit/server/change/FileInfoJsonOldImpl.java
@@ -60,8 +60,10 @@
Project.NameKey project, ObjectId objectId, int parentNum)
throws ResourceConflictException, PatchListNotAvailableException {
PatchListKey key =
- PatchListKey.againstParentNum(
- parentNum + 1, objectId, DiffPreferencesInfo.Whitespace.IGNORE_NONE);
+ parentNum == -1
+ ? PatchListKey.againstDefaultBase(objectId, Whitespace.IGNORE_NONE)
+ : PatchListKey.againstParentNum(
+ parentNum + 1, objectId, DiffPreferencesInfo.Whitespace.IGNORE_NONE);
return toFileInfoMap(project, key);
}
diff --git a/java/com/google/gerrit/server/restapi/project/FilesInCommitCollection.java b/java/com/google/gerrit/server/restapi/project/FilesInCommitCollection.java
index 3fcaeb8..7bee2f2 100644
--- a/java/com/google/gerrit/server/restapi/project/FilesInCommitCollection.java
+++ b/java/com/google/gerrit/server/restapi/project/FilesInCommitCollection.java
@@ -87,6 +87,11 @@
this.fileInfoJson = fileInfoJson;
}
+ public ListFiles setParent(int parentNum) {
+ this.parentNum = parentNum;
+ return this;
+ }
+
@Override
public Response<Map<String, FileInfo>> apply(CommitResource resource)
throws ResourceConflictException, PatchListNotAvailableException {
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/FilesInCommitIT.java b/javatests/com/google/gerrit/acceptance/rest/project/FilesInCommitIT.java
new file mode 100644
index 0000000..74ba48e
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/rest/project/FilesInCommitIT.java
@@ -0,0 +1,137 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance.rest.project;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.PushOneCommit.Result;
+import com.google.gerrit.common.RawInputUtil;
+import com.google.gerrit.extensions.common.FileInfo;
+import com.google.gerrit.extensions.restapi.BinaryResult;
+import com.google.gerrit.server.restapi.project.FilesInCommitCollection;
+import java.util.Map;
+import java.util.function.Function;
+import org.eclipse.jgit.lib.ObjectId;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Test class for {@link FilesInCommitCollection}. */
+public class FilesInCommitIT extends AbstractDaemonTest {
+ private String changeId;
+
+ @Before
+ public void setUp() throws Exception {
+ baseConfig.setString("cache", "diff", "timeout", "1 minute");
+
+ ObjectId headCommit = testRepo.getRepository().resolve("HEAD");
+ addCommit(
+ headCommit,
+ ImmutableMap.of("file_1.txt", "file 1 content", "file_2.txt", "file 2 content"));
+
+ Result result = createEmptyChange();
+ changeId = result.getChangeId();
+ }
+
+ @Test
+ public void listFilesForSingleParentCommit() throws Exception {
+ gApi.changes()
+ .id(changeId)
+ .edit()
+ .modifyFile("a_new_file.txt", RawInputUtil.create("Line 1\nLine 2\nLine 3"));
+ gApi.changes().id(changeId).edit().deleteFile("file_1.txt");
+ gApi.changes().id(changeId).edit().publish();
+
+ String lastCommitId = gApi.changes().id(changeId).get().currentRevision;
+
+ // When parentNum is 0, the diff is performed against the default base, i.e. the single parent
+ // in this case.
+ Map<String, FileInfo> changedFiles =
+ gApi.projects().name(project.get()).commit(lastCommitId).files(0);
+
+ assertThat(changedFiles.keySet())
+ .containsExactly("/COMMIT_MSG", "a_new_file.txt", "file_1.txt");
+ }
+
+ @Test
+ public void listFilesForMergeCommitAgainstParent1() throws Exception {
+ PushOneCommit.Result result = createMergeCommitChange("refs/for/master", "my_file.txt");
+
+ String changeId = result.getChangeId();
+ addModifiedPatchSet(changeId, "my_file.txt", content -> content.concat("Line I\nLine II\n"));
+
+ String lastCommitId = gApi.changes().id(changeId).get().currentRevision;
+
+ // Diffing against the first parent.
+ Map<String, FileInfo> changedFiles =
+ gApi.projects().name(project.get()).commit(lastCommitId).files(1);
+
+ assertThat(changedFiles.keySet())
+ .containsExactly(
+ "/COMMIT_MSG",
+ "/MERGE_LIST",
+ "bar", // file bar is coming from parent two
+ "my_file.txt");
+ }
+
+ @Test
+ public void listFilesForMergeCommitAgainstDefaultParent() throws Exception {
+ PushOneCommit.Result result = createMergeCommitChange("refs/for/master", "my_file.txt");
+
+ String changeId = result.getChangeId();
+ addModifiedPatchSet(changeId, "my_file.txt", content -> content.concat("Line I\nLine II\n"));
+
+ String lastCommitId = gApi.changes().id(changeId).get().currentRevision;
+
+ // When parentNum is 0, the diff is performed against the default base. In this case, the
+ // auto-merge commit.
+ Map<String, FileInfo> changedFiles =
+ gApi.projects().name(project.get()).commit(lastCommitId).files(0);
+
+ assertThat(changedFiles.keySet())
+ .containsExactly(
+ "/COMMIT_MSG",
+ "/MERGE_LIST",
+ "bar", // file bar is coming from parent two
+ "my_file.txt");
+ }
+
+ private void addModifiedPatchSet(
+ String changeId, String filePath, Function<String, String> contentModification)
+ throws Exception {
+ try (BinaryResult content = gApi.changes().id(changeId).current().file(filePath).content()) {
+ String newContent = contentModification.apply(content.asString());
+ gApi.changes().id(changeId).edit().modifyFile(filePath, RawInputUtil.create(newContent));
+ }
+ gApi.changes().id(changeId).edit().publish();
+ }
+
+ private ObjectId addCommit(ObjectId parentCommit, ImmutableMap<String, String> files)
+ throws Exception {
+ testRepo.reset(parentCommit);
+ PushOneCommit push =
+ pushFactory.create(admin.newIdent(), testRepo, "Adjust files of repo", files);
+ PushOneCommit.Result result = push.to("refs/for/master");
+ return result.getCommit();
+ }
+
+ private Result createEmptyChange() throws Exception {
+ PushOneCommit push =
+ pushFactory.create(admin.newIdent(), testRepo, "Test change", ImmutableMap.of());
+ return push.to("refs/for/master");
+ }
+}
