ObjectIdSerializer: Support serialization of known non-null ObjectId

The implementation of ObjectIdSerializer, added in change I7599cf8bd,
is not equivalent to the original implementation in Gerrit [1].

The Gerrit implementation provides separate methods to (de)serialize
instances of ObjectId that are known to be non-null. In these methods,
no "marker" is written to the stream. Replacing Gerrit's implementation
with ObjectIdSerializer [2] broke persistent caches because it started
writing markers where they were not expected [3].

Since ObjectIdSerializer is included in JGit 4.11 we can't change the
existing #write and #read methods. Keep those as-is, but extend the
Javadoc to clarify that they support possibly null ObjectId instances.

Add new methods #writeWithoutMarker and #readWithoutMarker to support
the cases where the ObjectId is known to be non-null and the marker
should not be written to the serialization stream.

Also:

- Replace the hard-coded `0` and `1` markers with constants that can
  be linked from the Javadocs.

- Include the marker value in the "Invalid flag before ObjectId"
  exception message.

[1] https://gerrit-review.googlesource.com/c/gerrit/+/9792
[2] https://gerrit-review.googlesource.com/c/gerrit/+/165851
[3] https://gerrit-review.googlesource.com/c/gerrit/+/165952

Change-Id: Iaf84c3ec32ecf83efffb306fdb4940cc85740f3f
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ObjectIdSerializerTest.java

@@ -79,10 +79,18 @@ public class ObjectIdSerializerTest {
 			throws Exception {
 		File file = File.createTempFile("ObjectIdSerializerTest_", "");
 		try (OutputStream out = new FileOutputStream(file)) {
-			ObjectIdSerializer.write(out, objectId);
+			if (objectId == null) {
+				ObjectIdSerializer.write(out, objectId);
+			} else {
+				ObjectIdSerializer.writeWithoutMarker(out, objectId);
+			}
 		}
 		try (InputStream in = new FileInputStream(file)) {
-			return ObjectIdSerializer.read(in);
+			if (objectId == null) {
+				return ObjectIdSerializer.read(in);
+			} else {
+				return ObjectIdSerializer.readWithoutMarker(in);
+			}
 		}
 	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdSerializer.java

@@ -51,57 +51,110 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 
+import org.eclipse.jgit.annotations.NonNull;
 import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.util.IO;
 
 /**
  * Helper to serialize {@link ObjectId} instances. {@link ObjectId} is already
- * serializable, but this class provides a more optimal implementation. It
+ * serializable, but this class provides methods to handle null and non-null
- * writes out a flag (0 or 1) followed by the object's words, or nothing if it
+ * instances.
- * was a null id.
  *
  * @since 4.11
  */
 public class ObjectIdSerializer {
+	/*
+	 * Marker to indicate a null ObjectId instance.
+	 */
+	private static final byte NULL_MARKER = 0;
+
+	/*
+	 * Marker to indicate a non-null ObjectId instance.
+	 */
+	private static final byte NON_NULL_MARKER = 1;
+
 	/**
+	 * Write a possibly null {@link ObjectId} to the stream, using markers to
+	 * differentiate null and non-null instances.
+	 *
+	 * <p>
+	 * If the id is non-null, writes a {@link #NON_NULL_MARKER} followed by the
+	 * id's words. If it is null, writes a {@link #NULL_MARKER} and nothing
+	 * else.
+	 *
 	 * @param out
 	 *            the output stream
 	 * @param id
-	 *            the object id to serialize
+	 *            the object id to serialize; may be null
 	 * @throws IOException
 	 *             the stream writing failed
 	 */
 	public static void write(OutputStream out, @Nullable AnyObjectId id)
 			throws IOException {
 		if (id != null) {
-			out.write((byte) 1);
+			out.write(NON_NULL_MARKER);
-			id.copyRawTo(out);
+			writeWithoutMarker(out, id);
 		} else {
-			out.write((byte) 0);
+			out.write(NULL_MARKER);
 		}
 	}
 
 	/**
+	 * Write a non-null {@link ObjectId} to the stream.
+	 *
+	 * @param out
+	 *            the output stream
+	 * @param id
+	 *            the object id to serialize; never null
+	 * @throws IOException
+	 *             the stream writing failed
+	 */
+	public static void writeWithoutMarker(OutputStream out, @NonNull AnyObjectId id)
+			throws IOException {
+		id.copyRawTo(out);
+	}
+
+	/**
+	 * Read a possibly null {@link ObjectId} from the stream.
+	 *
+	 * Reads the first byte of the stream, which is expected to be either
+	 * {@link #NON_NULL_MARKER} or {@link #NULL_MARKER}.
+	 *
 	 * @param in
 	 *            the input stream
-	 * @return the object id
+	 * @return the object id, or null
 	 * @throws IOException
 	 *             there was an error reading the stream
 	 */
 	@Nullable
 	public static ObjectId read(InputStream in) throws IOException {
-		switch (in.read()) {
+		byte marker = (byte) in.read();
-		case 0:
+		switch (marker) {
+		case NULL_MARKER:
 			return null;
-		case 1:
+		case NON_NULL_MARKER:
-			final byte[] b = new byte[OBJECT_ID_LENGTH];
+			return readWithoutMarker(in);
-			IO.readFully(in, b, 0, OBJECT_ID_LENGTH);
-			return ObjectId.fromRaw(b);
 		default:
-			throw new IOException("Invalid flag before ObjectId"); //$NON-NLS-1$
+			throw new IOException("Invalid flag before ObjectId: " + marker); //$NON-NLS-1$
 		}
 	}
 
+	/**
+	 * Read a non-null {@link ObjectId} from the stream.
+	 *
+	 * @param in
+	 *            the input stream
+	 * @return the object id; never null
+	 * @throws IOException
+	 *             there was an error reading the stream
+	 */
+	@NonNull
+	public static ObjectId readWithoutMarker(InputStream in) throws IOException {
+		final byte[] b = new byte[OBJECT_ID_LENGTH];
+		IO.readFully(in, b, 0, OBJECT_ID_LENGTH);
+		return ObjectId.fromRaw(b);
+	}
+
 	private ObjectIdSerializer() {
 	}
 }