From c70c0acb4752f00672e3b856539587e4977bfaea Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Tue, 21 Sep 2021 00:18:03 +0200 Subject: [PATCH 1/6] 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 2/6] 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 3/6] 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 4/6] 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 5/6] 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 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 6/6] 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);