Adjust FileInfoJson to accept 1-based integers for parent
There was a discrepancy between parent representations in DiffOperations
and FileInfoJson: The former used to accept 1-based parents, while the
latter accepted 0-based parents. There was a programming bug in
FilesInCommitCollection, since this API accepts a value "0" for parent
to indicate the default or auto-merge commit (See documentation in [1]).
The bug happened because we passed `parent - 1` (=-1) to FileInfoJson
resulting in array out of bounds while resolving the parent commit.
In this change we fix this bug. We unify the parent representation in
DiffOperations and FileInfoJson such that they both accept a 1-based
representation for parents. Also adding javadoc to clarify this. We also
remove the @Nullable from DiffOperations and use a primitive integer
with 0 to represent the default/auto-merge. We update all callers to
pass 0 instead of null.
[1]
https://gerrit-review.googlesource.com/Documentation/rest-api-projects.html#list-files
Change-Id: Ic10d6e925b2b0e1db295997f5e28c10a085874a9
diff --git a/java/com/google/gerrit/server/CommentsUtil.java b/java/com/google/gerrit/server/CommentsUtil.java
index 344549e..b18f499 100644
--- a/java/com/google/gerrit/server/CommentsUtil.java
+++ b/java/com/google/gerrit/server/CommentsUtil.java
@@ -438,7 +438,7 @@
// unignore the test in PortedCommentsIT.
Map<String, FileDiffOutput> modifiedFiles =
diffOperations.listModifiedFilesAgainstParent(
- change.getProject(), patchset.commitId(), /* parentNum= */ null);
+ change.getProject(), patchset.commitId(), /* parentNum= */ 0);
return modifiedFiles.isEmpty()
? null
: modifiedFiles.values().iterator().next().oldCommitId();
diff --git a/java/com/google/gerrit/server/change/FileInfoJson.java b/java/com/google/gerrit/server/change/FileInfoJson.java
index ad6f9c7..ab557dc 100644
--- a/java/com/google/gerrit/server/change/FileInfoJson.java
+++ b/java/com/google/gerrit/server/change/FileInfoJson.java
@@ -46,7 +46,8 @@
*
* @param change a Gerrit change.
* @param objectId a commit SHA-1 identifying a patchset commit.
- * @param parentNum an integer identifying the parent number used for comparison.
+ * @param parentNum 1-based integer identifying the parent number used for comparison. If zero,
+ * the only parent will be used or the auto-merge if {@code newCommit} is a merge commit.
* @return a mapping of the file paths to their related diff information.
*/
default Map<String, FileInfo> getFileInfoMap(Change change, ObjectId objectId, int parentNum)
@@ -74,7 +75,8 @@
*
* @param project a project identifying a repository.
* @param objectId a commit SHA-1 identifying a patchset commit.
- * @param parentNum an integer identifying the parent number used for comparison.
+ * @param parentNum 1-based integer identifying the parent number used for comparison. If zero,
+ * the only parent will be used or the auto-merge if {@code newCommit} is a merge commit.
* @return a mapping of the file paths to their related diff information.
*/
Map<String, FileInfo> getFileInfoMap(Project.NameKey project, ObjectId objectId, int parentNum)
diff --git a/java/com/google/gerrit/server/change/FileInfoJsonNewImpl.java b/java/com/google/gerrit/server/change/FileInfoJsonNewImpl.java
index 1ca2c93..7277404 100644
--- a/java/com/google/gerrit/server/change/FileInfoJsonNewImpl.java
+++ b/java/com/google/gerrit/server/change/FileInfoJsonNewImpl.java
@@ -47,8 +47,11 @@
throws ResourceConflictException, PatchListNotAvailableException {
try {
if (base == null) {
+ // Setting parentNum=0 requests the default parent, which is the only parent for
+ // single-parent commits, or the auto-merge otherwise
return asFileInfo(
- diffs.listModifiedFilesAgainstParent(change.getProject(), objectId, null));
+ diffs.listModifiedFilesAgainstParent(
+ change.getProject(), objectId, /* parentNum= */ 0));
}
return asFileInfo(diffs.listModifiedFiles(change.getProject(), base.commitId(), objectId));
} catch (DiffNotAvailableException e) {
@@ -63,7 +66,7 @@
throws ResourceConflictException, PatchListNotAvailableException {
try {
Map<String, FileDiffOutput> modifiedFiles =
- diffs.listModifiedFilesAgainstParent(project, objectId, parent + 1);
+ diffs.listModifiedFilesAgainstParent(project, objectId, parent);
return asFileInfo(modifiedFiles);
} catch (DiffNotAvailableException e) {
convertException(e);
diff --git a/java/com/google/gerrit/server/change/FileInfoJsonOldImpl.java b/java/com/google/gerrit/server/change/FileInfoJsonOldImpl.java
index 55d162a..0570296 100644
--- a/java/com/google/gerrit/server/change/FileInfoJsonOldImpl.java
+++ b/java/com/google/gerrit/server/change/FileInfoJsonOldImpl.java
@@ -60,10 +60,10 @@
Project.NameKey project, ObjectId objectId, int parentNum)
throws ResourceConflictException, PatchListNotAvailableException {
PatchListKey key =
- parentNum == -1
+ parentNum == 0
? PatchListKey.againstDefaultBase(objectId, Whitespace.IGNORE_NONE)
: PatchListKey.againstParentNum(
- parentNum + 1, objectId, DiffPreferencesInfo.Whitespace.IGNORE_NONE);
+ parentNum, objectId, DiffPreferencesInfo.Whitespace.IGNORE_NONE);
return toFileInfoMap(project, key);
}
diff --git a/java/com/google/gerrit/server/events/EventFactory.java b/java/com/google/gerrit/server/events/EventFactory.java
index 3a4dcff..3f988a3 100644
--- a/java/com/google/gerrit/server/events/EventFactory.java
+++ b/java/com/google/gerrit/server/events/EventFactory.java
@@ -399,7 +399,7 @@
try {
Map<String, FileDiffOutput> modifiedFiles =
diffOperations.listModifiedFilesAgainstParent(
- change.getProject(), patchSet.commitId(), /* parent= */ null);
+ change.getProject(), patchSet.commitId(), /* parent= */ 0);
for (FileDiffOutput diff : modifiedFiles.values()) {
if (patchSetAttribute.files == null) {
@@ -456,7 +456,7 @@
Map<String, FileDiffOutput> modifiedFiles =
diffOperations.listModifiedFilesAgainstParent(
- change.getProject(), patchSet.commitId(), /* parent= */ null);
+ change.getProject(), patchSet.commitId(), /* parent= */ 0);
for (FileDiffOutput fileDiff : modifiedFiles.values()) {
p.sizeDeletions += fileDiff.deletions();
p.sizeInsertions += fileDiff.insertions();
diff --git a/java/com/google/gerrit/server/patch/DiffOperations.java b/java/com/google/gerrit/server/patch/DiffOperations.java
index 7213581..d2da736 100644
--- a/java/com/google/gerrit/server/patch/DiffOperations.java
+++ b/java/com/google/gerrit/server/patch/DiffOperations.java
@@ -46,8 +46,8 @@
*
* @param project a project name representing a git repository.
* @param newCommit 20 bytes SHA-1 of the new commit used in the diff.
- * @param parentNum integer specifying which parent to use as base. If null, the only parent will
- * be used or the auto-merge if {@code newCommit} is a merge commit.
+ * @param parentNum 1-based integer specifying which parent to use as base. If zero, the only
+ * parent will be used or the auto-merge if {@code newCommit} is a merge commit.
* @return map of file paths to the file diffs. The map key is the new file path for all {@link
* ChangeType} file diffs except {@link ChangeType#DELETED} entries where the map key contains
* the old file path. The map entries are not sorted by key.
@@ -56,8 +56,7 @@
* an internal error occurred in Git while evaluating the diff.
*/
Map<String, FileDiffOutput> listModifiedFilesAgainstParent(
- Project.NameKey project, ObjectId newCommit, @Nullable Integer parentNum)
- throws DiffNotAvailableException;
+ Project.NameKey project, ObjectId newCommit, int parentNum) throws DiffNotAvailableException;
/**
* Returns the list of added, deleted or modified files between two commits (patchsets). The
@@ -85,8 +84,8 @@
*
* @param project a project name representing a git repository.
* @param newCommit 20 bytes SHA-1 of the new commit used in the diff.
- * @param parentNum integer specifying which parent to use as base. If null, the only parent will
- * be used or the auto-merge if {@code newCommit} is a merge commit.
+ * @param parentNum 1-based integer specifying which parent to use as base. If zero, the only
+ * parent will be used or the auto-merge if {@code newCommit} is a merge commit.
* @param fileName the file name for which the diff should be evaluated.
* @param whitespace preference controlling whitespace effect in diff computation.
* @return the diff for the single file between the two commits.
@@ -96,7 +95,7 @@
FileDiffOutput getModifiedFileAgainstParent(
Project.NameKey project,
ObjectId newCommit,
- @Nullable Integer parentNum,
+ int parentNum,
String fileName,
@Nullable DiffPreferencesInfo.Whitespace whitespace)
throws DiffNotAvailableException;
diff --git a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
index f500796..3423b32 100644
--- a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
+++ b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
@@ -91,8 +91,7 @@
@Override
public Map<String, FileDiffOutput> listModifiedFilesAgainstParent(
- Project.NameKey project, ObjectId newCommit, @Nullable Integer parent)
- throws DiffNotAvailableException {
+ Project.NameKey project, ObjectId newCommit, int parent) throws DiffNotAvailableException {
try {
DiffParameters diffParams = computeDiffParameters(project, newCommit, parent);
return getModifiedFiles(diffParams);
@@ -120,7 +119,7 @@
public FileDiffOutput getModifiedFileAgainstParent(
Project.NameKey project,
ObjectId newCommit,
- @Nullable Integer parent,
+ int parent,
String fileName,
@Nullable DiffPreferencesInfo.Whitespace whitespace)
throws DiffNotAvailableException {
@@ -376,7 +375,7 @@
Project.NameKey project, ObjectId newCommit, Integer parent) throws IOException {
DiffParameters.Builder result =
DiffParameters.builder().project(project).newCommit(newCommit).parent(parent);
- if (parent != null) {
+ if (parent > 0) {
result.baseCommit(baseCommitUtil.getBaseCommit(project, newCommit, parent));
result.comparisonType(ComparisonType.againstParent(parent));
return result.build();
diff --git a/java/com/google/gerrit/server/patch/PatchScriptFactory.java b/java/com/google/gerrit/server/patch/PatchScriptFactory.java
index 0c648b5..fbb6559 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptFactory.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptFactory.java
@@ -172,7 +172,7 @@
this.fileName = fileName;
this.psa = patchSetA;
- this.parentNum = -1;
+ this.parentNum = 0;
this.psb = patchSetB;
this.diffPrefs = diffPrefs;
this.currentUser = currentUser;
@@ -223,7 +223,7 @@
this.runNewDiffCache = cfg.getBoolean("cache", "diff_cache", "runNewDiffCache_GetDiff", false);
changeId = patchSetB.changeId();
- checkArgument(parentNum >= 0, "parentNum must be >= 0");
+ checkArgument(parentNum > 0, "parentNum must be > 0");
}
@Override
@@ -326,11 +326,7 @@
FileDiffOutput fileDiffOutput =
aId == null
? diffOperations.getModifiedFileAgainstParent(
- notes.getProjectName(),
- bId,
- parentNum == -1 ? null : parentNum + 1,
- fileName,
- diffPrefs.ignoreWhitespace)
+ notes.getProjectName(), bId, parentNum, fileName, diffPrefs.ignoreWhitespace)
: diffOperations.getModifiedFile(
notes.getProjectName(), aId, bId, fileName, diffPrefs.ignoreWhitespace);
return newBuilder().toPatchScriptNew(git, fileDiffOutput);
@@ -395,7 +391,7 @@
if (psa == null) {
return Optional.empty();
}
- checkState(parentNum < 0, "expected no parentNum when psa is present");
+ checkState(parentNum == 0, "expected no parentNum when psa is present");
checkArgument(psa.get() != 0, "edit not supported for left side");
return Optional.of(getCommitId(psa));
}
@@ -409,10 +405,10 @@
}
private PatchListKey keyFor(ObjectId aId, ObjectId bId, Whitespace whitespace) {
- if (parentNum < 0) {
+ if (parentNum == 0) {
return PatchListKey.againstCommit(aId, bId, whitespace);
}
- return PatchListKey.againstParentNum(parentNum + 1, bId, whitespace);
+ return PatchListKey.againstParentNum(parentNum, bId, whitespace);
}
private PatchList listFor(PatchListKey key) throws PatchListNotAvailableException {
diff --git a/java/com/google/gerrit/server/restapi/change/Files.java b/java/com/google/gerrit/server/restapi/change/Files.java
index 1efe378..320e57d 100644
--- a/java/com/google/gerrit/server/restapi/change/Files.java
+++ b/java/com/google/gerrit/server/restapi/change/Files.java
@@ -183,7 +183,7 @@
r =
Response.ok(
fileInfoJson.getFileInfoMap(
- resource.getChange(), resource.getPatchSet().commitId(), parentNum - 1));
+ resource.getChange(), resource.getPatchSet().commitId(), parentNum));
} else {
r = Response.ok(fileInfoJson.getFileInfoMap(resource.getChange(), resource.getPatchSet()));
}
@@ -280,11 +280,11 @@
Map<String, FileDiffOutput> oldList =
diffOperations.listModifiedFilesAgainstParent(
- project, patchSet.commitId(), /* parentNum= */ null);
+ project, patchSet.commitId(), /* parentNum= */ 0);
Map<String, FileDiffOutput> curList =
diffOperations.listModifiedFilesAgainstParent(
- project, resource.getPatchSet().commitId(), /* parentNum= */ null);
+ project, resource.getPatchSet().commitId(), /* parentNum= */ 0);
int sz = paths.size();
List<String> pathList = Lists.newArrayListWithCapacity(sz);
diff --git a/java/com/google/gerrit/server/restapi/change/GetDiff.java b/java/com/google/gerrit/server/restapi/change/GetDiff.java
index 2169d57..dd951a8 100644
--- a/java/com/google/gerrit/server/restapi/change/GetDiff.java
+++ b/java/com/google/gerrit/server/restapi/change/GetDiff.java
@@ -74,6 +74,7 @@
@Option(name = "--base", metaVar = "REVISION")
String base;
+ /** 1-based index of the parent's position in the commit object. */
@Option(name = "--parent", metaVar = "parent-number")
int parentNum;
@@ -143,7 +144,7 @@
} else if (parentNum > 0) {
psf =
patchScriptFactoryFactory.create(
- notes, fileName, parentNum - 1, pId, prefs, currentUser.get());
+ notes, fileName, parentNum, pId, prefs, currentUser.get());
} else {
psf = patchScriptFactoryFactory.create(notes, fileName, null, pId, prefs, currentUser.get());
}
diff --git a/java/com/google/gerrit/server/restapi/project/FilesInCommitCollection.java b/java/com/google/gerrit/server/restapi/project/FilesInCommitCollection.java
index 7bee2f2..6d054bd 100644
--- a/java/com/google/gerrit/server/restapi/project/FilesInCommitCollection.java
+++ b/java/com/google/gerrit/server/restapi/project/FilesInCommitCollection.java
@@ -77,6 +77,10 @@
}
public static final class ListFiles implements RestReadView<CommitResource> {
+ /**
+ * The 1-based parent number. If zero, the default base commit will be used, which is the only
+ * parent for commits having one parent or the auto-merge commit otherwise.
+ */
@Option(name = "--parent", metaVar = "parent-number")
int parentNum;
@@ -97,8 +101,7 @@
throws ResourceConflictException, PatchListNotAvailableException {
RevCommit commit = resource.getCommit();
return Response.ok(
- fileInfoJson.getFileInfoMap(
- resource.getProjectState().getNameKey(), commit, parentNum - 1));
+ fileInfoJson.getFileInfoMap(resource.getProjectState().getNameKey(), commit, parentNum));
}
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
index d8dab33..a01b340 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
@@ -153,7 +153,8 @@
PushOneCommit.Result result = push.to("refs/heads/master");
Map<String, FileDiffOutput> modifiedFiles =
- diffOperations.listModifiedFilesAgainstParent(project, result.getCommit(), null);
+ diffOperations.listModifiedFilesAgainstParent(
+ project, result.getCommit(), /* parentNum= */ 0);
assertThat(modifiedFiles.keySet()).containsExactly("/COMMIT_MSG", "f.txt");
assertThat(
diff --git a/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java b/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java
index 5bf5154..aa313e3 100644
--- a/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java
+++ b/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java
@@ -72,7 +72,7 @@
FileDiffOutput diffOutput =
diffOperations.getModifiedFileAgainstParent(
- testProjectName, newCommitId, /* parentNum=*/ null, fileName2, /* whitespace=*/ null);
+ testProjectName, newCommitId, /* parentNum=*/ 0, fileName2, /* whitespace=*/ null);
assertThat(diffOutput.oldCommitId()).isEqualTo(oldCommitId);
assertThat(diffOutput.newCommitId()).isEqualTo(newCommitId);
