Remove validate support when reusing cached pack

Cached packs are only used when writing over the network or to
a bundle file and reuse validation is always disabled in these
two contexts. The client/consumer of the stream will be SHA-1
checksumming every object.

Reuse validation is most critical during local GC to avoid silently
ignoring corruption by stopping as soon as a problem is found and
leaving everything alone for the end-user to debug and salvage.
Cached packs are not supported during local GC as the bitmap rebuild
logic does not support including a cached pack in the result.

Strip out the validation and force PackWriter to always disable the
cached pack feature if reuseValidation is enabled.

Change-Id: If0d7baf2ae1bf1f7e71bf773151302c9f7887039

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlock.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlock.java
index c1853327f55304a8b2920fade53c81fb3e979dda..79265363e05e0480966ff1d2bf3ff22cc09072d9 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlock.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlock.java
@@ -46,7 +46,6 @@
 package org.eclipse.jgit.internal.storage.dfs;
 
 import java.io.IOException;
-import java.security.MessageDigest;
 import java.util.zip.CRC32;
 import java.util.zip.DataFormatException;
 import java.util.zip.Inflater;
@@ -101,12 +100,9 @@ final class DfsBlock {
                out.update(block, ptr, cnt);
        }
 
-       void write(PackOutputStream out, long pos, int cnt, MessageDigest digest)
+       void write(PackOutputStream out, long pos, int cnt)
                        throws IOException {
-               int ptr = (int) (pos - start);
-               out.write(block, ptr, cnt);
-               if (digest != null)
-                       digest.update(block, ptr, cnt);
+               out.write(block, (int) (pos - start), cnt);
        }
 
        void check(Inflater inf, byte[] tmp, long pos, int cnt)

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsCachedPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsCachedPack.java
index 3da5184e08bb90309580c570f616591c60883143..a5308f617edae7378cee8e69d0325b79c469605c 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsCachedPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsCachedPack.java
@@ -78,8 +78,7 @@ public class DfsCachedPack extends CachedPack {
                return ((DfsObjectRepresentation) rep).pack == pack;
        }
 
-       void copyAsIs(PackOutputStream out, boolean validate, DfsReader ctx)
-                       throws IOException {
-               pack.copyPackAsIs(out, validate, ctx);
+       void copyAsIs(PackOutputStream out, DfsReader ctx) throws IOException {
+               pack.copyPackAsIs(out, ctx);
        }
 }

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java
index b51ee2f4decc29a36c8bc93ddee0c56db3af936a..e03488b3cf1641fb8c63b6e1f45f0165b19b8e8c 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java
@@ -56,9 +56,7 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.nio.ByteBuffer;
 import java.nio.channels.Channels;
-import java.security.MessageDigest;
 import java.text.MessageFormat;
-import java.util.Arrays;
 import java.util.Set;
 import java.util.zip.CRC32;
 import java.util.zip.DataFormatException;
@@ -466,55 +464,35 @@ public final class DfsPackFile {
                return dstbuf;
        }
 
-       void copyPackAsIs(PackOutputStream out, boolean validate, DfsReader ctx)
+       void copyPackAsIs(PackOutputStream out, DfsReader ctx)
                        throws IOException {
-               MessageDigest md = initCopyPack(out, validate, ctx);
-               long p;
-               if (cache.shouldCopyThroughCache(length))
-                       p = copyPackThroughCache(out, ctx, md);
-               else
-                       p = copyPackBypassCache(out, ctx, md);
-               verifyPackChecksum(p, md, ctx);
-       }
-
-       private MessageDigest initCopyPack(PackOutputStream out, boolean validate,
-                       DfsReader ctx) throws IOException {
                // If the length hasn't been determined yet, pin to set it.
-               if (length == -1)
+               if (length == -1) {
                        ctx.pin(this, 0);
-               if (!validate) {
                        ctx.unpin();
-                       return null;
                }
-
-               int hdrlen = 12;
-               byte[] buf = out.getCopyBuffer();
-               int n = ctx.copy(this, 0, buf, 0, hdrlen);
-               ctx.unpin();
-               if (n != hdrlen)
-                       throw packfileIsTruncated();
-               MessageDigest md = Constants.newMessageDigest();
-               md.update(buf, 0, hdrlen);
-               return md;
+               if (cache.shouldCopyThroughCache(length))
+                       copyPackThroughCache(out, ctx);
+               else
+                       copyPackBypassCache(out, ctx);
        }
 
-       private long copyPackThroughCache(PackOutputStream out, DfsReader ctx,
-                       MessageDigest md) throws IOException {
+       private void copyPackThroughCache(PackOutputStream out, DfsReader ctx)
+                       throws IOException {
                long position = 12;
                long remaining = length - (12 + 20);
                while (0 < remaining) {
                        DfsBlock b = cache.getOrLoad(this, position, ctx);
                        int ptr = (int) (position - b.start);
                        int n = (int) Math.min(b.size() - ptr, remaining);
-                       b.write(out, position, n, md);
+                       b.write(out, position, n);
                        position += n;
                        remaining -= n;
                }
-               return position;
        }
 
-       private long copyPackBypassCache(PackOutputStream out, DfsReader ctx,
-                       MessageDigest md) throws IOException {
+       private long copyPackBypassCache(PackOutputStream out, DfsReader ctx)
+                       throws IOException {
                try (ReadableChannel rc = ctx.db.openFile(packDesc, PACK)) {
                        ByteBuffer buf = newCopyBuffer(out, rc);
                        if (ctx.getOptions().getStreamPackBufferSize() > 0)
@@ -526,7 +504,7 @@ public final class DfsPackFile {
                                if (b != null) {
                                        int ptr = (int) (position - b.start);
                                        int n = (int) Math.min(b.size() - ptr, remaining);
-                                       b.write(out, position, n, md);
+                                       b.write(out, position, n);
                                        position += n;
                                        remaining -= n;
                                        rc.position(position);
@@ -540,8 +518,6 @@ public final class DfsPackFile {
                                else if (n > remaining)
                                        n = (int) remaining;
                                out.write(buf.array(), 0, n);
-                               if (md != null)
-                                       md.update(buf.array(), 0, n);
                                position += n;
                                remaining -= n;
                        }
@@ -557,22 +533,6 @@ public final class DfsPackFile {
                return ByteBuffer.wrap(copyBuf, 0, bs);
        }
 
-       private void verifyPackChecksum(long position, MessageDigest md,
-                       DfsReader ctx) throws IOException {
-               if (md != null) {
-                       byte[] buf = new byte[20];
-                       byte[] actHash = md.digest();
-                       if (ctx.copy(this, position, buf, 0, 20) != 20)
-                               throw packfileIsTruncated();
-                       if (!Arrays.equals(actHash, buf)) {
-                               invalid = true;
-                               throw new IOException(MessageFormat.format(
-                                               JGitText.get().packfileCorruptionDetected,
-                                               getPackName()));
-                       }
-               }
-       }
-
        void copyAsIs(PackOutputStream out, DfsObjectToPack src,
                        boolean validate, DfsReader ctx) throws IOException,
                        StoredObjectRepresentationNotAvailableException {
@@ -719,7 +679,7 @@ public final class DfsPackFile {
                        // and we have it pinned.  Write this out without copying.
                        //
                        out.writeHeader(src, inflatedLength);
-                       quickCopy.write(out, dataOffset, (int) dataLength, null);
+                       quickCopy.write(out, dataOffset, (int) dataLength);
 
                } else if (dataLength <= buf.length) {
                        // Tiny optimization: Lots of objects are very small deltas or

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java
index dc8552e49d99c22d9b0edc2ecfe382788e902952..f5f3375fa13d8ad20356ac449c1396127a2b28d0 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java
@@ -492,9 +492,9 @@ public final class DfsReader extends ObjectReader implements ObjectReuseAsIs {
                        out.writeObject(otp);
        }
 
-       public void copyPackAsIs(PackOutputStream out, CachedPack pack,
-                       boolean validate) throws IOException {
-               ((DfsCachedPack) pack).copyAsIs(out, validate, this);
+       public void copyPackAsIs(PackOutputStream out, CachedPack pack)
+                       throws IOException {
+               ((DfsCachedPack) pack).copyAsIs(out, this);
        }
 
        /**

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteArrayWindow.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteArrayWindow.java
index 863c553b3b6650b2463d3124f961d57f5aca7494..dc720bc62df18d63b0ccd6c4d20f0c992645cfa5 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteArrayWindow.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteArrayWindow.java
@@ -46,7 +46,6 @@
 package org.eclipse.jgit.internal.storage.file;
 
 import java.io.IOException;
-import java.security.MessageDigest;
 import java.util.zip.CRC32;
 import java.util.zip.DataFormatException;
 import java.util.zip.Inflater;
@@ -84,12 +83,10 @@ final class ByteArrayWindow extends ByteWindow {
        }
 
        @Override
-       void write(PackOutputStream out, long pos, int cnt, MessageDigest digest)
+       void write(PackOutputStream out, long pos, int cnt)
                        throws IOException {
                int ptr = (int) (pos - start);
                out.write(array, ptr, cnt);
-               if (digest != null)
-                       digest.update(array, ptr, cnt);
        }
 
        void check(Inflater inf, byte[] tmp, long pos, int cnt)

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteBufferWindow.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteBufferWindow.java
index 31925d28e8228cb5b7a6ddc884d22fb584b5a40d..05ddd69b369aac5c5601a9a1f862b8753ae0a68d 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteBufferWindow.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteBufferWindow.java
@@ -47,7 +47,6 @@ package org.eclipse.jgit.internal.storage.file;
 
 import java.io.IOException;
 import java.nio.ByteBuffer;
-import java.security.MessageDigest;
 import java.util.zip.DataFormatException;
 import java.util.zip.Inflater;
 
@@ -76,7 +75,7 @@ final class ByteBufferWindow extends ByteWindow {
        }
 
        @Override
-       void write(PackOutputStream out, long pos, int cnt, MessageDigest digest)
+       void write(PackOutputStream out, long pos, int cnt)
                        throws IOException {
                final ByteBuffer s = buffer.slice();
                s.position((int) (pos - start));
@@ -86,8 +85,6 @@ final class ByteBufferWindow extends ByteWindow {
                        int n = Math.min(cnt, buf.length);
                        s.get(buf, 0, n);
                        out.write(buf, 0, n);
-                       if (digest != null)
-                               digest.update(buf, 0, n);
                        cnt -= n;
                }
        }

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteWindow.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteWindow.java
index ab5eb7c9000e0b1327b8a1d04b4713abae27af7d..e774a14643ad68fc402f5aed7bb74c64f68ef2b3 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteWindow.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ByteWindow.java
@@ -45,7 +45,6 @@
 package org.eclipse.jgit.internal.storage.file;
 
 import java.io.IOException;
-import java.security.MessageDigest;
 import java.util.zip.DataFormatException;
 import java.util.zip.Inflater;
 
@@ -121,8 +120,8 @@ abstract class ByteWindow {
         */
        protected abstract int copy(int pos, byte[] dstbuf, int dstoff, int cnt);
 
-       abstract void write(PackOutputStream out, long pos, int cnt,
-                       MessageDigest md) throws IOException;
+       abstract void write(PackOutputStream out, long pos, int cnt)
+                       throws IOException;
 
        final int setInput(long pos, Inflater inf) throws DataFormatException {
                return setInput((int) (pos - start), inf);

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LocalCachedPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LocalCachedPack.java
index b70ebcf9ef3103f386dae50b634277a1aa4ab70f..fd9dcdafad78652a254ea6ff54ca70dd0180cb73 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LocalCachedPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LocalCachedPack.java
@@ -79,10 +79,10 @@ class LocalCachedPack extends CachedPack {
                return cnt;
        }
 
-       void copyAsIs(PackOutputStream out, boolean validate, WindowCursor wc)
+       void copyAsIs(PackOutputStream out, WindowCursor wc)
                        throws IOException {
                for (PackFile pack : getPacks())
-                       pack.copyPackAsIs(out, validate, wc);
+                       pack.copyPackAsIs(out, wc);
        }
 
        @Override

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java
index ad3322e0d05e5665108d40b677877ab9a48859f2..75c361e10bff1e2cd7a1b904a11552ff5e7adb88 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java
@@ -344,11 +344,11 @@ public class PackFile implements Iterable<PackIndex.MutableEntry> {
                return dstbuf;
        }
 
-       void copyPackAsIs(PackOutputStream out, boolean validate, WindowCursor curs)
+       void copyPackAsIs(PackOutputStream out, WindowCursor curs)
                        throws IOException {
                // Pin the first window, this ensures the length is accurate.
                curs.pin(this, 0);
-               curs.copyPackAsIs(this, length, validate, out);
+               curs.copyPackAsIs(this, length, out);
        }
 
        final void copyAsIs(PackOutputStream out, LocalObjectToPack src,
@@ -502,7 +502,7 @@ public class PackFile implements Iterable<PackIndex.MutableEntry> {
                        // and we have it pinned.  Write this out without copying.
                        //
                        out.writeHeader(src, inflatedLength);
-                       quickCopy.write(out, dataOffset, (int) dataLength, null);
+                       quickCopy.write(out, dataOffset, (int) dataLength);
 
                } else if (dataLength <= buf.length) {
                        // Tiny optimization: Lots of objects are very small deltas or

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCursor.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCursor.java
index 85c3c74255d0d34824f745c1ba7668f57f0b4a59..21d6cd2ca58697d4bd3630b5554d7c0bae90bef1 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCursor.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCursor.java
@@ -45,9 +45,6 @@
 package org.eclipse.jgit.internal.storage.file;
 
 import java.io.IOException;
-import java.security.MessageDigest;
-import java.text.MessageFormat;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
@@ -59,7 +56,6 @@ import java.util.zip.Inflater;
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.errors.StoredObjectRepresentationNotAvailableException;
-import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.internal.storage.pack.CachedPack;
 import org.eclipse.jgit.internal.storage.pack.ObjectReuseAsIs;
 import org.eclipse.jgit.internal.storage.pack.ObjectToPack;
@@ -232,27 +228,13 @@ final class WindowCursor extends ObjectReader implements ObjectReuseAsIs {
                return cnt - need;
        }
 
-       public void copyPackAsIs(PackOutputStream out, CachedPack pack,
-                       boolean validate) throws IOException {
-               ((LocalCachedPack) pack).copyAsIs(out, validate, this);
+       public void copyPackAsIs(PackOutputStream out, CachedPack pack)
+                       throws IOException {
+               ((LocalCachedPack) pack).copyAsIs(out, this);
        }
 
-       void copyPackAsIs(final PackFile pack, final long length, boolean validate,
+       void copyPackAsIs(final PackFile pack, final long length,
                        final PackOutputStream out) throws IOException {
-               MessageDigest md = null;
-               if (validate) {
-                       md = Constants.newMessageDigest();
-                       byte[] buf = out.getCopyBuffer();
-                       pin(pack, 0);
-                       if (window.copy(0, buf, 0, 12) != 12) {
-                               pack.setInvalid();
-                               throw new IOException(MessageFormat.format(
-                                               JGitText.get().packfileIsTruncated, pack.getPackFile()
-                                                               .getPath()));
-                       }
-                       md.update(buf, 0, 12);
-               }
-
                long position = 12;
                long remaining = length - (12 + 20);
                while (0 < remaining) {
@@ -260,29 +242,10 @@ final class WindowCursor extends ObjectReader implements ObjectReuseAsIs {
 
                        int ptr = (int) (position - window.start);
                        int n = (int) Math.min(window.size() - ptr, remaining);
-                       window.write(out, position, n, md);
+                       window.write(out, position, n);
                        position += n;
                        remaining -= n;
                }
-
-               if (md != null) {
-                       byte[] buf = new byte[20];
-                       byte[] actHash = md.digest();
-
-                       pin(pack, position);
-                       if (window.copy(position, buf, 0, 20) != 20) {
-                               pack.setInvalid();
-                               throw new IOException(MessageFormat.format(
-                                               JGitText.get().packfileIsTruncated, pack.getPackFile()
-                                                               .getPath()));
-                       }
-                       if (!Arrays.equals(actHash, buf)) {
-                               pack.setInvalid();
-                               throw new IOException(MessageFormat.format(
-                                               JGitText.get().packfileCorruptionDetected, pack
-                                                               .getPackFile().getPath()));
-                       }
-               }
        }
 
        /**

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/ObjectReuseAsIs.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/ObjectReuseAsIs.java
index 00b6b6536cceb73a1e55619c914b5b200c926e79..2e5d59960e5098f09cdc13838bf3716840a20a01 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/ObjectReuseAsIs.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/ObjectReuseAsIs.java
@@ -209,16 +209,11 @@ public interface ObjectReuseAsIs {
         *            stream to append the pack onto.
         * @param pack
         *            the cached pack to send.
-        * @param validate
-        *            if true the representation must be validated and not be
-        *            corrupt before being reused. If false, validation may be
-        *            skipped as it will be performed elsewhere in the processing
-        *            pipeline.
         * @throws IOException
         *             the pack cannot be read, or stream did not accept a write.
         */
-       public abstract void copyPackAsIs(PackOutputStream out, CachedPack pack,
-                       boolean validate) throws IOException;
+       public abstract void copyPackAsIs(PackOutputStream out, CachedPack pack)
+                       throws IOException;
 
        /**
         * Obtain the available cached packs that match the bitmap and update

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java
index 510538d8632931a54bdd8ec875af877582d9c2c7..6d0c8e6cde127e92cbbeb1df6276d2948e9c3565 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java
@@ -1048,7 +1048,7 @@ public class PackWriter implements AutoCloseable {
                                stats.reusedObjects += pack.getObjectCount();
                                stats.reusedDeltas += deltaCnt;
                                stats.totalDeltas += deltaCnt;
-                               reuseSupport.copyPackAsIs(out, pack, reuseValidate);
+                               reuseSupport.copyPackAsIs(out, pack);
                        }
                        writeChecksum(out);
                        out.flush();
@@ -1866,7 +1866,7 @@ public class PackWriter implements AutoCloseable {
                                false);
                BitmapBuilder needBitmap = wantBitmap.andNot(haveBitmap);
 
-               if (useCachedPacks && reuseSupport != null
+               if (useCachedPacks && reuseSupport != null && !reuseValidate
                                && (excludeInPacks == null || excludeInPacks.length == 0))
                        cachedPacks.addAll(
                                        reuseSupport.getCachedPacksAndUpdate(needBitmap));