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

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

Nigel Magnay <nigel.magnay@gmail.com> wrote:
> > 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.
> >
> 
> Yes, you have to have a public no-args constructor for Externalizable.
> 
>  I agree, it's hideous. But I thought that was known as you explicitly
> asked for Externalizable rather than Serializable with readObject /
> writeObject... :-/

I forgot/didn't remember that Externalizable has this hideous
requirement.  I'd rather not introduce a public no-arg constructor
just for Java serialization.  So yea, if you needed to add a
constructor than we should instead use Serializable and define an
explicit serialVersionUID.  Sorry.
 
> More than happy to re-roll with Serializable instead - do you want
> this for all 4? (RemoteConfig also gained a no-args constructor
> because of Externalizable..)

Yea, at least for ObjectId and RemoteConfig.
 
> > +             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?
> 
> Sure - if you're happy with that representation - it's not " a map of
> keys/values as it appears in the config " though as it's a map to a
> list because of the multi-values that are available for things like
> URL and Fetch.

Right.

The format of HashMap and ArrayList is pretty stable on the wire,
so we can depend on them not changing on us.  We might as well
just serialize with those and save ourselves some code in the JGit
library.  I don't see a compelling reason here to use our own map
format on the wire.  Especially not if we are doing this conversion
to a java.util.Map<String,java.util.List> just to dump to the wire.

On the other hand, you could modify the read and write code to do
direct writing of string pairs, and direct reading of string pairs,
that would save allocs in each direction and make RemoteConfig
that much easier to write out.  Of course to do that you can't use
a count header, but instead would want a sential marker to break
the reader out of its loop, like an empty key string.

-- 
Shawn.