Re: [JGIT PATCH] 1/2: Externalizable items

["Shawn O. Pearce" <spearce@spearce.org>]

Nigel Magnay <nigel.magnay@gmail.com> wrote:
> Make parts of jgit externalizable, so that they can be marshalled over
> the wire or onto disk,
> using formats from git mailing list.

As Dscho pointed out, a bit more detail here would be appreciated.
 
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectId.java
> b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectId.java
> index 52ce0d4..1385325 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectId.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectId.java
> @@ -56,6 +60,13 @@
>  	}
> 
>  	/**
> +	 * Empty constructor, for Externalizable.
> +	 */
> +	public ObjectId() {
> +		// For Externalizable
> +	}

Yikes.  Do we really need a public no-arg constructor for
Externalizable?  If we do, maybe we should use Serializable instead
so we can hide this constructor.  I don't like the idea of people
creating ObjectId.zeroId() by new ObjectId().  That's not a pattern
we should encourage.

> @@ -269,4 +280,22 @@ protected ObjectId(final AnyObjectId src) {
>  	public ObjectId toObjectId() {
>  		return this;
>  	}
> +
> +	public void readExternal(ObjectInput in) throws IOException,
> +			ClassNotFoundException {
> +		byte[] sha1 = new byte[20];
> +		in.read(sha1);
> +		
> +		w1 = NB.decodeInt32(sha1, 0);
> +		w2 = NB.decodeInt32(sha1, 4);
> +		w3 = NB.decodeInt32(sha1, 8);
> +		w4 = NB.decodeInt32(sha1, 12);
> +		w5 = NB.decodeInt32(sha1, 16);
> +	}
> +
> +	public void writeExternal(ObjectOutput out) throws IOException {
> +		byte[] sha1 = new byte[20];
> +		copyRawTo(sha1, 0);
> +		out.write(sha1);
> +	}

Hmm.  I was thinking of just writing the 5 ints out, and reading
the 5 ints back in.  We're always talking to another Java process.
The ints are written in network byte order anyway on a serialization
stream.  Doing this conversion to a byte[] thrases the caller's
per-thread new generation rather hard.  I think applications using
this type in a serialization stream would expect it to be quick.

> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java
> b/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java
> index 5bbf664..22443b4 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java
> +
> +	public void readExternal(ObjectInput in) throws IOException,
> +			ClassNotFoundException {
> +		name = in.readUTF();
> +		int items = in.readInt();
> +
> +		Map<String, Collection<String>> map = new HashMap<String,
> Collection<String>>();
> +		for (int i = 0; i < items; i++) {
> +			String key = in.readUTF();
> +			String value = in.readUTF();

Why not just serialize the Map in the stream?

-- 
Shawn.