From f8ac03459a96138cc9cf8578933a946fb1f179f3 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Mon, 7 Nov 2016 22:31:10 +0100 Subject: [PATCH] Fix loop in auto gc * GC.tooManyLooseObjects() always responded true since the loop missed to advance the iterator so it always incremented until the threshold was exceeded. * Also fix loop exit criterion which was off by 1. * Add some tests. Change-Id: I70976dfaa026efbcf3c46bd45941f37277a18e04 Signed-off-by: Matthias Sohn --- .../internal/storage/file/AutoGcTest.java | 87 +++++++++++++++++++ .../internal/storage/file/GcTestCase.java | 40 +++++++++ .../SampleDataRepositoryTestCase.java | 17 +++- .../jgit/internal/storage/file/GC.java | 10 +-- 4 files changed, 147 insertions(+), 7 deletions(-) create mode 100644 org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/AutoGcTest.java diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/AutoGcTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/AutoGcTest.java new file mode 100644 index 000000000..56994a63b --- /dev/null +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/AutoGcTest.java @@ -0,0 +1,87 @@ +/* + * Copyright (C) 2016, Matthias Sohn + * and other copyright owners as documented in the project's IP log. + * + * This program and the accompanying materials are made available + * under the terms of the Eclipse Distribution License v1.0 which + * accompanies this distribution, is reproduced below, and is + * available at http://www.eclipse.org/org/documents/edl-v10.php + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * + * - Neither the name of the Eclipse Foundation, Inc. nor the + * names of its contributors may be used to endorse or promote + * products derived from this software without specific prior + * written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.eclipse.jgit.internal.storage.file; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import org.eclipse.jgit.lib.ConfigConstants; +import org.eclipse.jgit.storage.file.FileBasedConfig; +import org.eclipse.jgit.test.resources.SampleDataRepositoryTestCase; +import org.junit.Test; + +public class AutoGcTest extends GcTestCase { + + @Test + public void testNotTooManyLooseObjects() { + assertFalse("should not find too many loose objects", + gc.tooManyLooseObjects()); + } + + @Test + public void testTooManyLooseObjects() throws Exception { + FileBasedConfig c = repo.getConfig(); + c.setInt(ConfigConstants.CONFIG_GC_SECTION, null, + ConfigConstants.CONFIG_KEY_AUTO, 255); + c.save(); + commitChain(10, 50); + assertTrue("should find too many loose objects", + gc.tooManyLooseObjects()); + } + + @Test + public void testNotTooManyPacks() { + assertFalse("should not find too many packs", gc.tooManyPacks()); + } + + @Test + public void testTooManyPacks() throws Exception { + FileBasedConfig c = repo.getConfig(); + c.setInt(ConfigConstants.CONFIG_GC_SECTION, null, + ConfigConstants.CONFIG_KEY_AUTOPACKLIMIT, 1); + c.save(); + SampleDataRepositoryTestCase.copyCGitTestPacks(repo); + + assertTrue("should find too many packs", gc.tooManyPacks()); + } +} diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcTestCase.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcTestCase.java index e46328591..90c115227 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcTestCase.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcTestCase.java @@ -105,6 +105,46 @@ public abstract class GcTestCase extends LocalDiskRepositoryTestCase { return tip; } + /** + * Create a chain of commits of given depth with given number of added files + * per commit. + *

+ * Each commit contains {@code files} files as its content. The created + * commit chain is referenced from any ref. + *

+ * A chain will create {@code (2 + files) * depth} objects in Gits object + * database. For each depth level the following objects are created: the + * commit object, the top-level tree object and @code files} blobs for the + * content of the file "a". + * + * @param depth + * the depth of the commit chain. + * @param width + * number of files added per commit + * @return the commit that is the tip of the commit chain + * @throws Exception + */ + protected RevCommit commitChain(int depth, int width) throws Exception { + if (depth <= 0) { + throw new IllegalArgumentException("Chain depth must be > 0"); + } + if (width <= 0) { + throw new IllegalArgumentException("Number of files per commit must be > 0"); + } + CommitBuilder cb = tr.commit(); + RevCommit tip = null; + do { + --depth; + for (int i=0; i < width; i++) { + String id = depth + "-" + i; + cb.add("a" + id, id).message(id); + } + tip = cb.create(); + cb = cb.child(); + } while (depth > 0); + return tip; + } + protected long lastModified(AnyObjectId objectId) throws IOException { return repo.getFS().lastModified( repo.getObjectDatabase().fileFor(objectId)); diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/test/resources/SampleDataRepositoryTestCase.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/test/resources/SampleDataRepositoryTestCase.java index 3a3b3d8d8..a57ef40b6 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/test/resources/SampleDataRepositoryTestCase.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/test/resources/SampleDataRepositoryTestCase.java @@ -47,7 +47,9 @@ package org.eclipse.jgit.test.resources; import java.io.File; +import java.io.IOException; +import org.eclipse.jgit.internal.storage.file.FileRepository; import org.eclipse.jgit.junit.JGitTestUtil; import org.eclipse.jgit.junit.RepositoryTestCase; @@ -57,7 +59,17 @@ public abstract class SampleDataRepositoryTestCase extends RepositoryTestCase { @Override public void setUp() throws Exception { super.setUp(); + copyCGitTestPacks(db); + } + /** + * Copy C Git generated pack files into given repository for testing + * + * @param repo + * test repository to receive packfile copies + * @throws IOException + */ + public static void copyCGitTestPacks(FileRepository repo) throws IOException { final String[] packs = { "pack-34be9032ac282b11fa9babdc2b2a93ca996c9c2f", "pack-df2982f284bbabb6bdb59ee3fcc6eb0983e20371", @@ -67,13 +79,14 @@ public abstract class SampleDataRepositoryTestCase extends RepositoryTestCase { "pack-e6d07037cbcf13376308a0a995d1fa48f8f76aaa", "pack-3280af9c07ee18a87705ef50b0cc4cd20266cf12" }; - final File packDir = new File(db.getObjectDatabase().getDirectory(), "pack"); + final File packDir = new File(repo.getObjectDatabase().getDirectory(), + "pack"); for (String n : packs) { JGitTestUtil.copyTestResource(n + ".pack", new File(packDir, n + ".pack")); JGitTestUtil.copyTestResource(n + ".idx", new File(packDir, n + ".idx")); } JGitTestUtil.copyTestResource("packed-refs", - new File(db.getDirectory(), "packed-refs")); + new File(repo.getDirectory(), "packed-refs")); } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java index f55e15f5f..9c048da40 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java @@ -1164,7 +1164,7 @@ public class GC { /** * @return {@code true} if number of packs > gc.autopacklimit (default 50) */ - private boolean tooManyPacks() { + boolean tooManyPacks() { int autopacklimit = repo.getConfig().getInt( ConfigConstants.CONFIG_GC_SECTION, ConfigConstants.CONFIG_KEY_AUTOPACKLIMIT, @@ -1183,7 +1183,7 @@ public class GC { * * @return {@code true} if number of loose objects > gc.auto (default 6700) */ - private boolean tooManyLooseObjects() { + boolean tooManyLooseObjects() { int auto = repo.getConfig().getInt(ConfigConstants.CONFIG_GC_SECTION, ConfigConstants.CONFIG_KEY_AUTO, DEFAULT_AUTOLIMIT); if (auto <= 0) { @@ -1204,9 +1204,9 @@ public class GC { .matches(); } })) { - Iterator iter = stream.iterator(); - while (iter.hasNext()) { - if (n++ > threshold) { + for (Iterator iter = stream.iterator(); iter.hasNext(); + iter.next()) { + if (++n > threshold) { return true; } }