From 26d36848fef202a69bfb75667d2a2617572e29fd Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Mon, 20 Sep 2021 16:41:17 +0200 Subject: [PATCH 01/19] Add org.bouncycastle.bcutil to p2 repository Bug: 575621 Change-Id: I57de7af9f9b236cac570d9c10c3d1c3886720868 --- .../org.eclipse.jgit.repository/category.xml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/org.eclipse.jgit.packaging/org.eclipse.jgit.repository/category.xml b/org.eclipse.jgit.packaging/org.eclipse.jgit.repository/category.xml index a56cf0a1f..ca0935a51 100644 --- a/org.eclipse.jgit.packaging/org.eclipse.jgit.repository/category.xml +++ b/org.eclipse.jgit.packaging/org.eclipse.jgit.repository/category.xml @@ -147,6 +147,12 @@ + + + + + + From c70c0acb4752f00672e3b856539587e4977bfaea Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Tue, 21 Sep 2021 00:18:03 +0200 Subject: [PATCH 02/19] Update eclipse-jarsigner-plugin to 1.3.2 Change-Id: Id5d05d96c392913de7b4451421c2ffb7b63ab83f --- org.eclipse.jgit.packaging/pom.xml | 2 +- pom.xml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/org.eclipse.jgit.packaging/pom.xml b/org.eclipse.jgit.packaging/pom.xml index 935d9c578..d3d4bcd5e 100644 --- a/org.eclipse.jgit.packaging/pom.xml +++ b/org.eclipse.jgit.packaging/pom.xml @@ -294,7 +294,7 @@ org.eclipse.cbi.maven.plugins eclipse-jarsigner-plugin - 1.1.5 + 1.3.2 org.codehaus.mojo diff --git a/pom.xml b/pom.xml index a9d5a1323..c2b0339db 100644 --- a/pom.xml +++ b/pom.xml @@ -322,7 +322,7 @@ org.eclipse.cbi.maven.plugins eclipse-jarsigner-plugin - 1.1.7 + 1.3.2 org.eclipse.tycho.extras From 283c23012f9bfd4fd83c6a6ec28e9d2149efb514 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Mon, 27 Sep 2021 15:53:30 +0200 Subject: [PATCH 03/19] Fix running benchmarks from bazel add missing dependency to - javaewah - slf4j-api Change-Id: I28dc982791b32f10d20b2fd0671aa8d2514a0fb3 --- org.eclipse.jgit.benchmarks/BUILD | 2 ++ 1 file changed, 2 insertions(+) diff --git a/org.eclipse.jgit.benchmarks/BUILD b/org.eclipse.jgit.benchmarks/BUILD index 7e331b101..ecd268c4b 100644 --- a/org.eclipse.jgit.benchmarks/BUILD +++ b/org.eclipse.jgit.benchmarks/BUILD @@ -8,6 +8,8 @@ jmh_java_benchmarks( name = "benchmarks", srcs = SRCS, deps = [ + "//lib:javaewah", + "//lib:slf4j-api", "//org.eclipse.jgit:jgit", ], ) From 9c3190ce7a8ee036497b93cbb385e828bbc3d701 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Tue, 21 Sep 2021 00:18:03 +0200 Subject: [PATCH 04/19] Update eclipse-jarsigner-plugin to 1.3.2 Change-Id: Id5d05d96c392913de7b4451421c2ffb7b63ab83f (cherry picked from commit c70c0acb4752f00672e3b856539587e4977bfaea) --- org.eclipse.jgit.packaging/pom.xml | 2 +- pom.xml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/org.eclipse.jgit.packaging/pom.xml b/org.eclipse.jgit.packaging/pom.xml index f36d41c9c..03f48d99c 100644 --- a/org.eclipse.jgit.packaging/pom.xml +++ b/org.eclipse.jgit.packaging/pom.xml @@ -313,7 +313,7 @@ org.eclipse.cbi.maven.plugins eclipse-jarsigner-plugin - 1.1.5 + 1.3.2 org.codehaus.mojo diff --git a/pom.xml b/pom.xml index ac1a2164c..795f822a1 100644 --- a/pom.xml +++ b/pom.xml @@ -354,7 +354,7 @@ org.eclipse.cbi.maven.plugins eclipse-jarsigner-plugin - 1.1.7 + 1.3.2 org.eclipse.tycho.extras From b4782d74fdb8e730e06b2fb6c4285fcb4e38439f Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Mon, 27 Sep 2021 16:07:27 +0200 Subject: [PATCH 05/19] reftable: pass on invalid object ID in conversion Before, while trying to determine if an object ID was a tag or not, the reftable conversion would yield an exception. Change-Id: I3688a0ffa9e774ba27f320e3840ff8cada21ecf0 --- .../storage/file/FileReftableTest.java | 23 +++++++++++-------- .../storage/file/FileReftableDatabase.java | 18 ++++++++++----- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java index 2ffbc6255..4907073ff 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java @@ -58,7 +58,9 @@ import static org.junit.Assert.fail; import java.io.File; +import java.io.FileOutputStream; import java.io.IOException; +import java.io.OutputStream; import java.security.SecureRandom; import java.util.ArrayList; import java.util.List; @@ -162,20 +164,21 @@ public void testConvert() throws Exception { assertTrue(db.getRefDatabase().hasFastTipsWithSha1()); } + @Test - public void testConvertToRefdir() throws Exception { + public void testConvertBrokenObjectId() throws Exception { db.convertToPackedRefs(false, false); - assertTrue(db.getRefDatabase() instanceof RefDirectory); - Ref h = db.exactRef("HEAD"); - assertTrue(h.isSymbolic()); - assertEquals("refs/heads/master", h.getTarget().getName()); + new File(db.getDirectory(), "refs/heads").mkdirs(); - Ref b = db.exactRef("refs/heads/b"); - assertFalse(b.isSymbolic()); - assertTrue(b.isPeeled()); - assertEquals(bCommit, b.getObjectId().name()); + String invalidId = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; + File headFile = new File(db.getDirectory(), "refs/heads/broken"); + try (OutputStream os = new FileOutputStream(headFile)) { + os.write(Constants.encodeASCII(invalidId + "\n")); + } - assertFalse(db.getRefDatabase().hasFastTipsWithSha1()); + Ref r = db.exactRef("refs/heads/broken"); + assertNotNull(r); + db.convertToReftable(true, false); } @Test diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java index c0dc625d9..e5ffd77c5 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java @@ -59,6 +59,7 @@ import java.util.stream.Collectors; import org.eclipse.jgit.annotations.NonNull; +import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.events.RefsChangedEvent; import org.eclipse.jgit.internal.storage.reftable.MergedReftable; import org.eclipse.jgit.internal.storage.reftable.ReftableBatchRefUpdate; @@ -615,15 +616,20 @@ private static Ref refForWrite(RevWalk rw, Ref r) throws IOException { r.getTarget().getName(), null)); } ObjectId newId = r.getObjectId(); - RevObject obj = rw.parseAny(newId); RevObject peel = null; - if (obj instanceof RevTag) { - peel = rw.peel(obj); + try { + RevObject obj = rw.parseAny(newId); + if (obj instanceof RevTag) { + peel = rw.peel(obj); + } + } catch (MissingObjectException e) { + /* ignore this error and copy the dangling object ID into reftable too. */ } if (peel != null) { - return new ObjectIdRef.PeeledTag(PACKED, r.getName(), newId, - peel.copy()); - } + return new ObjectIdRef.PeeledTag(PACKED, r.getName(), newId, + peel.copy()); + } + return new ObjectIdRef.PeeledNonTag(PACKED, r.getName(), newId); } From 5f8c48413623ea4d1685063582c74a216207ef51 Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Tue, 28 Sep 2021 20:05:03 +0200 Subject: [PATCH 06/19] reftable: drop code for truncated reads The reftable format is a block based format, but allows for variably sized blocks. This obviously happens for reflog blocks (which are zlib compressed), but is also accepted for index blocks: In the spec, this is motivated as To achieve constant O(1) disk seeks for lookups the index must be a single level, which is permitted to exceed the file's configured block size, but not the format's max block size of 15.99 MiB. Hence, when parsing a block, one cannot be sure of its exact size: after reading a default-size block (eg. 4kb), the block header may state that the block is in fact larger. Before, the code would mark the block as `truncated`, noting // Its OK during sequential scan for an index block to have been // partially read and be truncated in-memory. This happens when // the index block is larger than the file's blockSize. Caller // will break out of its scan loop once it sees the blockType. This looks like either * a remnant of never-implemented functionality. There is no reason to ever sequentially scan an index block. * alluding to sequential scan of the data blocks before the index blocks (eg. scanning refs, which ends when we find the first ref index block, and we can then ignore the index block). This comment is followed by code that populates the restartTbl/restartCnt fields relative to the (possibly truncated) buffer. If the buffer is truncated, this essentially reads garbage, leading to OOB array access when using the index block. Fix this by dropping the truncated logic and issuing a second read if the first read was short. Add a test. We have never observed this failure scenario at Google. We use 64kb blocksize, which requires us to need fewer index entries. The reftable spec mentions an Android repo of size 36M. With 64kb blocks, that's just 562 index entries. Even with historical growth, we are long from requiring an index whose size exceeds a single block. When adding the analogous test for seeking refs, there was no failure. This points to another possibility which is that the code tries to avoid writing large index blocks for refs. I did not investigate further which one it is. Fixes https://bugs.eclipse.org/bugs/show_bug.cgi?id=576250 Bug: 576250 Change-Id: I41ec21fac9e526ef57b3d6fb57b988bd353ee338 Signed-off-by: Han-Wen Nienhuys --- .../storage/reftable/ReftableTest.java | 6 ++++++ .../storage/reftable/BlockReader.java | 19 +++---------------- .../storage/reftable/ReftableReader.java | 2 +- 3 files changed, 10 insertions(+), 17 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableTest.java index ec8b7593f..229c75344 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableTest.java @@ -836,6 +836,12 @@ public void logScan() throws IOException { } assertFalse(lc.next()); } + + for (Ref exp : refs) { + try (LogCursor lc = t.seekLog(exp.getName())) { + assertTrue("has " + exp.getName(), lc.next()); + } + } } @Test diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/BlockReader.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/BlockReader.java index b66751b94..6ae7e45ef 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/BlockReader.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/BlockReader.java @@ -92,7 +92,6 @@ class BlockReader { private byte blockType; private long endPosition; - private boolean truncated; private byte[] buf; private int bufLen; @@ -112,10 +111,6 @@ byte type() { return blockType; } - boolean truncated() { - return truncated; - } - long endPosition() { return endPosition; } @@ -331,16 +326,8 @@ private void parseBlockStart(BlockSource src, long pos, int fileBlockSize) // Log blocks must be inflated after the header. long deflatedSize = inflateBuf(src, pos, blockLen, fileBlockSize); endPosition = pos + 4 + deflatedSize; - } - if (bufLen < blockLen) { - if (blockType != INDEX_BLOCK_TYPE) { - throw invalidBlock(); - } - // Its OK during sequential scan for an index block to have been - // partially read and be truncated in-memory. This happens when - // the index block is larger than the file's blockSize. Caller - // will break out of its scan loop once it sees the blockType. - truncated = true; + } else if (bufLen < blockLen) { + readBlockIntoBuf(src, pos, blockLen); } else if (bufLen > blockLen) { bufLen = blockLen; } @@ -405,7 +392,7 @@ private void setupEmptyFileBlock() { } void verifyIndex() throws IOException { - if (blockType != INDEX_BLOCK_TYPE || truncated) { + if (blockType != INDEX_BLOCK_TYPE) { throw invalidBlock(); } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableReader.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableReader.java index fef5fa57c..3b912ea1c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableReader.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableReader.java @@ -468,7 +468,7 @@ private BlockReader readBlock(long pos, long end) throws IOException { BlockReader b = new BlockReader(); b.readBlock(src, pos, sz); - if (b.type() == INDEX_BLOCK_TYPE && !b.truncated()) { + if (b.type() == INDEX_BLOCK_TYPE) { if (indexCache == null) { indexCache = new LongMap<>(); } From 578b6a79a3de1dcb933f3f58f5e6b5b133371360 Mon Sep 17 00:00:00 2001 From: kylezhao Date: Tue, 28 Sep 2021 10:48:11 +0800 Subject: [PATCH 07/19] GarbageCollectCommand: add numberOfBitmaps to statistics Change-Id: I630afac9408c7313d1cecb1b24476f645c94fc27 Signed-off-by: kylezhao --- .../tst/org/eclipse/jgit/api/GarbageCollectCommandTest.java | 4 ++-- .../tst/org/eclipse/jgit/api/PushCommandTest.java | 2 +- .../src/org/eclipse/jgit/api/GarbageCollectCommand.java | 1 + 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/GarbageCollectCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/GarbageCollectCommandTest.java index 623cdde7b..f98db3497 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/GarbageCollectCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/GarbageCollectCommandTest.java @@ -39,7 +39,7 @@ public void testGConeCommit() throws Exception { Date expire = GitDateParser.parse("now", null, SystemReader .getInstance().getLocale()); Properties res = git.gc().setExpire(expire).call(); - assertTrue(res.size() == 7); + assertTrue(res.size() == 8); } @Test @@ -57,6 +57,6 @@ public void testGCmoreCommits() throws Exception { .setExpire( GitDateParser.parse("now", null, SystemReader .getInstance().getLocale())).call(); - assertTrue(res.size() == 7); + assertTrue(res.size() == 8); } } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PushCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PushCommandTest.java index 5ddc16aaa..a78606556 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PushCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PushCommandTest.java @@ -324,7 +324,7 @@ public void testPushAfterGC() throws Exception { // run a gc to ensure we have a bitmap index Properties res = git1.gc().setExpire(null).call(); - assertEquals(7, res.size()); + assertEquals(8, res.size()); // create another commit so we have something else to push writeTrashFile("b", "content of b"); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/GarbageCollectCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/GarbageCollectCommand.java index a2fbd411f..b5fff7d6d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/GarbageCollectCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/GarbageCollectCommand.java @@ -221,6 +221,7 @@ public Properties getStatistics() throws GitAPIException { @SuppressWarnings("boxing") private static Properties toProperties(RepoStatistics stats) { Properties p = new Properties(); + p.put("numberOfBitmaps", stats.numberOfBitmaps); //$NON-NLS-1$ p.put("numberOfLooseObjects", stats.numberOfLooseObjects); //$NON-NLS-1$ p.put("numberOfLooseRefs", stats.numberOfLooseRefs); //$NON-NLS-1$ p.put("numberOfPackedObjects", stats.numberOfPackedObjects); //$NON-NLS-1$ From c5b30547355327c1536593cc7c4213d4949f5024 Mon Sep 17 00:00:00 2001 From: kylezhao Date: Tue, 31 Aug 2021 19:36:44 +0800 Subject: [PATCH 08/19] Optimize RevWalk.getMergedInto() Transitive Relation Definition: On the DAG of commit history, if A can reach B, C can reach A, then C can reach B. Example: As is shown in the graph below: 1 - 2 - 3 - 4 (side) \ 5 - 6^ (master) - 7 (topic) Find out which branches is 2 merged into: After we calculated that master contains 2, we can mark 6 as TEMP_MARK to avoid unwanted walks. When we want to figure out if 2 is merge into the topic, the traversal path would be [7, 6] instead of [7, 6, 5, 3, 2]. Test: This change can significantly improve performance for tags. On a copy of the Linux repository, the command 'git tag --contains ' had the following performance improvement: commit | Before | After | Rel % 47a44d27ca | 29251ms | 6687ms | -77% 90327e7dff | 21388ms | 6256ms | -70% f85fac0efa | 11150ms | 7338ms | -34% The current version ignores tags, even though the tag is a type of the ref. Follow-up commits I'll fix it. Change-Id: Ie6295ca4d16070499912af462239e679a97cce47 Signed-off-by: kylezhao Reviewed-by: Christian Halstrick Reviewed-by: Martin Fick --- .../jgit/revwalk/RevWalkUtilsReachableTest.java | 17 +++++++++++++++-- .../src/org/eclipse/jgit/revwalk/RevWalk.java | 11 +++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkUtilsReachableTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkUtilsReachableTest.java index 200cb6a4f..0a045c917 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkUtilsReachableTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkUtilsReachableTest.java @@ -82,20 +82,33 @@ public void findBranchesReachableManyTimes() throws Exception { * a b * | | * c d + * | \ + * f e + * | / + * g */ RevCommit a = commit(); RevCommit b = commit(); RevCommit c = commit(a); RevCommit d = commit(b); + RevCommit f = commit(d); + RevCommit e = commit(d); + RevCommit g = commit(f, e); Ref branchA = branch("a", a); Ref branchB = branch("b", b); Ref branchC = branch("c", c); Ref branchD = branch("d", d); + Ref branchE = branch("e", e); + Ref branchF = branch("f", f); + Ref branchG = branch("g", g); assertContains(a, asList(branchA, branchC)); - assertContains(b, asList(branchB, branchD)); + assertContains(b, asList(branchB, branchD, branchE, branchF, branchG)); assertContains(c, asList(branchC)); - assertContains(d, asList(branchD)); + assertContains(d, asList(branchD, branchE, branchF, branchG)); + assertContains(e, asList(branchE, branchG)); + assertContains(f, asList(branchF, branchG)); + assertContains(g, asList(branchG)); } private Ref branch(String name, RevCommit dst) throws Exception { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java index d5b3643af..d2ab4a19e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java @@ -528,6 +528,7 @@ private List getMergedInto(RevCommit needle, Collection haystacks, Enum returnStrategy, ProgressMonitor monitor) throws IOException { List result = new ArrayList<>(); List uninteresting = new ArrayList<>(); + List marked = new ArrayList<>(); RevFilter oldRF = filter; TreeFilter oldTF = treeFilter; try { @@ -545,17 +546,20 @@ private List getMergedInto(RevCommit needle, Collection haystacks, continue; } RevCommit c = (RevCommit) o; - resetRetain(RevFlag.UNINTERESTING); + reset(UNINTERESTING | TEMP_MARK); markStart(c); boolean commitFound = false; RevCommit next; while ((next = next()) != null) { - if (References.isSameObject(next, needle)) { + if (References.isSameObject(next, needle) + || (next.flags & TEMP_MARK) != 0) { result.add(r); if (returnStrategy == GetMergedIntoStrategy.RETURN_ON_FIRST_FOUND) { return result; } commitFound = true; + c.flags |= TEMP_MARK; + marked.add(c); break; } } @@ -571,6 +575,9 @@ private List getMergedInto(RevCommit needle, Collection haystacks, roots.addAll(uninteresting); filter = oldRF; treeFilter = oldTF; + for (RevCommit c : marked) { + c.flags &= ~TEMP_MARK; + } } return result; } From 60b81c5a9280e44fa48d533a61f915382b2b9ce2 Mon Sep 17 00:00:00 2001 From: kylezhao Date: Thu, 2 Sep 2021 11:53:48 +0800 Subject: [PATCH 09/19] Fix RevWalk.getMergedInto() ignores annotated tags If an annotated tag refers to a commit, we should not ignore it. Change-Id: I77504f93636e9e984540e7d8535ef301adce6a80 Signed-off-by: kylezhao --- .../jgit/revwalk/RevWalkMergedIntoTest.java | 21 ++++++++++++++++++- .../src/org/eclipse/jgit/revwalk/RevWalk.java | 2 +- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkMergedIntoTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkMergedIntoTest.java index 2f16aa49e..2754bd36a 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkMergedIntoTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkMergedIntoTest.java @@ -99,7 +99,7 @@ public void testIsMergedIntoAny() throws Exception { createBranch(commit(commit(a)), b); createBranch(commit(commit(i)), c); - assertTrue( rw.isMergedIntoAny(a, getRefs())); + assertTrue(rw.isMergedIntoAny(a, getRefs())); } @Test @@ -125,4 +125,23 @@ public void testIsMergedIntoAll() throws Exception { assertTrue(rw.isMergedIntoAll(a, getRefs())); } + + @Test + public void testMergeIntoAnnotatedTag() throws Exception { + /* + * a + * | + * b + * / \ + * c v1 (annotated tag) + */ + String c = "refs/heads/c"; + String v1 = "refs/tags/v1"; + final RevCommit a = commit(); + final RevCommit b = commit(a); + createBranch(commit(b), c); + createBranch(tag("v1", b), v1); + + assertTrue(rw.isMergedIntoAll(a, getRefs())); + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java index d2ab4a19e..6e29438d0 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java @@ -541,7 +541,7 @@ private List getMergedInto(RevCommit needle, Collection haystacks, return result; } monitor.update(1); - RevObject o = parseAny(r.getObjectId()); + RevObject o = peel(parseAny(r.getObjectId())); if (!(o instanceof RevCommit)) { continue; } From 7d4f3c22ab26b98b703873c3554fec514f109752 Mon Sep 17 00:00:00 2001 From: Alina Djamankulova Date: Mon, 23 Aug 2021 18:22:41 -0700 Subject: [PATCH 10/19] DFS block cache: allow multiple passes for blocks before eviction Let certain pack extensions that are expensive to load from storage (e.g. pack index, bitmap index) stay in DFS block cache longer than others by overriding default cache count through DfsBlockCacheConfig Don't change default behavior when cache override map is empty. Use int cacheCount instead of boolean hot for Ref Signed-off-by: Alina Djamankulova Change-Id: I18062784ec9cc14dbba3e4bb8d9509440cf2d44f --- .../storage/dfs/DfsBlockCacheTest.java | 41 +++++++++++++++ .../internal/storage/dfs/DfsBlockCache.java | 52 ++++++++++++++----- .../storage/dfs/DfsBlockCacheConfig.java | 31 +++++++++++ 3 files changed, 112 insertions(+), 12 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheTest.java index cea00daad..4f300bcd8 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheTest.java @@ -16,9 +16,12 @@ import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.stream.LongStream; +import org.eclipse.jgit.internal.storage.pack.PackExt; import org.eclipse.jgit.junit.TestRng; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; @@ -103,6 +106,44 @@ public void weirdBlockSize() throws Exception { } } + @SuppressWarnings("resource") + @Test + public void hasCacheHotMap() throws Exception { + Map cacheHotMap = new HashMap<>(); + // Pack index will be kept in cache longer. + cacheHotMap.put(PackExt.INDEX, Integer.valueOf(3)); + DfsBlockCache.reconfigure(new DfsBlockCacheConfig().setBlockSize(512) + .setBlockLimit(512 * 4).setCacheHotMap(cacheHotMap)); + cache = DfsBlockCache.getInstance(); + + DfsRepositoryDescription repo = new DfsRepositoryDescription("test"); + InMemoryRepository r1 = new InMemoryRepository(repo); + byte[] content = rng.nextBytes(424242); + ObjectId id; + try (ObjectInserter ins = r1.newObjectInserter()) { + id = ins.insert(OBJ_BLOB, content); + ins.flush(); + } + + try (ObjectReader rdr = r1.newObjectReader()) { + byte[] actual = rdr.open(id, OBJ_BLOB).getBytes(); + assertTrue(Arrays.equals(content, actual)); + } + // All cache entries are hot and cache is at capacity. + assertTrue(LongStream.of(cache.getHitCount()).sum() > 0); + assertEquals(99, cache.getFillPercentage()); + + InMemoryRepository r2 = new InMemoryRepository(repo); + content = rng.nextBytes(424242); + try (ObjectInserter ins = r2.newObjectInserter()) { + ins.insert(OBJ_BLOB, content); + ins.flush(); + } + assertEquals(0, LongStream.of(cache.getMissCount()).sum()); + assertTrue(cache.getEvictions()[PackExt.PACK.getPosition()] > 0); + assertEquals(0, cache.getEvictions()[PackExt.INDEX.getPosition()]); + } + private void resetCache() { DfsBlockCache.reconfigure(new DfsBlockCacheConfig() .setBlockSize(512) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java index 092d035b3..e87bfe24e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java @@ -39,9 +39,10 @@ * Its too expensive during object access to be accurate with a least recently * used (LRU) algorithm. Strictly ordering every read is a lot of overhead that * typically doesn't yield a corresponding benefit to the application. This - * cache implements a clock replacement algorithm, giving each block one chance - * to have been accessed during a sweep of the cache to save itself from - * eviction. + * cache implements a clock replacement algorithm, giving each block at least + * one chance to have been accessed during a sweep of the cache to save itself + * from eviction. The number of swipe chances is configurable per pack + * extension. *

* Entities created by the cache are held under hard references, preventing the * Java VM from clearing anything. Blocks are discarded by the replacement @@ -161,6 +162,9 @@ public static DfsBlockCache getInstance() { /** Current position of the clock. */ private Ref clockHand; + /** Limits of cache hot count per pack file extension. */ + private final int[] cacheHotLimits = new int[PackExt.values().length]; + @SuppressWarnings("unchecked") private DfsBlockCache(DfsBlockCacheConfig cfg) { tableSize = tableSize(cfg); @@ -196,6 +200,15 @@ private DfsBlockCache(DfsBlockCacheConfig cfg) { liveBytes = new AtomicReference<>(newCounters()); refLockWaitTime = cfg.getRefLockWaitTimeConsumer(); + + for (int i = 0; i < PackExt.values().length; ++i) { + Integer limit = cfg.getCacheHotMap().get(PackExt.values()[i]); + if (limit != null && limit.intValue() > 0) { + cacheHotLimits[i] = limit.intValue(); + } else { + cacheHotLimits[i] = DfsBlockCacheConfig.DEFAULT_CACHE_HOT_MAX; + } + } } boolean shouldCopyThroughCache(long length) { @@ -394,7 +407,7 @@ DfsBlock getOrLoad(BlockBasedFile file, long position, DfsReader ctx, } Ref ref = new Ref<>(key, position, v.size(), v); - ref.hot = true; + ref.markHotter(); for (;;) { HashEntry n = new HashEntry(clean(e2), ref); if (table.compareAndSet(slot, e2, n)) { @@ -424,10 +437,10 @@ private void reserveSpace(long reserve, DfsStreamKey key) { Ref prev = clockHand; Ref hand = clockHand.next; do { - if (hand.hot) { - // Value was recently touched. Clear - // hot and give it another chance. - hand.hot = false; + if (hand.isHot()) { + // Value was recently touched. Cache is still hot so + // give it another chance, but cool it down a bit. + hand.markColder(); prev = hand; hand = hand.next; continue; @@ -525,7 +538,7 @@ Ref getOrLoadRef( } getStat(statMiss, key).incrementAndGet(); ref = loader.load(); - ref.hot = true; + ref.markHotter(); // Reserve after loading to get the size of the object reserveSpace(ref.size, key); for (;;) { @@ -568,7 +581,7 @@ Ref put(DfsStreamKey key, long pos, long size, T v) { } ref = new Ref<>(key, pos, size, v); - ref.hot = true; + ref.markHotter(); for (;;) { HashEntry n = new HashEntry(clean(e2), ref); if (table.compareAndSet(slot, e2, n)) { @@ -692,7 +705,8 @@ static final class Ref { final long size; volatile T value; Ref next; - volatile boolean hot; + + private volatile int hotCount; Ref(DfsStreamKey key, long position, long size, T v) { this.key = key; @@ -704,7 +718,7 @@ static final class Ref { T get() { T v = value; if (v != null) { - hot = true; + markHotter(); } return v; } @@ -712,6 +726,20 @@ T get() { boolean has() { return value != null; } + + void markHotter() { + int cap = DfsBlockCache + .getInstance().cacheHotLimits[key.packExtPos]; + hotCount = Math.min(cap, hotCount + 1); + } + + void markColder() { + hotCount = Math.max(0, hotCount - 1); + } + + boolean isHot() { + return hotCount > 0; + } } @FunctionalInterface diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfig.java index 6e7ad3e61..2716f79a1 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfig.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfig.java @@ -18,9 +18,12 @@ import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_STREAM_RATIO; import java.text.MessageFormat; +import java.util.Collections; +import java.util.Map; import java.util.function.Consumer; import org.eclipse.jgit.internal.JGitText; +import org.eclipse.jgit.internal.storage.pack.PackExt; import org.eclipse.jgit.lib.Config; /** @@ -34,6 +37,9 @@ public class DfsBlockCacheConfig { /** 1024 {@link #KB} (number of bytes in one mebibyte/megabyte) */ public static final int MB = 1024 * KB; + /** Default number of max cache hits. */ + public static final int DEFAULT_CACHE_HOT_MAX = 1; + private long blockLimit; private int blockSize; private double streamRatio; @@ -41,6 +47,8 @@ public class DfsBlockCacheConfig { private Consumer refLock; + private Map cacheHotMap; + /** * Create a default configuration. */ @@ -49,6 +57,7 @@ public DfsBlockCacheConfig() { setBlockSize(64 * KB); setStreamRatio(0.30); setConcurrencyLevel(32); + cacheHotMap = Collections.emptyMap(); } /** @@ -184,6 +193,28 @@ public DfsBlockCacheConfig setRefLockWaitTimeConsumer(Consumer c) { return this; } + /** + * Get the map of hot count per pack extension for {@code DfsBlockCache}. + * + * @return map of hot count per pack extension for {@code DfsBlockCache}. + */ + public Map getCacheHotMap() { + return cacheHotMap; + } + + /** + * Set the map of hot count per pack extension for {@code DfsBlockCache}. + * + * @param cacheHotMap + * map of hot count per pack extension for {@code DfsBlockCache}. + * @return {@code this} + */ + public DfsBlockCacheConfig setCacheHotMap( + Map cacheHotMap) { + this.cacheHotMap = Collections.unmodifiableMap(cacheHotMap); + return this; + } + /** * Update properties by setting fields from the configuration. *

From d160e8a9354c1acdb6f60d743581e99cee520077 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Fri, 15 Oct 2021 10:13:53 +0200 Subject: [PATCH 11/19] Fix missing peel-part in lsRefsV2 for loose annotated tags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We observed the following issue: $ git tag -a v1.0 -m v1.0 $ git push origin tag v1.0 $ git ls-remote origin v1.0^{} ... empty result ... On the server (Gerrit) side run git-gc to pack the refs: $ git gc Repeat the ls-remote from the client and the result is correct: $ git ls-remote origin v1.0^{} 7ad85c810de3ae922903d4bdd17c53cd627260ba refs/tags/v1.0^{} Unfortunately, the existing UploadPackTest didn't reveal this issue although it provided the test case needed to do so: testV2LsRefsPeel. This is because The UploadPackTest uses InMemoryRepository which internally uses Dfs* implementations. The issue is only reproducible when using the FileRepository. It is a non-trivial task to refactor the UploadPackTest to work against both InMemoryRepository and FileRepository and this change is not trying to do that. This change creates a new test: UploadPackLsRefsFileRepositoryTest and copies the necesssary code from the UploadPackTest. Change-Id: Icfc7d0ca63f1524bafe24c9626ce12ea72aa3718 Signed-off-by: Saša Živkov --- .../UploadPackLsRefsFileRepositoryTest.java | 123 ++++++++++++++++++ .../eclipse/jgit/transport/UploadPack.java | 1 + 2 files changed, 124 insertions(+) create mode 100644 org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackLsRefsFileRepositoryTest.java diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackLsRefsFileRepositoryTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackLsRefsFileRepositoryTest.java new file mode 100644 index 000000000..7d5fc6101 --- /dev/null +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackLsRefsFileRepositoryTest.java @@ -0,0 +1,123 @@ +/* + * Copyright (C) 2021, Saša Živkov and others + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Distribution License v. 1.0 which is available at + * https://www.eclipse.org/org/documents/edl-v10.php. + * + * SPDX-License-Identifier: BSD-3-Clause + */ +package org.eclipse.jgit.transport; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertTrue; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.util.Objects; +import java.util.function.Consumer; + +import org.eclipse.jgit.internal.storage.file.FileRepository; +import org.eclipse.jgit.junit.LocalDiskRepositoryTestCase; +import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.lib.Sets; +import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevTag; +import org.junit.Before; +import org.junit.Test; + +// TODO: refactor UploadPackTest to run against both DfsRepository and FileRepository +public class UploadPackLsRefsFileRepositoryTest + extends LocalDiskRepositoryTestCase { + + private FileRepository server; + + private TestRepository remote; + + @Before + @Override + public void setUp() throws Exception { + super.setUp(); + server = createWorkRepository(); + remote = new TestRepository<>(server); + } + + @Test + public void testV2LsRefsPeel() throws Exception { + RevCommit tip = remote.commit().message("message").create(); + remote.update("master", tip); + server.updateRef("HEAD").link("refs/heads/master"); + RevTag tag = remote.tag("tag", tip); + remote.update("refs/tags/tag", tag); + + ByteArrayInputStream recvStream = uploadPackV2("command=ls-refs\n", + PacketLineIn.delimiter(), "peel", PacketLineIn.end()); + PacketLineIn pckIn = new PacketLineIn(recvStream); + + assertThat(pckIn.readString(), + is(tip.toObjectId().getName() + " HEAD")); + assertThat(pckIn.readString(), + is(tip.toObjectId().getName() + " refs/heads/master")); + assertThat(pckIn.readString(), is(tag.toObjectId().getName() + + " refs/tags/tag peeled:" + tip.toObjectId().getName())); + assertTrue(PacketLineIn.isEnd(pckIn.readString())); + } + + private ByteArrayInputStream uploadPackV2(String... inputLines) + throws Exception { + return uploadPackV2(null, inputLines); + } + + private ByteArrayInputStream uploadPackV2( + Consumer postConstructionSetup, String... inputLines) + throws Exception { + ByteArrayInputStream recvStream = uploadPackV2Setup( + postConstructionSetup, inputLines); + PacketLineIn pckIn = new PacketLineIn(recvStream); + + // drain capabilities + while (!PacketLineIn.isEnd(pckIn.readString())) { + // do nothing + } + return recvStream; + } + + private ByteArrayInputStream uploadPackV2Setup( + Consumer postConstructionSetup, String... inputLines) + throws Exception { + + ByteArrayInputStream send = linesAsInputStream(inputLines); + + server.getConfig().setString("protocol", null, "version", "2"); + UploadPack up = new UploadPack(server); + if (postConstructionSetup != null) { + postConstructionSetup.accept(up); + } + up.setExtraParameters(Sets.of("version=2")); + + ByteArrayOutputStream recv = new ByteArrayOutputStream(); + up.upload(send, recv, null); + + return new ByteArrayInputStream(recv.toByteArray()); + } + + private static ByteArrayInputStream linesAsInputStream(String... inputLines) + throws IOException { + try (ByteArrayOutputStream send = new ByteArrayOutputStream()) { + PacketLineOut pckOut = new PacketLineOut(send); + for (String line : inputLines) { + Objects.requireNonNull(line); + if (PacketLineIn.isEnd(line)) { + pckOut.end(); + } else if (PacketLineIn.isDelimiter(line)) { + pckOut.writeDelim(); + } else { + pckOut.writeString(line); + } + } + return new ByteArrayInputStream(send.toByteArray()); + } + } +} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index 988901526..e88e59b9a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -1086,6 +1086,7 @@ private void lsRefsV2(PacketLineOut pckOut) throws IOException { rawOut.stopBuffering(); PacketLineOutRefAdvertiser adv = new PacketLineOutRefAdvertiser(pckOut); + adv.init(db); adv.setUseProtocolV2(true); if (req.getPeel()) { adv.setDerefTags(true); From f8b0c00e6a8f5628babff6dd37254a21589b6e44 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Tue, 19 Oct 2021 09:07:14 +0200 Subject: [PATCH 12/19] Set JSch global config values only if not set already Bug: 576604 Change-Id: I04415f07bf2023bc8435c097d1d3c65221d563f1 Signed-off-by: Thomas Wolf --- .../ssh/jsch/JschConfigSessionFactory.java | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/org.eclipse.jgit.ssh.jsch/src/org/eclipse/jgit/transport/ssh/jsch/JschConfigSessionFactory.java b/org.eclipse.jgit.ssh.jsch/src/org/eclipse/jgit/transport/ssh/jsch/JschConfigSessionFactory.java index 453433e0c..77b68bb03 100644 --- a/org.eclipse.jgit.ssh.jsch/src/org/eclipse/jgit/transport/ssh/jsch/JschConfigSessionFactory.java +++ b/org.eclipse.jgit.ssh.jsch/src/org/eclipse/jgit/transport/ssh/jsch/JschConfigSessionFactory.java @@ -45,6 +45,7 @@ import org.eclipse.jgit.transport.SshSessionFactory; import org.eclipse.jgit.transport.URIish; import org.eclipse.jgit.util.FS; +import org.eclipse.jgit.util.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -417,14 +418,26 @@ protected JSch getJSch(OpenSshConfig.Host hc, FS fs) throws JSchException { */ protected JSch createDefaultJSch(FS fs) throws JSchException { final JSch jsch = new JSch(); - JSch.setConfig("ssh-rsa", JSch.getConfig("signature.rsa")); //$NON-NLS-1$ //$NON-NLS-2$ - JSch.setConfig("ssh-dss", JSch.getConfig("signature.dss")); //$NON-NLS-1$ //$NON-NLS-2$ + // See https://bugs.eclipse.org/bugs/show_bug.cgi?id=537790 and + // https://bugs.eclipse.org/bugs/show_bug.cgi?id=576604 + copyGlobalConfigIfNotSet("signature.rsa", "ssh-rsa"); //$NON-NLS-1$ //$NON-NLS-2$ + copyGlobalConfigIfNotSet("signature.dss", "ssh-dss"); //$NON-NLS-1$ //$NON-NLS-2$ configureJSch(jsch); knownHosts(jsch, fs); identities(jsch, fs); return jsch; } + private void copyGlobalConfigIfNotSet(String from, String to) { + String toValue = JSch.getConfig(to); + if (StringUtils.isEmptyOrNull(toValue)) { + String fromValue = JSch.getConfig(from); + if (!StringUtils.isEmptyOrNull(fromValue)) { + JSch.setConfig(to, fromValue); + } + } + } + private static void knownHosts(JSch sch, FS fs) throws JSchException { final File home = fs.userHome(); if (home == null) From f698fbf919c67edac4b3999cc932c1f22da1dffd Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Tue, 19 Oct 2021 09:32:20 +0200 Subject: [PATCH 13/19] JSch: fix service publication for ServiceLoader The file name in META-INF/services must be the fully qualified interface name; the content the fully qualified implementation class name. This was broken in commit 9683bc71. Add a test for the default factory being found by the ServiceLoader. Change-Id: I1f180d7f60e5c1e74a39bbd9a5f0099bd8343e21 Signed-off-by: Thomas Wolf --- .../transport/ssh/jsch/ServiceLoaderTest.java | 24 +++++++++++++++++++ ....eclipse.jgit.transport.SshSessionFactory} | 0 2 files changed, 24 insertions(+) create mode 100644 org.eclipse.jgit.ssh.jsch.test/tst/org/eclipse/jgit/transport/ssh/jsch/ServiceLoaderTest.java rename org.eclipse.jgit.ssh.jsch/resources/META-INF/services/{org.eclipse.jgit.transport.ssh.jsch.SshSessionFactory => org.eclipse.jgit.transport.SshSessionFactory} (100%) diff --git a/org.eclipse.jgit.ssh.jsch.test/tst/org/eclipse/jgit/transport/ssh/jsch/ServiceLoaderTest.java b/org.eclipse.jgit.ssh.jsch.test/tst/org/eclipse/jgit/transport/ssh/jsch/ServiceLoaderTest.java new file mode 100644 index 000000000..2bae221e2 --- /dev/null +++ b/org.eclipse.jgit.ssh.jsch.test/tst/org/eclipse/jgit/transport/ssh/jsch/ServiceLoaderTest.java @@ -0,0 +1,24 @@ +/* + * Copyright (C) 2021, Thomas Wolf and others + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Distribution License v. 1.0 which is available at + * https://www.eclipse.org/org/documents/edl-v10.php. + * + * SPDX-License-Identifier: BSD-3-Clause + */ +package org.eclipse.jgit.transport.ssh.jsch; + +import static org.junit.Assert.assertNotNull; + +import org.eclipse.jgit.transport.SshSessionFactory; +import org.junit.Test; + +public class ServiceLoaderTest { + + @Test + public void testDefaultFactoryFound() { + SshSessionFactory defaultFactory = SshSessionFactory.getInstance(); + assertNotNull(defaultFactory); + } +} diff --git a/org.eclipse.jgit.ssh.jsch/resources/META-INF/services/org.eclipse.jgit.transport.ssh.jsch.SshSessionFactory b/org.eclipse.jgit.ssh.jsch/resources/META-INF/services/org.eclipse.jgit.transport.SshSessionFactory similarity index 100% rename from org.eclipse.jgit.ssh.jsch/resources/META-INF/services/org.eclipse.jgit.transport.ssh.jsch.SshSessionFactory rename to org.eclipse.jgit.ssh.jsch/resources/META-INF/services/org.eclipse.jgit.transport.SshSessionFactory From 3b960ae72daa76d0d8f85c8a0d74575af0c48b24 Mon Sep 17 00:00:00 2001 From: Alina Djamankulova Date: Wed, 13 Oct 2021 13:16:41 -0700 Subject: [PATCH 14/19] DFS block cache: fix lock issue and support parallel index loading This change is a fix to http://git.eclipse.org/r/c/jgit/jgit/+/183562 that was reverted in http://git.eclipse.org/r/c/jgit/jgit/+/184978 due to deadlocks. Separate locks in DfsBlockFile are removed to rely on getting value from DfsBlockCache with region locking in place. With this change bitmap index creation is not blocked on index and reverse index full initialization in DfsPackFile. Now bitmap index and index could be read from storage in parallel in separate threads. A unit test is added for parallel index loading. Signed-off-by: Alina Djamankulova Change-Id: Ic6d9c5a4a254628636aa98a5008447a27a003f69 --- .../storage/dfs/DfsBlockCacheTest.java | 57 ++++++- .../internal/storage/dfs/DfsPackFile.java | 157 ++++++++---------- .../storage/file/PackBitmapIndex.java | 60 ++++++- .../storage/file/PackBitmapIndexV1.java | 97 ++++++++--- 4 files changed, 250 insertions(+), 121 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheTest.java index 4f300bcd8..549e1f469 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheTest.java @@ -21,11 +21,17 @@ import java.util.Map; import java.util.stream.LongStream; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; import org.eclipse.jgit.internal.storage.pack.PackExt; +import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.junit.TestRng; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.ObjectReader; +import org.eclipse.jgit.revwalk.RevCommit; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -144,10 +150,57 @@ public void hasCacheHotMap() throws Exception { assertEquals(0, cache.getEvictions()[PackExt.INDEX.getPosition()]); } + @SuppressWarnings("resource") + @Test + public void noConcurrencySerializedReads() throws Exception { + DfsRepositoryDescription repo = new DfsRepositoryDescription("test"); + InMemoryRepository r1 = new InMemoryRepository(repo); + TestRepository repository = new TestRepository<>( + r1); + RevCommit commit = repository.branch("/refs/ref1").commit() + .add("blob1", "blob1").create(); + repository.branch("/refs/ref2").commit().add("blob2", "blob2") + .parent(commit).create(); + + new DfsGarbageCollector(r1).pack(null); + // Reset cache with concurrency Level at 1 i.e. no concurrency. + DfsBlockCache.reconfigure(new DfsBlockCacheConfig().setBlockSize(512) + .setBlockLimit(1 << 20).setConcurrencyLevel(1)); + cache = DfsBlockCache.getInstance(); + + DfsReader reader = (DfsReader) r1.newObjectReader(); + ExecutorService pool = Executors.newFixedThreadPool(10); + for (DfsPackFile pack : r1.getObjectDatabase().getPacks()) { + // Only load non-garbage pack with bitmap. + if (pack.isGarbage()) { + continue; + } + asyncRun(pool, () -> pack.getBitmapIndex(reader)); + asyncRun(pool, () -> pack.getPackIndex(reader)); + asyncRun(pool, () -> pack.getBitmapIndex(reader)); + } + + pool.shutdown(); + pool.awaitTermination(500, TimeUnit.MILLISECONDS); + assertTrue("Threads did not complete, likely due to a deadlock.", + pool.isTerminated()); + assertEquals(1, cache.getMissCount()[PackExt.BITMAP_INDEX.ordinal()]); + assertEquals(1, cache.getMissCount()[PackExt.INDEX.ordinal()]); + } + private void resetCache() { - DfsBlockCache.reconfigure(new DfsBlockCacheConfig() - .setBlockSize(512) + DfsBlockCache.reconfigure(new DfsBlockCacheConfig().setBlockSize(512) .setBlockLimit(1 << 20)); cache = DfsBlockCache.getInstance(); } + + private void asyncRun(ExecutorService pool, Callable call) { + pool.execute(() -> { + try { + call.call(); + } catch (Exception e) { + // Ignore. + } + }); + } } 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 9be3df3b1..b5bf03fcb 100644 --- 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 @@ -59,13 +59,6 @@ public final class DfsPackFile extends BlockBasedFile { private static final int REC_SIZE = Constants.OBJECT_ID_LENGTH + 8; private static final long REF_POSITION = 0; - /** - * Lock for initialization of {@link #index} and {@link #corruptObjects}. - *

- * This lock ensures only one thread can perform the initialization work. - */ - private final Object initLock = new Object(); - /** Index mapping {@link ObjectId} to position within the pack stream. */ private volatile PackIndex index; @@ -84,6 +77,9 @@ public final class DfsPackFile extends BlockBasedFile { */ private volatile LongList corruptObjects; + /** Lock for {@link #corruptObjects}. */ + private final Object corruptObjectsLock = new Object(); + /** * Construct a reader for an existing, packfile. * @@ -155,35 +151,26 @@ private PackIndex idx(DfsReader ctx) throws IOException { Repository.getGlobalListenerList() .dispatch(new BeforeDfsPackIndexLoadedEvent(this)); - - synchronized (initLock) { - if (index != null) { - return index; + try { + DfsStreamKey idxKey = desc.getStreamKey(INDEX); + AtomicBoolean cacheHit = new AtomicBoolean(true); + DfsBlockCache.Ref idxref = cache.getOrLoadRef(idxKey, + REF_POSITION, () -> { + cacheHit.set(false); + return loadPackIndex(ctx, idxKey); + }); + if (cacheHit.get()) { + ctx.stats.idxCacheHit++; } - - try { - DfsStreamKey idxKey = desc.getStreamKey(INDEX); - AtomicBoolean cacheHit = new AtomicBoolean(true); - DfsBlockCache.Ref idxref = cache.getOrLoadRef( - idxKey, - REF_POSITION, - () -> { - cacheHit.set(false); - return loadPackIndex(ctx, idxKey); - }); - if (cacheHit.get()) { - ctx.stats.idxCacheHit++; - } - PackIndex idx = idxref.get(); - if (index == null && idx != null) { - index = idx; - } - return index; - } catch (IOException e) { - invalid = true; - invalidatingCause = e; - throw e; + PackIndex idx = idxref.get(); + if (index == null && idx != null) { + index = idx; } + return index; + } catch (IOException e) { + invalid = true; + invalidatingCause = e; + throw e; } } @@ -191,7 +178,17 @@ final boolean isGarbage() { return desc.getPackSource() == UNREACHABLE_GARBAGE; } - PackBitmapIndex getBitmapIndex(DfsReader ctx) throws IOException { + /** + * Get the BitmapIndex for this PackFile. + * + * @param ctx + * reader context to support reading from the backing store if + * the index is not already loaded in memory. + * @return the BitmapIndex. + * @throws java.io.IOException + * the bitmap index is not available, or is corrupt. + */ + public PackBitmapIndex getBitmapIndex(DfsReader ctx) throws IOException { if (invalid || isGarbage() || !desc.hasFileExt(BITMAP_INDEX)) { return null; } @@ -200,31 +197,21 @@ PackBitmapIndex getBitmapIndex(DfsReader ctx) throws IOException { return bitmapIndex; } - synchronized (initLock) { - if (bitmapIndex != null) { - return bitmapIndex; - } - - PackIndex idx = idx(ctx); - PackReverseIndex revidx = getReverseIdx(ctx); - DfsStreamKey bitmapKey = desc.getStreamKey(BITMAP_INDEX); - AtomicBoolean cacheHit = new AtomicBoolean(true); - DfsBlockCache.Ref idxref = cache.getOrLoadRef( - bitmapKey, - REF_POSITION, - () -> { - cacheHit.set(false); - return loadBitmapIndex(ctx, bitmapKey, idx, revidx); - }); - if (cacheHit.get()) { - ctx.stats.bitmapCacheHit++; - } - PackBitmapIndex bmidx = idxref.get(); - if (bitmapIndex == null && bmidx != null) { - bitmapIndex = bmidx; - } - return bitmapIndex; + DfsStreamKey bitmapKey = desc.getStreamKey(BITMAP_INDEX); + AtomicBoolean cacheHit = new AtomicBoolean(true); + DfsBlockCache.Ref idxref = cache + .getOrLoadRef(bitmapKey, REF_POSITION, () -> { + cacheHit.set(false); + return loadBitmapIndex(ctx, bitmapKey); + }); + if (cacheHit.get()) { + ctx.stats.bitmapCacheHit++; } + PackBitmapIndex bmidx = idxref.get(); + if (bitmapIndex == null && bmidx != null) { + bitmapIndex = bmidx; + } + return bitmapIndex; } PackReverseIndex getReverseIdx(DfsReader ctx) throws IOException { @@ -232,31 +219,23 @@ PackReverseIndex getReverseIdx(DfsReader ctx) throws IOException { return reverseIndex; } - synchronized (initLock) { - if (reverseIndex != null) { - return reverseIndex; - } - - PackIndex idx = idx(ctx); - DfsStreamKey revKey = new DfsStreamKey.ForReverseIndex( - desc.getStreamKey(INDEX)); - AtomicBoolean cacheHit = new AtomicBoolean(true); - DfsBlockCache.Ref revref = cache.getOrLoadRef( - revKey, - REF_POSITION, - () -> { - cacheHit.set(false); - return loadReverseIdx(ctx, revKey, idx); - }); - if (cacheHit.get()) { - ctx.stats.ridxCacheHit++; - } - PackReverseIndex revidx = revref.get(); - if (reverseIndex == null && revidx != null) { - reverseIndex = revidx; - } - return reverseIndex; + PackIndex idx = idx(ctx); + DfsStreamKey revKey = new DfsStreamKey.ForReverseIndex( + desc.getStreamKey(INDEX)); + AtomicBoolean cacheHit = new AtomicBoolean(true); + DfsBlockCache.Ref revref = cache.getOrLoadRef(revKey, + REF_POSITION, () -> { + cacheHit.set(false); + return loadReverseIdx(ctx, revKey, idx); + }); + if (cacheHit.get()) { + ctx.stats.ridxCacheHit++; } + PackReverseIndex revidx = revref.get(); + if (reverseIndex == null && revidx != null) { + reverseIndex = revidx; + } + return reverseIndex; } /** @@ -1003,7 +982,7 @@ boolean isCorrupt(long offset) { private void setCorrupt(long offset) { LongList list = corruptObjects; if (list == null) { - synchronized (initLock) { + synchronized (corruptObjectsLock) { list = corruptObjects; if (list == null) { list = new LongList(); @@ -1066,11 +1045,8 @@ private DfsBlockCache.Ref loadReverseIdx( revidx); } - private DfsBlockCache.Ref loadBitmapIndex( - DfsReader ctx, - DfsStreamKey bitmapKey, - PackIndex idx, - PackReverseIndex revidx) throws IOException { + private DfsBlockCache.Ref loadBitmapIndex(DfsReader ctx, + DfsStreamKey bitmapKey) throws IOException { ctx.stats.readBitmap++; long start = System.nanoTime(); try (ReadableChannel rc = ctx.db.openFile(desc, BITMAP_INDEX)) { @@ -1086,7 +1062,8 @@ private DfsBlockCache.Ref loadBitmapIndex( bs = wantSize; } in = new BufferedInputStream(in, bs); - bmidx = PackBitmapIndex.read(in, idx, revidx); + bmidx = PackBitmapIndex.read(in, () -> idx(ctx), + () -> getReverseIdx(ctx)); } finally { size = rc.position(); ctx.stats.readBitmapIdxBytes += size; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackBitmapIndex.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackBitmapIndex.java index beb51dc2e..8401f0718 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackBitmapIndex.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackBitmapIndex.java @@ -57,11 +57,10 @@ public abstract class PackBitmapIndex { * @throws CorruptObjectException * the stream does not contain a valid pack bitmap index. */ - public static PackBitmapIndex open( - File idxFile, PackIndex packIndex, PackReverseIndex reverseIndex) + public static PackBitmapIndex open(File idxFile, PackIndex packIndex, + PackReverseIndex reverseIndex) throws IOException { - try (SilentFileInputStream fd = new SilentFileInputStream( - idxFile)) { + try (SilentFileInputStream fd = new SilentFileInputStream(idxFile)) { try { return read(fd, packIndex, reverseIndex); } catch (IOException ioe) { @@ -94,10 +93,39 @@ public static PackBitmapIndex open( * @throws CorruptObjectException * the stream does not contain a valid pack bitmap index. */ - public static PackBitmapIndex read( - InputStream fd, PackIndex packIndex, PackReverseIndex reverseIndex) + public static PackBitmapIndex read(InputStream fd, PackIndex packIndex, + PackReverseIndex reverseIndex) throws IOException { + return new PackBitmapIndexV1(fd, () -> packIndex, () -> reverseIndex); + } + + /** + * Read an existing pack bitmap index file from a buffered stream. + *

+ * The format of the file will be automatically detected and a proper access + * implementation for that format will be constructed and returned to the + * caller. The file may or may not be held open by the returned instance. + * + * @param fd + * stream to read the bitmap index file from. The stream must be + * buffered as some small IOs are performed against the stream. + * The caller is responsible for closing the stream. + * @param packIndexSupplier + * the supplier for pack index for the corresponding pack file. + * @param reverseIndexSupplier + * the supplier for pack reverse index for the corresponding pack + * file. + * @return a copy of the index in-memory. + * @throws java.io.IOException + * the stream cannot be read. + * @throws CorruptObjectException + * the stream does not contain a valid pack bitmap index. + */ + public static PackBitmapIndex read(InputStream fd, + SupplierWithIOException packIndexSupplier, + SupplierWithIOException reverseIndexSupplier) throws IOException { - return new PackBitmapIndexV1(fd, packIndex, reverseIndex); + return new PackBitmapIndexV1(fd, packIndexSupplier, + reverseIndexSupplier); } /** Footer checksum applied on the bottom of the pack file. */ @@ -121,7 +149,8 @@ public static PackBitmapIndex read( * @throws java.lang.IllegalArgumentException * when the item is not found. */ - public abstract ObjectId getObject(int position) throws IllegalArgumentException; + public abstract ObjectId getObject(int position) + throws IllegalArgumentException; /** * Returns a bitmap containing positions for objects that have the given Git @@ -161,4 +190,19 @@ public abstract EWAHCompressedBitmap ofObjectType( * @return the number of bitmaps in this bitmap index. */ public abstract int getBitmapCount(); + + /** + * Supplier that propagates IOException. + * + * @param + * the return type which is expected from {@link #get()} + */ + @FunctionalInterface + public interface SupplierWithIOException { + /** + * @return result + * @throws IOException + */ + T get() throws IOException; + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackBitmapIndexV1.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackBitmapIndexV1.java index b7d241f3f..6846e3bca 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackBitmapIndexV1.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackBitmapIndexV1.java @@ -14,8 +14,11 @@ import java.io.IOException; import java.io.InputStream; import java.text.MessageFormat; +import java.util.ArrayList; import java.util.Arrays; +import java.util.List; +import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.Constants; @@ -46,11 +49,13 @@ class PackBitmapIndexV1 extends BasePackBitmapIndex { private final ObjectIdOwnerMap bitmaps; - PackBitmapIndexV1(final InputStream fd, PackIndex packIndex, - PackReverseIndex reverseIndex) throws IOException { + PackBitmapIndexV1(final InputStream fd, + SupplierWithIOException packIndexSupplier, + SupplierWithIOException reverseIndexSupplier) + throws IOException { + // An entry is object id, xor offset, flag byte, and a length encoded + // bitmap. The object id is an int32 of the nth position sorted by name. super(new ObjectIdOwnerMap()); - this.packIndex = packIndex; - this.reverseIndex = reverseIndex; this.bitmaps = getBitmaps(); final byte[] scratch = new byte[32]; @@ -97,10 +102,10 @@ class PackBitmapIndexV1 extends BasePackBitmapIndex { this.blobs = readBitmap(dataInput); this.tags = readBitmap(dataInput); - // An entry is object id, xor offset, flag byte, and a length encoded - // bitmap. The object id is an int32 of the nth position sorted by name. + // Read full bitmap from storage first. + List idxPositionBitmapList = new ArrayList<>(); // The xor offset is a single byte offset back in the list of entries. - StoredBitmap[] recentBitmaps = new StoredBitmap[MAX_XOR_OFFSET]; + IdxPositionBitmap[] recentBitmaps = new IdxPositionBitmap[MAX_XOR_OFFSET]; for (int i = 0; i < (int) numEntries; i++) { IO.readFully(fd, scratch, 0, 6); int nthObjectId = NB.decodeInt32(scratch, 0); @@ -108,38 +113,58 @@ class PackBitmapIndexV1 extends BasePackBitmapIndex { int flags = scratch[5]; EWAHCompressedBitmap bitmap = readBitmap(dataInput); - if (nthObjectId < 0) + if (nthObjectId < 0) { throw new IOException(MessageFormat.format( JGitText.get().invalidId, String.valueOf(nthObjectId))); - if (xorOffset < 0) + } + if (xorOffset < 0) { throw new IOException(MessageFormat.format( JGitText.get().invalidId, String.valueOf(xorOffset))); - if (xorOffset > MAX_XOR_OFFSET) + } + if (xorOffset > MAX_XOR_OFFSET) { throw new IOException(MessageFormat.format( JGitText.get().expectedLessThanGot, String.valueOf(MAX_XOR_OFFSET), String.valueOf(xorOffset))); - if (xorOffset > i) + } + if (xorOffset > i) { throw new IOException(MessageFormat.format( JGitText.get().expectedLessThanGot, String.valueOf(i), String.valueOf(xorOffset))); - - ObjectId objectId = packIndex.getObjectId(nthObjectId); - StoredBitmap xorBitmap = null; + } + IdxPositionBitmap xorIdxPositionBitmap = null; if (xorOffset > 0) { int index = (i - xorOffset); - xorBitmap = recentBitmaps[index % recentBitmaps.length]; - if (xorBitmap == null) + xorIdxPositionBitmap = recentBitmaps[index + % recentBitmaps.length]; + if (xorIdxPositionBitmap == null) { throw new IOException(MessageFormat.format( JGitText.get().invalidId, String.valueOf(xorOffset))); + } } - - StoredBitmap sb = new StoredBitmap( - objectId, bitmap, xorBitmap, flags); - bitmaps.add(sb); - recentBitmaps[i % recentBitmaps.length] = sb; + IdxPositionBitmap idxPositionBitmap = new IdxPositionBitmap( + nthObjectId, xorIdxPositionBitmap, bitmap, flags); + idxPositionBitmapList.add(idxPositionBitmap); + recentBitmaps[i % recentBitmaps.length] = idxPositionBitmap; } + + this.packIndex = packIndexSupplier.get(); + for (int i = 0; i < idxPositionBitmapList.size(); ++i) { + IdxPositionBitmap idxPositionBitmap = idxPositionBitmapList.get(i); + ObjectId objectId = packIndex + .getObjectId(idxPositionBitmap.nthObjectId); + StoredBitmap sb = new StoredBitmap(objectId, + idxPositionBitmap.bitmap, + idxPositionBitmap.getXorStoredBitmap(), + idxPositionBitmap.flags); + // Save the StoredBitmap for a possible future XorStoredBitmap + // reference. + idxPositionBitmap.sb = sb; + bitmaps.add(sb); + } + + this.reverseIndex = reverseIndexSupplier.get(); } /** {@inheritDoc} */ @@ -214,4 +239,34 @@ private static EWAHCompressedBitmap readBitmap(DataInput dataInput) bitmap.deserialize(dataInput); return bitmap; } + + /** + * Temporary holder of object position in pack index and other metadata for + * {@code StoredBitmap}. + */ + private static final class IdxPositionBitmap { + int nthObjectId; + + IdxPositionBitmap xorIdxPositionBitmap; + + EWAHCompressedBitmap bitmap; + + int flags; + + StoredBitmap sb; + + IdxPositionBitmap(int nthObjectId, + @Nullable IdxPositionBitmap xorIdxPositionBitmap, + EWAHCompressedBitmap bitmap, int flags) { + this.nthObjectId = nthObjectId; + this.xorIdxPositionBitmap = xorIdxPositionBitmap; + this.bitmap = bitmap; + this.flags = flags; + } + + StoredBitmap getXorStoredBitmap() { + return xorIdxPositionBitmap == null ? null + : xorIdxPositionBitmap.sb; + } + } } From 4b4a95b1bbd910976bf872b18ac6cab5944d531c Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Wed, 23 Jun 2021 10:38:50 +0200 Subject: [PATCH 15/19] Remove use of deprecated getAllRefs() in UploadPack Repository.getAllRefs() is deprecated and should not be used anymore. Bug: 534731 Change-Id: I037a9b901275bfa7952b4c79861d7639c9d42715 --- .../eclipse/jgit/transport/UploadPack.java | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index fa2448083..6da6c1334 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -57,6 +57,7 @@ import java.util.Optional; import java.util.Set; import java.util.TreeMap; +import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -406,14 +407,16 @@ public final Map getAdvertisedRefs() { * were advertised. */ public void setAdvertisedRefs(@Nullable Map allRefs) { - if (allRefs != null) + if (allRefs != null) { refs = allRefs; - else - refs = db.getAllRefs(); - if (refFilter == RefFilter.DEFAULT) + } else { + refs = getAllRefs(); + } + if (refFilter == RefFilter.DEFAULT) { refs = transferConfig.getRefFilter().filter(refs); - else + } else { refs = refFilter.filter(refs); + } } /** @@ -864,6 +867,20 @@ public PackStatistics getStatistics() { return statistics; } + /** + * Extract the full list of refs from the ref-db. + * + * @return Map of all refname/ref + */ + private Map getAllRefs() { + try { + return db.getRefDatabase().getRefs().stream().collect( + Collectors.toMap(Ref::getName, Function.identity())); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + private Map getAdvertisedOrDefaultRefs() throws IOException { if (refs != null) { return refs; From 6640baf57d10e6bc54b79955e3477534682b742f Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Wed, 20 Oct 2021 21:17:55 +0200 Subject: [PATCH 16/19] Minor code-clean-up in OpenSshConfigFile Change-Id: I45d50198e43aeb2a56c74026de7ee8c1a30f9d10 Signed-off-by: Thomas Wolf --- .../jgit/internal/transport/ssh/OpenSshConfigFile.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFile.java index 228c25f0a..a7a143328 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFile.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFile.java @@ -210,7 +210,7 @@ private List parse(BufferedReader reader) // The man page doesn't say so, but the openssh parser (readconf.c) // starts out in active mode and thus always applies any lines that // occur before the first host block. We gather those options in a - // HostEntry for DEFAULT_NAME. + // HostEntry. HostEntry defaults = new HostEntry(); HostEntry current = defaults; entries.add(defaults); @@ -309,8 +309,7 @@ private List parseList(String argument) { * @return the validated and possibly sanitized value */ protected String validate(String key, String value) { - if (String.CASE_INSENSITIVE_ORDER.compare(key, - SshConstants.PREFERRED_AUTHENTICATIONS) == 0) { + if (SshConstants.PREFERRED_AUTHENTICATIONS.equalsIgnoreCase(key)) { return stripWhitespace(value); } return value; From ff3c3d8ff52eec0fff297b3ab056447599d589b2 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Sun, 24 Oct 2021 21:32:33 +0200 Subject: [PATCH 17/19] Fix bad indentation in pom.xml Change-Id: I683f190d6914fb48f32cc3b3f0910b4264c7d725 Signed-off-by: Thomas Wolf --- org.eclipse.jgit.ssh.apache/pom.xml | 18 +++++++++--------- org.eclipse.jgit.ssh.jsch/pom.xml | 18 +++++++++--------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/org.eclipse.jgit.ssh.apache/pom.xml b/org.eclipse.jgit.ssh.apache/pom.xml index 6027a2095..823148dfd 100644 --- a/org.eclipse.jgit.ssh.apache/pom.xml +++ b/org.eclipse.jgit.ssh.apache/pom.xml @@ -105,15 +105,15 @@ org.apache.maven.plugins - maven-source-plugin - true - - - attach-sources - process-classes - - jar - + maven-source-plugin + true + + + attach-sources + process-classes + + jar + ${source-bundle-manifest} diff --git a/org.eclipse.jgit.ssh.jsch/pom.xml b/org.eclipse.jgit.ssh.jsch/pom.xml index 95a20b50a..0ad648ff7 100644 --- a/org.eclipse.jgit.ssh.jsch/pom.xml +++ b/org.eclipse.jgit.ssh.jsch/pom.xml @@ -96,15 +96,15 @@ org.apache.maven.plugins - maven-source-plugin - true - - - attach-sources - process-classes - - jar - + maven-source-plugin + true + + + attach-sources + process-classes + + jar + ${source-bundle-manifest} From fc6fe793ce5c17a9270de443d09e38ff2fa45a43 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Fri, 3 Sep 2021 19:49:27 +0200 Subject: [PATCH 18/19] Don't rely on an implicit default character set JEP 400 (Java 18) will change the default character set to UTF-8 unconditionally.[1] Introduce SystemReader.getDefaultCharset() that provides the locale-dependent charset the way JEP 400 recommends. Change all code locations using Charset.defaultCharset() to use the new SystemReader method instead. [1] https://openjdk.java.net/jeps/400 Change-Id: I986f97a410d2fc70748b6f93228a2d45ff100b2c Signed-off-by: Thomas Wolf --- .../tst/org/eclipse/jgit/util/FSTest.java | 5 ++- .../eclipse/jgit/internal/JGitText.properties | 1 + .../src/org/eclipse/jgit/hooks/GitHook.java | 5 +-- .../org/eclipse/jgit/internal/JGitText.java | 1 + .../src/org/eclipse/jgit/util/FS.java | 5 ++- .../src/org/eclipse/jgit/util/FS_POSIX.java | 11 +++--- .../src/org/eclipse/jgit/util/FS_Win32.java | 7 ++-- .../org/eclipse/jgit/util/RawParseUtils.java | 2 +- .../org/eclipse/jgit/util/SystemReader.java | 34 +++++++++++++++++++ 9 files changed, 54 insertions(+), 17 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/FSTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/FSTest.java index 8de7ba6c1..171d80c3d 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/FSTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/FSTest.java @@ -19,7 +19,6 @@ import java.io.File; import java.io.IOException; -import java.nio.charset.Charset; import java.nio.file.Files; import java.nio.file.InvalidPathException; import java.nio.file.Path; @@ -182,7 +181,7 @@ public void testReadPipePosixCommandFailure() FS.readPipe(fs.userHome(), new String[] { "/bin/sh", "-c", "exit 1" }, - Charset.defaultCharset().name()); + SystemReader.getInstance().getDefaultCharset().name()); } @Test(expected = CommandFailedException.class) @@ -192,7 +191,7 @@ public void testReadPipeCommandStartFailure() FS.readPipe(fs.userHome(), new String[] { "this-command-does-not-exist" }, - Charset.defaultCharset().name()); + SystemReader.getInstance().getDefaultCharset().name()); } @Test diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index 3acceab09..74762a902 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -440,6 +440,7 @@ lockOnNotHeld=Lock on {0} not held. lockStreamClosed=Output to lock on {0} already closed lockStreamMultiple=Output to lock on {0} already opened logInconsistentFiletimeDiff={}: inconsistent duration from file timestamps on {}, {}: {} > {}, but diff = {}. Aborting measurement at resolution {}. +logInvalidDefaultCharset=System property "native.encoding" specifies unknown character set: {} logLargerFiletimeDiff={}: inconsistent duration from file timestamps on {}, {}: diff = {} > {} (last good value). Aborting measurement. logSmallerFiletime={}: got smaller file timestamp on {}, {}: {} < {}. Aborting measurement at resolution {}. logXDGConfigHomeInvalid=Environment variable XDG_CONFIG_HOME contains an invalid path {} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/hooks/GitHook.java b/org.eclipse.jgit/src/org/eclipse/jgit/hooks/GitHook.java index ce3ad2239..dbfd41594 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/hooks/GitHook.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/hooks/GitHook.java @@ -12,13 +12,13 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.OutputStream; -import java.nio.charset.Charset; import java.util.concurrent.Callable; import org.eclipse.jgit.api.errors.AbortedByHookException; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.util.FS; import org.eclipse.jgit.util.ProcessResult; +import org.eclipse.jgit.util.SystemReader; import org.eclipse.jgit.util.io.TeeOutputStream; /** @@ -171,7 +171,8 @@ protected void doRun() throws AbortedByHookException, IOException { getStdinArgs()); if (result.isExecutedWithError()) { handleError(new String(errorByteArray.toByteArray(), - Charset.defaultCharset().name()), result); + SystemReader.getInstance().getDefaultCharset().name()), + result); } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index 76340dabb..3d5d0607e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -468,6 +468,7 @@ public static JGitText get() { /***/ public String lockStreamClosed; /***/ public String lockStreamMultiple; /***/ public String logInconsistentFiletimeDiff; + /***/ public String logInvalidDefaultCharset; /***/ public String logLargerFiletimeDiff; /***/ public String logSmallerFiletime; /***/ public String logXDGConfigHomeInvalid; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java index 31a05933c..dd656e5f2 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java @@ -23,7 +23,6 @@ import java.io.OutputStream; import java.io.OutputStreamWriter; import java.io.Writer; -import java.nio.charset.Charset; import java.nio.file.AccessDeniedException; import java.nio.file.FileStore; import java.nio.file.Files; @@ -1507,7 +1506,7 @@ protected File discoverGitSystemConfig() { try { v = readPipe(gitExe.getParentFile(), new String[] { gitExe.getPath(), "--version" }, //$NON-NLS-1$ - Charset.defaultCharset().name()); + SystemReader.getInstance().getDefaultCharset().name()); } catch (CommandFailedException e) { LOG.warn(e.getMessage()); return null; @@ -1527,7 +1526,7 @@ protected File discoverGitSystemConfig() { w = readPipe(gitExe.getParentFile(), new String[] { gitExe.getPath(), "config", "--system", //$NON-NLS-1$ //$NON-NLS-2$ "--edit" }, //$NON-NLS-1$ - Charset.defaultCharset().name(), env); + SystemReader.getInstance().getDefaultCharset().name(), env); } catch (CommandFailedException e) { LOG.warn(e.getMessage()); return null; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java index 946d81c73..1c113617f 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java @@ -17,7 +17,6 @@ import java.io.IOException; import java.io.InputStreamReader; import java.io.OutputStream; -import java.nio.charset.Charset; import java.nio.file.FileAlreadyExistsException; import java.nio.file.FileStore; import java.nio.file.FileSystemException; @@ -119,8 +118,8 @@ private static int readUmask() { new String[] { "sh", "-c", "umask" }, //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ null, null); try (BufferedReader lineRead = new BufferedReader( - new InputStreamReader(p.getInputStream(), Charset - .defaultCharset().name()))) { + new InputStreamReader(p.getInputStream(), SystemReader + .getInstance().getDefaultCharset().name()))) { if (p.waitFor() == 0) { String s = lineRead.readLine(); if (s != null && s.matches("0?\\d{3}")) { //$NON-NLS-1$ @@ -150,7 +149,8 @@ protected File discoverGitExe() { try { String w = readPipe(userHome(), new String[]{"bash", "--login", "-c", "which git"}, // //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ - Charset.defaultCharset().name()); + SystemReader.getInstance().getDefaultCharset() + .name()); if (!StringUtils.isEmptyOrNull(w)) { gitExe = new File(w); } @@ -168,7 +168,8 @@ protected File discoverGitExe() { try { String w = readPipe(userHome(), new String[] { "xcode-select", "-p" }, //$NON-NLS-1$ //$NON-NLS-2$ - Charset.defaultCharset().name()); + SystemReader.getInstance().getDefaultCharset() + .name()); if (StringUtils.isEmptyOrNull(w)) { gitExe = null; } else { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_Win32.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_Win32.java index d44dc32d1..ff094f697 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_Win32.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_Win32.java @@ -13,7 +13,6 @@ import java.io.File; import java.io.IOException; -import java.nio.charset.Charset; import java.nio.file.FileVisitOption; import java.nio.file.FileVisitResult; import java.nio.file.Files; @@ -150,8 +149,10 @@ protected File discoverGitExe() { String w; try { w = readPipe(userHome(), - new String[]{"bash", "--login", "-c", "which git"}, // //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ - Charset.defaultCharset().name()); + new String[] { "bash", "--login", "-c", //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + "which git" }, // //$NON-NLS-1$ + SystemReader.getInstance().getDefaultCharset() + .name()); } catch (CommandFailedException e) { LOG.warn(e.getMessage()); return null; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/RawParseUtils.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/RawParseUtils.java index df9c6c78f..93bf84719 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/RawParseUtils.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/RawParseUtils.java @@ -1160,7 +1160,7 @@ public static String decodeNoFallback(final Charset cs, // Try the default character set. A small group of people // might actually use the same (or very similar) locale. - Charset defcs = Charset.defaultCharset(); + Charset defcs = SystemReader.getInstance().getDefaultCharset(); if (!defcs.equals(cs) && !defcs.equals(UTF_8)) { try { return decode(b, defcs); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/SystemReader.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/SystemReader.java index 54fd539f6..16e257791 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/SystemReader.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/SystemReader.java @@ -17,6 +17,9 @@ import java.io.IOException; import java.net.InetAddress; import java.net.UnknownHostException; +import java.nio.charset.Charset; +import java.nio.charset.IllegalCharsetNameException; +import java.nio.charset.UnsupportedCharsetException; import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.nio.file.Paths; @@ -198,6 +201,8 @@ public static void setInstance(SystemReader newReader) { private AtomicReference jgitConfig = new AtomicReference<>(); + private volatile Charset defaultCharset; + private void init() { // Creating ObjectChecker must be deferred. Unit tests change // behavior of is{Windows,MacOS} in constructor of subclass. @@ -438,6 +443,35 @@ public Locale getLocale() { return Locale.getDefault(); } + /** + * Retrieves the default {@link Charset} depending on the system locale. + * + * @return the {@link Charset} + * @since 6.0 + * @see JEP 400 + */ + public Charset getDefaultCharset() { + Charset result = defaultCharset; + if (result == null) { + // JEP 400: Java 18 populates this system property. + String encoding = getProperty("native.encoding"); //$NON-NLS-1$ + try { + if (!StringUtils.isEmptyOrNull(encoding)) { + result = Charset.forName(encoding); + } + } catch (IllegalCharsetNameException + | UnsupportedCharsetException e) { + LOG.error(JGitText.get().logInvalidDefaultCharset, encoding); + } + if (result == null) { + // This is always UTF-8 on Java >= 18. + result = Charset.defaultCharset(); + } + defaultCharset = result; + } + return result; + } + /** * Returns a simple date format instance as specified by the given pattern. * From b3a8a94a97d7f1370f534c2bbe069ff0e24b2da9 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Tue, 12 Oct 2021 22:14:33 +0200 Subject: [PATCH 19/19] Fix checkout of files with mixed line endings on text=auto eol=crlf Add tests for files having been checked in with mixed LF and CR-LF line endings, and then being checked out with text or text=auto and eol=lf or eol=crlf. These test cases were missing, and thus commit efd1cc05 missed that AutoCRLFOutputStream needs the same detection as AutoLFOutputStream. Fix AutoCRLFOutputStream to not convert line endings if the blob in the repository contains CR-LF. Bug: 575393 Change-Id: Id0c7ae772e282097e95fddcd3f1f9d82aae31e43 Signed-off-by: Thomas Wolf --- .../jgit/api/EolStreamTypeUtilTest.java | 3 +- .../jgit/lib/DirCacheCheckoutTest.java | 28 +++++++++++++++++++ .../util/io/AutoCRLFOutputStreamTest.java | 2 +- .../jgit/util/io/AutoCRLFOutputStream.java | 3 ++ 4 files changed, 34 insertions(+), 2 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/EolStreamTypeUtilTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/EolStreamTypeUtilTest.java index 2a553ce1a..673aa1e9c 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/EolStreamTypeUtilTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/EolStreamTypeUtilTest.java @@ -83,7 +83,8 @@ public void testCheckoutCRLF() throws Exception { testCheckout(TEXT_CRLF, AUTO_CRLF, "\r\n", "\r\n"); testCheckout(TEXT_CRLF, AUTO_CRLF, "\n\r", "\r\n\r"); - testCheckout(TEXT_CRLF, AUTO_CRLF, "\n\r\n", "\r\n\r\n"); + testCheckout(null, AUTO_CRLF, "\n\r\n", "\n\r\n"); + testCheckout(TEXT_CRLF, null, "\n\r\n", "\r\n\r\n"); testCheckout(TEXT_CRLF, AUTO_CRLF, "\r\n\r", "\r\n\r"); testCheckout(TEXT_CRLF, AUTO_CRLF, "a\nb\n", "a\r\nb\r\n"); diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/DirCacheCheckoutTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/DirCacheCheckoutTest.java index b943486b1..af8a58f6f 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/DirCacheCheckoutTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/DirCacheCheckoutTest.java @@ -337,6 +337,34 @@ public void testCheckoutWithLFAutoEolCrLf() throws Exception { "first line\r\nsecond line\r\n", "f text=auto eol=crlf"); } + @Test + public void testCheckoutMixedAutoEolCrLf() throws Exception { + checkoutLineEndings("first line\nsecond line\r\n", + "first line\nsecond line\r\n", "f text=auto eol=crlf"); + } + + @Test + public void testCheckoutMixedAutoEolLf() throws Exception { + checkoutLineEndings("first line\nsecond line\r\n", + "first line\nsecond line\r\n", "f text=auto eol=lf"); + } + + @Test + public void testCheckoutMixedTextCrLf() throws Exception { + // Huh? Is this a bug in git? Both git 2.18.0 and git 2.33.0 do + // write the file with CRLF (and consequently report the file as + // modified in "git status" after check-out), however the CRLF in the + // repository is _not_ replaced by LF with eol=lf (see test below). + checkoutLineEndings("first line\nsecond line\r\n", + "first line\r\nsecond line\r\n", "f text eol=crlf"); + } + + @Test + public void testCheckoutMixedTextLf() throws Exception { + checkoutLineEndings("first line\nsecond line\r\nfoo", + "first line\nsecond line\r\nfoo", "f text eol=lf"); + } + private DirCacheCheckout resetHard(RevCommit commit) throws NoWorkTreeException, CorruptObjectException, IOException { diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/io/AutoCRLFOutputStreamTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/io/AutoCRLFOutputStreamTest.java index 85ce5381f..db2f6da31 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/io/AutoCRLFOutputStreamTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/io/AutoCRLFOutputStreamTest.java @@ -34,7 +34,7 @@ public void test() throws IOException { assertNoCrLf("\r\n\r", "\n\r"); assertNoCrLf("\r\n\r\r", "\r\n\r\r"); assertNoCrLf("\r\n\r\n", "\r\n\r\n"); - assertNoCrLf("\r\n\r\n\r", "\n\r\n\r"); + assertNoCrLf("\n\r\n\r", "\n\r\n\r"); assertNoCrLf("\0\n", "\0\n"); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/io/AutoCRLFOutputStream.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/io/AutoCRLFOutputStream.java index ea6b6588c..97fe01e5d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/io/AutoCRLFOutputStream.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/io/AutoCRLFOutputStream.java @@ -137,6 +137,9 @@ private int buffer(byte[] b, int off, int len) throws IOException { private void decideMode() throws IOException { if (detectBinary) { isBinary = RawText.isBinary(binbuf, binbufcnt); + if (!isBinary) { + isBinary = RawText.isCrLfText(binbuf, binbufcnt); + } detectBinary = false; } int cachedLen = binbufcnt;