ChangeEdits: Propagate exception instead of swallowing it
We've seen broken pack files being inserted into the storage that
reference the null-sha in trees. We believe that swallowing
exceptions in change edit file modifications is causing this.
The code makes it seem like when we fail to write the new blob
to the storage, we don't stop there. Instead, we have instructed
the DirCache that it can expect a blob (with unset ObjectId) and
potentially a file mode. The transaction then goes through but
has corrupted the Git tree.
Release-Notes: Fix bug that allows inserting a broken pack
Change-Id: I8f8f0252c5d3cfe070b94332ca5a9ce6662bad6e
diff --git a/java/com/google/gerrit/server/edit/tree/ChangeFileContentModification.java b/java/com/google/gerrit/server/edit/tree/ChangeFileContentModification.java
index d722779..af0496a 100644
--- a/java/com/google/gerrit/server/edit/tree/ChangeFileContentModification.java
+++ b/java/com/google/gerrit/server/edit/tree/ChangeFileContentModification.java
@@ -21,7 +21,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
-import com.google.common.flogger.FluentLogger;
import com.google.common.io.ByteStreams;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.restapi.RawInput;
@@ -32,7 +31,6 @@
import java.util.List;
import org.eclipse.jgit.dircache.DirCacheEditor;
import org.eclipse.jgit.dircache.DirCacheEntry;
-import org.eclipse.jgit.errors.InvalidObjectIdException;
import org.eclipse.jgit.lib.FileMode;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
@@ -40,8 +38,6 @@
/** A {@code TreeModification} which changes the content of a file. */
public class ChangeFileContentModification implements TreeModification {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
private final String filePath;
private final RawInput newContent;
private final Integer newGitFileMode;
@@ -119,15 +115,10 @@
ObjectId newBlobObjectId = createNewBlobAndGetItsId();
dirCacheEntry.setObjectId(newBlobObjectId);
}
- // Previously, these two exceptions were swallowed. To improve the
- // situation, we log them now. However, we should think of a better
- // approach.
} catch (IOException e) {
String message =
String.format("Could not change the content of %s", dirCacheEntry.getPathString());
- logger.atSevere().withCause(e).log("%s", message);
- } catch (InvalidObjectIdException e) {
- logger.atSevere().withCause(e).log("Invalid object id in submodule link");
+ throw new IllegalStateException(message, e);
}
}
