MidxWriter: Write to tmp files and use atomic rename to commit Write the midx to a tmp file and atomically rename it to the destination file to commit the write. This is a common pattern used in git to prevent half-written files in case of trouble. Make also sure to delete the previous bitmaps Change-Id: Ifcf76a6ece513032873d9e673f10dfea6a6a6964
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/MidxWriterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/MidxWriterTest.java new file mode 100644 index 0000000..bb45d0a --- /dev/null +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/MidxWriterTest.java
@@ -0,0 +1,91 @@ +package org.eclipse.jgit.internal.storage.file; + +import static org.eclipse.jgit.internal.storage.pack.PackExt.BITMAP_INDEX; +import static org.eclipse.jgit.lib.Constants.MIDX_FILE; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.io.File; +import java.io.OutputStreamWriter; +import java.nio.charset.StandardCharsets; + +import org.eclipse.jgit.internal.storage.midx.MultiPackIndex; +import org.eclipse.jgit.internal.storage.midx.MultiPackIndexLoader; +import org.eclipse.jgit.junit.LocalDiskRepositoryTestCase; +import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.lib.TextProgressMonitor; +import org.eclipse.jgit.revwalk.RevBlob; +import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.storage.file.WindowCacheConfig; +import org.eclipse.jgit.storage.pack.PackConfig; +import org.junit.Before; +import org.junit.Test; + +public class MidxWriterTest extends LocalDiskRepositoryTestCase { + private int streamThreshold = 16 * 1024; + + private final static ObjectId UNKNOWN_OBJ = ObjectId + .fromString("678668bdd3a609c6e1d7fea516e9fc1ed4f50ad2"); + + private FileRepository repo; + + private TestRepository<Repository> tr; + +// private WindowCursor wc; + + @Override + @Before + public void setUp() throws Exception { + super.setUp(); + + WindowCacheConfig cfg = new WindowCacheConfig(); + cfg.setStreamFileThreshold(streamThreshold); + cfg.install(); + + repo = createBareRepository(); + tr = new TestRepository<>(repo); +// wc = (WindowCursor) repo.newObjectReader(); + } + + @Test + public void midx_write() throws Exception { + TestRepository<Repository>.BranchBuilder branch = tr + .branch("refs/heads/main"); + RevBlob contentsOfA = tr.blob("contents of a"); + RevCommit commitA = branch.commit().add("a.txt", contentsOfA).create(); + tr.packAndPrune(); + + RevBlob contentsOfB = tr.blob("contents of b"); + RevCommit commitB = branch.commit().add("b.txt", contentsOfB).create(); + tr.packAndPrune(); + + RevCommit commitC = branch.commit().add("c.txt", "contents of c commit") + .create(); + tr.packAndPrune(); + + File midxFile = new File(repo.getObjectDatabase().getPackDirectory(), + MIDX_FILE); + assertFalse(midxFile.exists()); + MidxWriter.writeMidx( + new TextProgressMonitor(new OutputStreamWriter(System.out, + StandardCharsets.UTF_8)), + repo, repo.getObjectDatabase().getPacks(), midxFile, + new PackConfig()); + assertTrue(midxFile.exists()); + + MultiPackIndex midx = MultiPackIndexLoader.open(midxFile); + assertTrue(midx.hasObject(commitA)); + assertTrue(midx.hasObject(commitB)); + assertTrue(midx.hasObject(commitC)); + assertFalse(midx.hasObject(UNKNOWN_OBJ)); + + File midxBitmaps = new File(repo.getObjectDatabase().getPackDirectory(), + String.format("%s-%s.%s", MIDX_FILE, + ObjectId.fromRaw(midx.getChecksum()).name(), + BITMAP_INDEX.getExtension())); + assertTrue(midxBitmaps.exists()); + + } +}
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/midx/MultiPackIndexWriterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/midx/MultiPackIndexWriterTest.java index 1ca8aaf..1743fb0 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/midx/MultiPackIndexWriterTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/midx/MultiPackIndexWriterTest.java
@@ -174,6 +174,18 @@ public void jgit_emptyMidx() throws IOException { assertEquals(5, chunkIds.indexOf(MIDX_CHUNKID_PACKNAMES)); } + @Test + public void jgit_noPacks() throws IOException { + PackIndexMerger packs = PackIndexMerger.builder().build(); + MultiPackIndexWriter writer = new MultiPackIndexWriter(); + ByteArrayOutputStream out = new ByteArrayOutputStream(); + MultiPackIndexWriter.Result result = writer + .write(NullProgressMonitor.INSTANCE, out, packs); + assertEquals(0, result.packNames().size()); + MidxHeader header = readHeader(out); + assertEquals(0, header.packCount()); + } + private List<Integer> readChunkIds(ByteArrayOutputStream out) { List<Integer> chunkIds = new ArrayList<>(); byte[] raw = out.toByteArray(); @@ -193,4 +205,14 @@ private static PackIndex indexOf(IndexObject... objs) { private static IndexObject object(String name, long offset) { return new IndexObject(name, offset); } + + private record MidxHeader(int packCount) { + } + + private MidxHeader readHeader(ByteArrayOutputStream out) { + byte[] midx = out.toByteArray(); + + int packCount = NB.decodeInt32(midx, 8); + return new MidxHeader(packCount); + } }
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/MidxWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/MidxWriter.java index ef58d63..8143bbb 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/MidxWriter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/MidxWriter.java
@@ -9,11 +9,15 @@ */ package org.eclipse.jgit.internal.storage.file; +import static org.eclipse.jgit.internal.storage.pack.PackExt.BITMAP_INDEX; + import java.io.File; import java.io.FileOutputStream; import java.io.IOException; +import java.nio.file.StandardCopyOption; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.Comparator; import java.util.HashSet; import java.util.List; @@ -21,6 +25,7 @@ import java.util.Set; import org.eclipse.jgit.internal.revwalk.RefAdvancerWalk; +import org.eclipse.jgit.internal.storage.midx.MidxMetadataReader; import org.eclipse.jgit.internal.storage.midx.MultiPackIndex; import org.eclipse.jgit.internal.storage.midx.MultiPackIndexWriter; import org.eclipse.jgit.internal.storage.midx.PackIndexMerger; @@ -37,6 +42,7 @@ import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.storage.pack.PackConfig; import org.eclipse.jgit.util.Base64; +import org.eclipse.jgit.util.FileUtils; /** * Helper to write multipack indexes. @@ -44,6 +50,11 @@ public class MidxWriter { /** + * Do not build a midx if there are less than this amount of packs to cover. + */ + private static final int MIN_PACKS_FOR_MIDX = 2; + + /** * Write a mdix over the packs * * @param pm @@ -62,12 +73,15 @@ public class MidxWriter { public static void writeMidx(ProgressMonitor pm, Repository repo, Collection<Pack> packs, File midxOut, PackConfig packConfig) throws IOException { + + Collection<Pack> packList = flattenMidxPackList(packs).stream() + .sorted(Comparator.comparing(Pack::getPackName)).toList(); + if (packList.size() < MIN_PACKS_FOR_MIDX) { + return; + } PackIndexMerger.Builder builder = PackIndexMerger.builder(); builder.setProgressMonitor(pm); - - Collection<Pack> packList = packs.stream() - .sorted(Comparator.comparing(Pack::getPackName)).toList(); - pm.beginTask("Adding packs to midx", packList.size()); + pm.beginTask("Adding packs to midx", packList.size()); //$NON-NLS-1$ for (Pack pack : packList) { if (pack instanceof PackMidx) { throw new IllegalArgumentException( @@ -75,25 +89,60 @@ public static void writeMidx(ProgressMonitor pm, Repository repo, } PackFile packFile = pack.getPackFile().create(PackExt.INDEX); builder.addPack(packFile.getName(), pack.getIndex()); + pm.update(1); } PackIndexMerger data = builder.build(); pm.endTask(); + File oldMidxBitmaps = null; + if (midxOut.exists()) { + MidxMetadataReader.MidxMetadata midxMetadata = MidxMetadataReader + .read(midxOut); + byte[] checksum = midxMetadata.checksum(); + String midxBitmapsPath = midxOut.getAbsoluteFile() + "-" //$NON-NLS-1$ + + ObjectId.fromRaw(checksum).name() + "." //$NON-NLS-1$ + + BITMAP_INDEX.getExtension(); + File previousBitmaps = new File(midxBitmapsPath); + if (previousBitmaps.exists()) { + oldMidxBitmaps = previousBitmaps; + } + } + + String midxFilename = midxOut.getAbsolutePath(); + File midxOutTmp = new File( + midxFilename + BITMAP_INDEX.getTmpExtension()); MultiPackIndexWriter writer = new MultiPackIndexWriter(); MultiPackIndexWriter.Result result; try (FileOutputStream out = new FileOutputStream( - midxOut.getAbsolutePath())) { + midxOutTmp.getAbsolutePath())) { result = writer.write(pm, out, data); } + File midxOutBitmaps = new File(midxOut.getAbsoluteFile() + "-" //$NON-NLS-1$ + + ObjectId.fromRaw(Base64.decode(result.checksum())).name() + + "." + BITMAP_INDEX.getExtension()); + File midxOutBitmapsTmp = null; if (packConfig != null) { - File midxOutBitmaps = new File( - midxOut.getAbsolutePath() + ".bitmaps"); - createAndAttachBitmaps(pm, repo, midxOutBitmaps, + midxOutBitmapsTmp = new File(midxOut.getAbsolutePath() + "-" //$NON-NLS-1$ + + ObjectId.fromRaw(Base64.decode(result.checksum())).name() + + BITMAP_INDEX.getTmpExtension()); + createAndAttachBitmaps(pm, repo, midxOutBitmapsTmp, Base64.decode(result.checksum()), data, packList, new PackConfig(repo)); } + + FileUtils.rename(midxOutTmp.getAbsoluteFile(), + midxOut.getAbsoluteFile(), StandardCopyOption.ATOMIC_MOVE); + if (midxOutBitmapsTmp != null) { + FileUtils.rename(midxOutBitmapsTmp.getAbsoluteFile(), + midxOutBitmaps.getAbsoluteFile(), + StandardCopyOption.ATOMIC_MOVE); + } + + if (oldMidxBitmaps != null && !oldMidxBitmaps.equals(midxOutBitmaps)) { + FileUtils.delete(oldMidxBitmaps); + } } private static void createAndAttachBitmaps(ProgressMonitor pm, @@ -162,4 +211,18 @@ private static List<ObjectToPack> asObjectsToPack(ProgressMonitor pm, pm.endTask(); return result; } + + static List<Pack> flattenMidxPackList(Collection<Pack> packs) { + List<Pack> output = new ArrayList<>(); + for (Pack p : packs) { + List<Pack> coveredPacks = new ArrayList<>(p.getCoveredPacks()); + if (coveredPacks.isEmpty()) { + output.add(p); + } else { + Collections.reverse(coveredPacks); + output.addAll(coveredPacks); + } + } + return Collections.unmodifiableList(output); + } }