GitFileDiffCacheImp: Improve logging for MissingObjectExceptions
We recently saw MissingObjectExceptions when trying to get the file
header for a diff entry, but we were not able to tell from the
stacktrace for which path the object was missing and whether it was the
old or the new path.
Example stacktrace:
...
Caused by: org.eclipse.jgit.errors.MissingObjectException: Missing blob 3c89d305edb38258665b9ffca5ca08d79d3953a8
at org.eclipse.jgit.internal.storage.dfs.DfsReader.open(DfsReader.java:225)
at com.google.devtools.gerritcodereview.git.storage.GoogleObjectReader.open(GoogleObjectReader.java:248)
at org.eclipse.jgit.diff.ContentSource$ObjectReaderSource.open(ContentSource.java:135)
at org.eclipse.jgit.diff.ContentSource$Pair.open(ContentSource.java:300)
at org.eclipse.jgit.diff.DiffFormatter.open(DiffFormatter.java:1075)
at org.eclipse.jgit.diff.DiffFormatter.createFormatResult(DiffFormatter.java:1002)
at org.eclipse.jgit.diff.DiffFormatter.toFileHeader(DiffFormatter.java:965)
at com.google.gerrit.server.patch.gitfilediff.GitFileDiffCacheImpl$Loader.lambda$createGitFileDiff$2(GitFileDiffCacheImpl.java:356)
...
Bug: Google b/263077919
Signed-off-by: Edwin Kempin <ekempin@google.com>
Release-Notes: skip
Change-Id: Ia810b47264b6a3efb9e3bd46c7e57f76165b4590
diff --git a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
index 2b856fb..43a212c 100644
--- a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
@@ -28,6 +28,7 @@
import com.google.common.collect.MultimapBuilder;
import com.google.common.collect.Multimaps;
import com.google.common.collect.Streams;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
@@ -66,6 +67,7 @@
import org.eclipse.jgit.diff.DiffFormatter;
import org.eclipse.jgit.diff.HistogramDiff;
import org.eclipse.jgit.diff.RawTextComparator;
+import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.AbbreviatedObjectId;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
@@ -76,6 +78,7 @@
/** Implementation of the {@link GitFileDiffCache} */
@Singleton
public class GitFileDiffCacheImpl implements GitFileDiffCache {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private static final String GIT_DIFF = "git_file_diff";
public static Module module() {
@@ -340,8 +343,7 @@
throws IOException {
if (!key.useTimeout()) {
try (CloseablePool<DiffFormatter>.Handle formatter = diffPool.get()) {
- FileHeader fileHeader = formatter.get().toFileHeader(diffEntry);
- return GitFileDiff.create(diffEntry, fileHeader);
+ return GitFileDiff.create(diffEntry, getFileHeader(formatter, diffEntry));
}
}
// This submits the DiffFormatter to a different thread. The CloseablePool and our usage of it
@@ -353,7 +355,7 @@
diffExecutor.submit(
() -> {
try (CloseablePool<DiffFormatter>.Handle formatter = diffPool.get()) {
- return GitFileDiff.create(diffEntry, formatter.get().toFileHeader(diffEntry));
+ return GitFileDiff.create(diffEntry, getFileHeader(formatter, diffEntry));
}
});
try {
@@ -385,6 +387,46 @@
? diffEntry.getOldPath()
: diffEntry.getNewPath();
}
+
+ private FileHeader getFileHeader(
+ CloseablePool<DiffFormatter>.Handle formatter, DiffEntry diffEntry) throws IOException {
+ logger.atFine().log("getting file header for %s", formatDiffEntryForLogging(diffEntry));
+ try {
+ return formatter.get().toFileHeader(diffEntry);
+ } catch (MissingObjectException e) {
+ throw new IOException(
+ String.format("Failed to get file header for %s", formatDiffEntryForLogging(diffEntry)),
+ e);
+ }
+ }
+
+ private String formatDiffEntryForLogging(DiffEntry diffEntry) {
+ StringBuilder buf = new StringBuilder();
+ buf.append("DiffEntry[");
+ buf.append(diffEntry.getChangeType());
+ buf.append(" ");
+ switch (diffEntry.getChangeType()) {
+ case ADD:
+ buf.append(String.format("%s (%s)", diffEntry.getNewPath(), diffEntry.getNewId().name()));
+ break;
+ case COPY:
+ case RENAME:
+ buf.append(
+ String.format(
+ "%s (%s) -> %s (%s)",
+ diffEntry.getOldPath(),
+ diffEntry.getOldId().name(),
+ diffEntry.getNewPath(),
+ diffEntry.getNewId().name()));
+ break;
+ case DELETE:
+ case MODIFY:
+ buf.append(String.format("%s (%s)", diffEntry.getOldPath(), diffEntry.getOldId().name()));
+ break;
+ }
+ buf.append("]");
+ return buf.toString();
+ }
}
/**
