Skip to main content
aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorHan-Wen Nienhuys2019-07-30 10:54:31 -0400
committerHan-Wen Nienhuys2019-08-19 05:40:25 -0400
commit9f9163cbcace9729eec181c0b2f1313a12fcb0b4 (patch)
treee87f3962d83f25916407b440e16ee0231e6ac32e
parentca3b4b608340218e15d45479c424794e95bfb316 (diff)
downloadjgit-9f9163cbcace9729eec181c0b2f1313a12fcb0b4.tar.gz
jgit-9f9163cbcace9729eec181c0b2f1313a12fcb0b4.tar.xz
jgit-9f9163cbcace9729eec181c0b2f1313a12fcb0b4.zip
reftable: enforce ordering for ref and log writes
Previously, the API did not enforce ordering of writes. Misuse of this API would lead to data effectively being lost. Guard against that with IllegalArgumentException, and add a test. Change-Id: I04f55c481d60532fc64d35fa32c47037a03988ae Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
-rw-r--r--org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableTest.java53
-rw-r--r--org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableWriter.java26
2 files changed, 76 insertions, 3 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 db9a4d0ac1..fc363ecf2f 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
@@ -46,13 +46,16 @@ package org.eclipse.jgit.internal.storage.reftable;
import static org.eclipse.jgit.lib.Constants.HEAD;
import static org.eclipse.jgit.lib.Constants.OBJECT_ID_LENGTH;
import static org.eclipse.jgit.lib.Constants.R_HEADS;
+import static org.eclipse.jgit.lib.MoreAsserts.assertThrows;
import static org.eclipse.jgit.lib.Ref.Storage.NEW;
import static org.eclipse.jgit.lib.Ref.Storage.PACKED;
+import static org.hamcrest.CoreMatchers.containsString;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
@@ -415,6 +418,54 @@ public class ReftableTest {
}
@Test
+ public void invalidRefWriteOrder() throws IOException {
+ Ref master = ref(MASTER, 1);
+ Ref next = ref(NEXT, 2);
+ ReftableWriter writer = new ReftableWriter()
+ .setMinUpdateIndex(1)
+ .setMaxUpdateIndex(1)
+ .begin(new ByteArrayOutputStream());
+
+ writer.writeRef(next);
+ IllegalArgumentException e = assertThrows(
+ IllegalArgumentException.class,
+ () -> writer.writeRef(master));
+ assertThat(e.getMessage(), containsString("records must be increasing"));
+ }
+
+ @Test
+ public void invalidReflogWriteOrderUpdateIndex() throws IOException {
+ ReftableWriter writer = new ReftableWriter()
+ .setMinUpdateIndex(1)
+ .setMaxUpdateIndex(2)
+ .begin(new ByteArrayOutputStream());
+ PersonIdent who = new PersonIdent("Log", "Ger", 1500079709, -8 * 60);
+ String msg = "test";
+
+ writer.writeLog(MASTER, 1, who, ObjectId.zeroId(), id(1), msg);
+ IllegalArgumentException e = assertThrows(IllegalArgumentException.class,
+ () -> writer.writeLog(
+ MASTER, 2, who, ObjectId.zeroId(), id(2), msg));
+ assertThat(e.getMessage(), containsString("records must be increasing"));
+ }
+
+ @Test
+ public void invalidReflogWriteOrderName() throws IOException {
+ ReftableWriter writer = new ReftableWriter()
+ .setMinUpdateIndex(1)
+ .setMaxUpdateIndex(1)
+ .begin(new ByteArrayOutputStream());
+ PersonIdent who = new PersonIdent("Log", "Ger", 1500079709, -8 * 60);
+ String msg = "test";
+
+ writer.writeLog(NEXT, 1, who, ObjectId.zeroId(), id(1), msg);
+ IllegalArgumentException e = assertThrows(IllegalArgumentException.class,
+ () -> writer.writeLog(
+ MASTER, 1, who, ObjectId.zeroId(), id(2), msg));
+ assertThat(e.getMessage(), containsString("records must be increasing"));
+ }
+
+ @Test
public void withReflog() throws IOException {
Ref master = ref(MASTER, 1);
Ref next = ref(NEXT, 2);
@@ -577,7 +628,7 @@ public class ReftableTest {
List<Ref> refs = new ArrayList<>();
for (int i = 1; i <= 5670; i++) {
- Ref ref = ref(String.format("refs/heads/%03d", i), i);
+ Ref ref = ref(String.format("refs/heads/%04d", i), i);
refs.add(ref);
writer.writeRef(ref);
}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableWriter.java
index f8b9ffd176..726339617d 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableWriter.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableWriter.java
@@ -44,6 +44,7 @@
package org.eclipse.jgit.internal.storage.reftable;
import static java.lang.Math.log;
+import static java.nio.charset.StandardCharsets.UTF_8;
import static org.eclipse.jgit.internal.storage.reftable.BlockWriter.padBetweenBlocks;
import static org.eclipse.jgit.internal.storage.reftable.ReftableConstants.FILE_FOOTER_LEN;
import static org.eclipse.jgit.internal.storage.reftable.ReftableConstants.FILE_HEADER_LEN;
@@ -108,6 +109,8 @@ public class ReftableWriter {
private ReftableOutputStream out;
private ObjectIdSubclassMap<RefList> obj2ref;
+ private BlockWriter.Entry lastRef;
+ private BlockWriter.Entry lastLog;
private BlockWriter cur;
private Section refs;
private Section objs;
@@ -120,6 +123,8 @@ public class ReftableWriter {
*/
public ReftableWriter() {
this(new ReftableConfig());
+ lastRef = null;
+ lastLog = null;
}
/**
@@ -269,10 +274,22 @@ public class ReftableWriter {
throw new IllegalArgumentException();
}
long d = updateIndex - minUpdateIndex;
- long blockPos = refs.write(new RefEntry(ref, d));
+ RefEntry entry = new RefEntry(ref, d);
+ if (lastRef != null && Entry.compare(lastRef, entry) >= 0) {
+ throwIllegalEntry(lastRef, entry);
+ }
+ lastRef = entry;
+
+ long blockPos = refs.write(entry);
indexRef(ref, blockPos);
}
+ private void throwIllegalEntry(Entry last, Entry now) {
+ throw new IllegalArgumentException(
+ String.format("records must be increasing: last %s, this %s",
+ new String(last.key, UTF_8), new String(now.key, UTF_8)));
+ }
+
private void indexRef(Ref ref, long blockPos) {
if (indexObjects && !ref.isSymbolic()) {
indexId(ref.getObjectId(), blockPos);
@@ -322,7 +339,12 @@ public class ReftableWriter {
throws IOException {
String msg = message != null ? message : ""; //$NON-NLS-1$
beginLog();
- logs.write(new LogEntry(ref, updateIndex, who, oldId, newId, msg));
+ LogEntry entry = new LogEntry(ref, updateIndex, who, oldId, newId, msg);
+ if (lastLog != null && Entry.compare(lastLog, entry) >= 0) {
+ throwIllegalEntry(lastLog, entry);
+ }
+ lastLog = entry;
+ logs.write(entry);
}
/**

Back to the top