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

[Nigel Magnay <nigel.magnay@gmail.com>]

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

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

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

I've taken the request for "the 20 byte SHA-1" too literally :-)

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