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

From: Shawn O. Pearce <spearce@spearce.org>
Date: Feb 16, 2009, 17:20 UTC
Message-ID: <20090216172025.GE18525@spearce.org>
In-Reply-To: <320075ff0902160845m264f78cdh8dc5307b24f4c3ed@mail.gmail.com>

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.

Show 14 quoted lines

> 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.

Show 22 quoted lines

> @@ -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.

Show 16 quoted lines

> 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.