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

Documentation/rest-api-projects.txt

@@ -2704,7 +2704,10 @@ sorted by file path.
 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

java/com/google/gerrit/extensions/api/projects/CommitApi.java

@@ -18,8 +18,10 @@ import com.google.gerrit.extensions.api.changes.ChangeApi;
 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 @@ public interface CommitApi {
 
   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 interface CommitApi {
     public IncludedInInfo includedIn() throws RestApiException {
       throw new NotImplementedException();
     }
+
+    @Override
+    public Map<String, FileInfo> files(int parentNum) throws RestApiException {
+      throw new NotImplementedException();
+    }
   }
 }

java/com/google/gerrit/server/api/projects/CommitApiImpl.java

@@ -22,13 +22,16 @@ import com.google.gerrit.extensions.api.changes.CherryPickInput;
 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 @@ public class CommitApiImpl implements CommitApi {
   private final CherryPickCommit cherryPickCommit;
   private final CommitIncludedIn includedIn;
   private final CommitResource commitResource;
+  private final FilesInCommitCollection.ListFiles listFiles;
 
   @Inject
   CommitApiImpl(
@@ -47,11 +51,13 @@ public class CommitApiImpl implements CommitApi {
       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 @@ public class CommitApiImpl implements CommitApi {
       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);
+    }
+  }
 }

java/com/google/gerrit/server/change/FileInfoJson.java

@@ -69,7 +69,8 @@ public interface FileInfoJson {
   /**
    * 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.

java/com/google/gerrit/server/change/FileInfoJsonOldImpl.java

@@ -60,8 +60,10 @@ class FileInfoJsonOldImpl implements FileInfoJson {
       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);
   }
 

java/com/google/gerrit/server/restapi/project/FilesInCommitCollection.java

@@ -87,6 +87,11 @@ public class FilesInCommitCollection implements ChildCollection<CommitResource,
       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 {

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");
+  }
+}