Do not fail when listing files for initial commit against the first parent
DiffOperations used an empty *tree* as the base *commit* when getting
the diff for an initial commit and the user specified comparing against
the first parent (which doesn't exist). In this case the request failed
with 500 ISE because the empty tree is not a commit and thus we run into
an IncorrectObjectTypeException.
Instead of failing in this case we are now falling back to comparing
against the default root.
This also makes the code more readable as it was quite confusing that
the methods getParentCommit and getBaseCommit in BaseCommitUtil were
actually not guaranteed to return a commit, but could return a Git tree
instead. It seems that none of the callers was expecting and handling
this. Now we return null as the base/parent commit if the commit doesn't
have a parent. Null was already returned before when the commit had more
than 2 parents, so returning null in the new case is consistent with
that. FWIW null is not properly handled by all callers yet, but it is at
least handled by the one caller now that is relevant for the use case
that we are fixing here.
The old code also used the inserter that was supposed for creating
auto-merge commits only to insert the empty tree into the repository. It
seems this was not intended and it's good if we can get rid of this now.
At the moment at Google this issue happens very frequently and these
errors are counted against our SLO budget, potentially triggering SLO
violation alerts.
Release-Notes: Fixed listing files for initial commit when user specified to compare against the first parent which doesn't exist
Change-Id: I37edf7c394ef9472a641270798612652c6586c8c
Signed-off-by: Edwin Kempin <ekempin@google.com>
Bug: Google b/284443167
diff --git a/java/com/google/gerrit/server/patch/BaseCommitUtil.java b/java/com/google/gerrit/server/patch/BaseCommitUtil.java
index 9a103cd..a264793 100644
--- a/java/com/google/gerrit/server/patch/BaseCommitUtil.java
+++ b/java/com/google/gerrit/server/patch/BaseCommitUtil.java
@@ -26,13 +26,11 @@
import java.io.IOException;
import java.util.Optional;
import org.eclipse.jgit.lib.Config;
-import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
-import org.eclipse.jgit.revwalk.RevObject;
import org.eclipse.jgit.revwalk.RevWalk;
/** A utility class for computing the base commit / parent for a specific patchset commit. */
@@ -51,7 +49,8 @@
this.repoManager = repoManager;
}
- RevObject getBaseCommit(Project.NameKey project, ObjectId newCommit, @Nullable Integer parentNum)
+ @Nullable
+ RevCommit getBaseCommit(Project.NameKey project, ObjectId newCommit, @Nullable Integer parentNum)
throws IOException {
try (Repository repo = repoManager.openRepository(project);
ObjectInserter ins = newInserter(repo);
@@ -89,10 +88,12 @@
* commitId} has a single parent, it will be returned.
* @param commitId 20 bytes commitId SHA-1 hash.
* @return Returns the parent commit of the commit represented by the commitId parameter. Note
- * that auto-merge is not supported for commits having more than two parents.
+ * that auto-merge is not supported for commits having more than two parents. If the commit
+ * has no parents (initial commit) or more than 2 parents {@code null} is returned as the
+ * parent commit.
*/
@Nullable
- RevObject getParentCommit(
+ RevCommit getParentCommit(
Repository repo,
ObjectInserter ins,
RevWalk rw,
@@ -102,7 +103,7 @@
RevCommit current = rw.parseCommit(commitId);
switch (current.getParentCount()) {
case 0:
- return rw.parseAny(emptyTree(ins));
+ return null;
case 1:
return current.getParent(0);
default:
@@ -146,10 +147,4 @@
private ObjectInserter newInserter(Repository repo) {
return saveAutomerge ? repo.newObjectInserter() : new InMemoryInserter(repo);
}
-
- private static ObjectId emptyTree(ObjectInserter ins) throws IOException {
- ObjectId id = ins.insert(Constants.OBJ_TREE, new byte[] {});
- ins.flush();
- return id;
- }
}
diff --git a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
index 86f122e..2606d66 100644
--- a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
+++ b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
@@ -55,6 +55,7 @@
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.util.io.DisabledOutputStream;
@@ -488,7 +489,16 @@
DiffParameters.Builder result =
DiffParameters.builder().project(project).newCommit(newCommit).parent(parent);
if (parent > 0) {
- result.baseCommit(baseCommitUtil.getBaseCommit(project, newCommit, parent));
+ RevCommit baseCommit = baseCommitUtil.getBaseCommit(project, newCommit, parent);
+ if (baseCommit == null) {
+ // The specified parent doesn't exist or is not supported, fall back to comparing against
+ // the root.
+ result.baseCommit(ObjectId.zeroId());
+ result.comparisonType(ComparisonType.againstRoot());
+ return result.build();
+ }
+
+ result.baseCommit(baseCommit);
result.comparisonType(ComparisonType.againstParent(parent));
return result.build();
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/ListCommitFilesIT.java b/javatests/com/google/gerrit/acceptance/rest/project/ListCommitFilesIT.java
index 3ac2d10..5f02af1 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/ListCommitFilesIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/ListCommitFilesIT.java
@@ -15,17 +15,21 @@
package com.google.gerrit.acceptance.rest.project;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.GitUtil.assertPushOk;
import static com.google.gerrit.acceptance.GitUtil.getChangeId;
import static com.google.gerrit.acceptance.GitUtil.pushHead;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
+import com.google.gerrit.acceptance.TestProjectInput;
+import com.google.gerrit.entities.Patch;
import com.google.gerrit.extensions.common.FileInfo;
import com.google.gson.reflect.TypeToken;
import java.lang.reflect.Type;
import java.util.Map;
import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.transport.PushResult;
import org.junit.Test;
public class ListCommitFilesIT extends AbstractDaemonTest {
@@ -87,4 +91,31 @@
assertThat(files1).isEqualTo(files2);
}
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void listFilesOfInitialCommitAgainstFirstParent() throws Exception {
+ // create initial commit with no parent and push it directly to refs/heads/master
+ RevCommit c =
+ testRepo
+ .commit()
+ .message("Initial commit")
+ .add("a.txt", "aContent")
+ .add("b.txt", "bContent")
+ .create();
+ testRepo.reset(c);
+ PushResult r = pushHead(testRepo, "refs/heads/master", false);
+ assertPushOk(r, "refs/heads/master");
+
+ // Request diff against first parent although the initial commit doesn't have a parent
+ RestResponse response =
+ userRestSession.get(
+ "/projects/" + project.get() + "/commits/" + c.name() + "/files/?parent=1");
+ response.assertOK();
+ Type type = new TypeToken<Map<String, FileInfo>>() {}.getType();
+ Map<String, FileInfo> files = newGson().fromJson(response.getReader(), type);
+ response.consume();
+
+ assertThat(files.keySet()).containsExactly(Patch.COMMIT_MSG, "a.txt", "b.txt");
+ }
}
