Diff against root commits: use ObjectId#zeroId instead of EMPTY_TREE_ID
We used to reserve the special EMPTY_TREE_ID
4b825dc642cb6eb9a060e54bf8d69288fbee4904 for the
FileDiffCacheKey#oldCommit and the GitFileDiffCacheKey#oldTree. We also
use the same thing for the ModifiedFilesCache and GitModifiedFilesCache.
Callers pass this value to request the diff for a root commit. The cache
loader used to pass this value further to DiffFormatter#scan (in JGit)
to perform the scan.
Some repositories may not have the EMPTY_TREE_ID as a valid git object.
This resulted in a MissingObjectException in DiffFormatter#scan.
Here we change the following:
1) Use ObjectId#zeroId instead of EMPTY_TREE_ID in the cache keys to
signal requesting a diff against an added tree (i.e. root commit).
2) Change the handling in the loader by passing a null value to the old
tree parameter in DiffFormatter#scan. The method documents accepting
null values to perform this kind of diff. Check [1].
[1] https://git.eclipse.org/r/plugins/gitiles/jgit/jgit/+/f598e69529e0a1864e8224265ed82326f2a296f5/org.eclipse.jgit/src/org/eclipse/jgit/diff/DiffFormatter.java#426
Change-Id: I004dcf991f600052054aa167f24a925f1994eab3
diff --git a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
index fd30868..01a577a 100644
--- a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
+++ b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
@@ -16,7 +16,6 @@
import static com.google.gerrit.entities.Patch.COMMIT_MSG;
import static com.google.gerrit.entities.Patch.MERGE_LIST;
-import static org.eclipse.jgit.lib.Constants.EMPTY_TREE_ID;
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
@@ -346,7 +345,7 @@
/**
* Base commit represents the old commit of the diff. For diffs against the root commit, this
- * should be set to {@code EMPTY_TREE_ID}.
+ * should be set to {@link ObjectId#zeroId()}.
*/
abstract ObjectId baseCommit();
@@ -394,7 +393,7 @@
}
int numParents = baseCommitUtil.getNumParents(project, newCommit);
if (numParents == 0) {
- result.baseCommit(EMPTY_TREE_ID);
+ result.baseCommit(ObjectId.zeroId());
result.comparisonType(ComparisonType.againstRoot());
return result.build();
}
diff --git a/java/com/google/gerrit/server/patch/diff/ModifiedFilesCache.java b/java/com/google/gerrit/server/patch/diff/ModifiedFilesCache.java
index bcae238..56f49c9 100644
--- a/java/com/google/gerrit/server/patch/diff/ModifiedFilesCache.java
+++ b/java/com/google/gerrit/server/patch/diff/ModifiedFilesCache.java
@@ -28,8 +28,8 @@
* files.
*
* <p>If the {@link ModifiedFilesCacheKey#aCommit()} is equal to {@link
- * org.eclipse.jgit.lib.Constants#EMPTY_TREE_ID}, the diff will be evaluated against the empty tree,
- * and the result will be exactly the same as the caller can get from {@link
+ * org.eclipse.jgit.lib.ObjectId#zeroId()}, the diff will be evaluated against the empty tree, and
+ * the result will be exactly the same as the caller can get from {@link
* GitModifiedFilesCache#get(GitModifiedFilesCacheKey)}
*/
public interface ModifiedFilesCache {
diff --git a/java/com/google/gerrit/server/patch/diff/ModifiedFilesCacheImpl.java b/java/com/google/gerrit/server/patch/diff/ModifiedFilesCacheImpl.java
index e168386..b779bf7 100644
--- a/java/com/google/gerrit/server/patch/diff/ModifiedFilesCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/diff/ModifiedFilesCacheImpl.java
@@ -15,7 +15,6 @@
package com.google.gerrit.server.patch.diff;
import static com.google.common.collect.ImmutableList.toImmutableList;
-import static org.eclipse.jgit.lib.Constants.EMPTY_TREE_ID;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
@@ -52,10 +51,9 @@
* <p>The loader of this cache wraps a {@link GitModifiedFilesCache} to retrieve the git modified
* files.
*
- * <p>If the {@link ModifiedFilesCacheKey#aCommit()} is equal to {@link
- * org.eclipse.jgit.lib.Constants#EMPTY_TREE_ID}, the diff will be evaluated against the empty tree,
- * and the result will be exactly the same as the caller can get from {@link
- * GitModifiedFilesCache#get(GitModifiedFilesCacheKey)}
+ * <p>If the {@link ModifiedFilesCacheKey#aCommit()} is equal to {@link ObjectId#zeroId()}, the diff
+ * will be evaluated against the empty tree, and the result will be exactly the same as the caller
+ * can get from {@link GitModifiedFilesCache#get(GitModifiedFilesCacheKey)}
*/
@Singleton
public class ModifiedFilesCacheImpl implements ModifiedFilesCache {
@@ -130,7 +128,7 @@
private ImmutableList<ModifiedFile> loadModifiedFiles(ModifiedFilesCacheKey key, RevWalk rw)
throws IOException, DiffNotAvailableException {
ObjectId aTree =
- key.aCommit().equals(EMPTY_TREE_ID)
+ key.aCommit().equals(ObjectId.zeroId())
? key.aCommit()
: DiffUtil.getTreeId(rw, key.aCommit());
ObjectId bTree = DiffUtil.getTreeId(rw, key.bCommit());
@@ -142,7 +140,7 @@
.renameScore(key.renameScore())
.build();
List<ModifiedFile> modifiedFiles = gitCache.get(gitKey);
- if (key.aCommit().equals(EMPTY_TREE_ID)) {
+ if (key.aCommit().equals(ObjectId.zeroId())) {
return ImmutableList.copyOf(modifiedFiles);
}
RevCommit revCommitA = DiffUtil.getRevCommit(rw, key.aCommit());
diff --git a/java/com/google/gerrit/server/patch/filediff/AllDiffsEvaluator.java b/java/com/google/gerrit/server/patch/filediff/AllDiffsEvaluator.java
index 12decc3..9afa1e0 100644
--- a/java/com/google/gerrit/server/patch/filediff/AllDiffsEvaluator.java
+++ b/java/com/google/gerrit/server/patch/filediff/AllDiffsEvaluator.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server.patch.filediff;
-import static org.eclipse.jgit.lib.Constants.EMPTY_TREE_ID;
-
import com.google.common.collect.ImmutableMap;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.server.patch.DiffNotAvailableException;
@@ -203,7 +201,7 @@
FileDiffCacheKey key, ObjectId aCommit, ObjectId bCommit, String pathNew, RevWalk rw)
throws IOException {
ObjectId oldTreeId =
- aCommit.equals(EMPTY_TREE_ID) ? EMPTY_TREE_ID : DiffUtil.getTreeId(rw, aCommit);
+ aCommit.equals(ObjectId.zeroId()) ? ObjectId.zeroId() : DiffUtil.getTreeId(rw, aCommit);
ObjectId newTreeId = DiffUtil.getTreeId(rw, bCommit);
return GitFileDiffCacheKey.builder()
.project(key.project())
diff --git a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
index ee5e156..c05aae0 100644
--- a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
@@ -16,7 +16,6 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static java.nio.charset.StandardCharsets.UTF_8;
-import static org.eclipse.jgit.lib.Constants.EMPTY_TREE_ID;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
@@ -76,9 +75,8 @@
* Cache for the single file diff between two commits for a single file path. This cache adds extra
* Gerrit logic such as identifying edits due to rebase.
*
- * <p>If the {@link FileDiffCacheKey#oldCommit()} is equal to {@link
- * org.eclipse.jgit.lib.Constants#EMPTY_TREE_ID}, the git diff will be evaluated against the empty
- * tree.
+ * <p>If the {@link FileDiffCacheKey#oldCommit()} is equal to {@link ObjectId#zeroId()}, the git
+ * diff will be evaluated against the empty tree.
*/
@Singleton
public class FileDiffCacheImpl implements FileDiffCache {
@@ -204,7 +202,7 @@
private ComparisonType getComparisonType(
RevWalk rw, ObjectReader reader, ObjectId oldCommitId, ObjectId newCommitId)
throws IOException {
- if (oldCommitId.equals(EMPTY_TREE_ID)) {
+ if (oldCommitId.equals(ObjectId.zeroId())) {
return ComparisonType.againstRoot();
}
RevCommit oldCommit = DiffUtil.getRevCommit(rw, oldCommitId);
@@ -238,7 +236,7 @@
ComparisonType comparisonType =
getComparisonType(rw, reader, key.oldCommit(), key.newCommit());
RevCommit aCommit =
- key.oldCommit().equals(EMPTY_TREE_ID)
+ key.oldCommit().equals(ObjectId.zeroId())
? null
: DiffUtil.getRevCommit(rw, key.oldCommit());
RevCommit bCommit = DiffUtil.getRevCommit(rw, key.newCommit());
@@ -345,7 +343,7 @@
FileHeader fileHeader = new FileHeader(rawHdr, edits, PatchType.UNIFIED);
Patch.ChangeType changeType = FileHeaderUtil.getChangeType(fileHeader);
return FileDiffOutput.builder()
- .oldCommitId(oldCommit == null ? EMPTY_TREE_ID : oldCommit)
+ .oldCommitId(oldCommit == null ? ObjectId.zeroId() : oldCommit)
.newCommitId(newCommit)
.comparisonType(comparisonType)
.oldPath(FileHeaderUtil.getOldPath(fileHeader))
@@ -399,12 +397,14 @@
}
List<Edit> rebaseEdits = rebaseFileEdits.edits();
- RevTree aTree = rw.parseTree(allDiffs.mainDiff().gitKey().oldTree());
+ ObjectId oldTreeId = allDiffs.mainDiff().gitKey().oldTree();
+
+ RevTree aTree = oldTreeId.equals(ObjectId.zeroId()) ? null : rw.parseTree(oldTreeId);
RevTree bTree = rw.parseTree(allDiffs.mainDiff().gitKey().newTree());
GitFileDiff mainGitDiff = allDiffs.mainDiff().gitDiff();
Long oldSize =
- mainGitDiff.oldMode().isPresent() && mainGitDiff.oldPath().isPresent()
+ aTree != null && mainGitDiff.oldMode().isPresent() && mainGitDiff.oldPath().isPresent()
? new FileSizeEvaluator(reader, aTree)
.compute(
mainGitDiff.oldId(),
@@ -453,7 +453,7 @@
private List<AugmentedFileDiffCacheKey> wrapKeys(List<FileDiffCacheKey> keys, RevWalk rw) {
List<AugmentedFileDiffCacheKey> result = new ArrayList<>();
for (FileDiffCacheKey key : keys) {
- if (key.oldCommit().equals(EMPTY_TREE_ID)) {
+ if (key.oldCommit().equals(ObjectId.zeroId())) {
result.add(AugmentedFileDiffCacheKey.builder().key(key).ignoreRebase(true).build());
continue;
}
diff --git a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheKey.java b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheKey.java
index 7ac9343..1c0e141 100644
--- a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheKey.java
+++ b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheKey.java
@@ -26,7 +26,6 @@
import com.google.gerrit.server.cache.serialize.CacheSerializer;
import com.google.gerrit.server.cache.serialize.ObjectIdConverter;
import com.google.gerrit.server.patch.gitfilediff.GitFileDiffCacheImpl.DiffAlgorithm;
-import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
/** Cache key for the {@link FileDiffCache}. */
@@ -38,7 +37,7 @@
/**
* The 20 bytes SHA-1 commit ID of the old commit used in the diff. If set to {@link
- * Constants#EMPTY_TREE_ID}, the diff is performed against the root commit.
+ * ObjectId#zeroId()}, an empty tree is used for the diff.
*/
public abstract ObjectId oldCommit();
diff --git a/java/com/google/gerrit/server/patch/gitdiff/GitModifiedFilesCacheImpl.java b/java/com/google/gerrit/server/patch/gitdiff/GitModifiedFilesCacheImpl.java
index 9b96da1..b7144d7 100644
--- a/java/com/google/gerrit/server/patch/gitdiff/GitModifiedFilesCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/gitdiff/GitModifiedFilesCacheImpl.java
@@ -39,6 +39,7 @@
import org.eclipse.jgit.diff.DiffEntry;
import org.eclipse.jgit.diff.DiffEntry.ChangeType;
import org.eclipse.jgit.diff.DiffFormatter;
+import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.util.io.DisabledOutputStream;
@@ -132,7 +133,7 @@
}
// The scan method only returns the file paths that are different. Callers may choose to
// format these paths themselves.
- return df.scan(key.aTree(), key.bTree());
+ return df.scan(key.aTree().equals(ObjectId.zeroId()) ? null : key.aTree(), key.bTree());
}
}
diff --git a/java/com/google/gerrit/server/patch/gitdiff/GitModifiedFilesCacheKey.java b/java/com/google/gerrit/server/patch/gitdiff/GitModifiedFilesCacheKey.java
index fb8fce1..16b0e65 100644
--- a/java/com/google/gerrit/server/patch/gitdiff/GitModifiedFilesCacheKey.java
+++ b/java/com/google/gerrit/server/patch/gitdiff/GitModifiedFilesCacheKey.java
@@ -37,7 +37,8 @@
/**
* The git SHA-1 {@link ObjectId} of the first git tree object for which the diff should be
- * computed.
+ * computed. If equals to {@link ObjectId#zeroId()}, a null tree is used for the diff scan, and
+ * {@link #bTree()} is treated as an added tree.
*/
public abstract ObjectId aTree();
diff --git a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
index 31afd17..761910f 100644
--- a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
@@ -219,7 +219,9 @@
throws IOException {
Set<String> filePathsSet = ImmutableSet.copyOf(filePaths);
List<DiffEntry> diffEntries =
- diffFormatter.scan(diffOptions.oldTree(), diffOptions.newTree());
+ diffFormatter.scan(
+ diffOptions.oldTree().equals(ObjectId.zeroId()) ? null : diffOptions.oldTree(),
+ diffOptions.newTree());
return diffEntries.stream()
.filter(d -> filePathsSet.contains(pathExtractor.apply(d)))
diff --git a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheKey.java b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheKey.java
index f104388..4fc5279 100644
--- a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheKey.java
+++ b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheKey.java
@@ -34,7 +34,11 @@
/** A specific git project / repository. */
public abstract Project.NameKey project();
- /** The old 20 bytes SHA-1 git tree ID used in the git tree diff */
+ /**
+ * The old 20 bytes SHA-1 git tree ID used in the git tree diff. If equals to {@link
+ * ObjectId#zeroId()}, a null tree is used for the diff scan, and {@link #newTree()} ()} is
+ * treated as an added tree.
+ */
public abstract ObjectId oldTree();
/** The new 20 bytes SHA-1 git tree ID used in the git tree diff */
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
index 1a55b82..d8dab33 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
@@ -26,7 +26,6 @@
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toMap;
-import static org.eclipse.jgit.lib.Constants.EMPTY_TREE_ID;
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
@@ -161,7 +160,7 @@
modifiedFiles.values().stream()
.map(FileDiffOutput::oldCommitId)
.collect(Collectors.toSet()))
- .containsExactly(EMPTY_TREE_ID);
+ .containsExactly(ObjectId.zeroId());
assertThat(modifiedFiles.get("/COMMIT_MSG").changeType()).isEqualTo(Patch.ChangeType.ADDED);
assertThat(modifiedFiles.get("f.txt").changeType()).isEqualTo(Patch.ChangeType.ADDED);
}
