[MERGE] Fix bencoding of bools, provide a bdecode_tuple function.

[Andrew Bennetts]

John Arbash Meinel wrote:
> John Arbash Meinel has voted tweak.
> Status is now: Conditionally approved
> Comment:
> So this is ok, though there are a few bits to consider
>
> 1) Copyright. Not sure exactly what to do here, but the code was written  
> by Petru Paler, and this is a fairly large reorganization. Reading over  
> it, I almost wonder if it is fully GPL compatible just because of this  
> clause:
> # The above copyright notice and this permission notice shall be
> # included in all copies or substantial portions of the Software.
>
> Though I think that one is ok, and it is the one about advertising using  
> the code that the GPL doesn't like about BSD 4-clause.
>
> Anyway, we don't have to do anything here, but I at least wondered about  
> it.

Well, we should at least mention that we've modified the file, rather than
leaving the impression that Petru Paler is to blame for my changes :)

So I've added this to the end of the comments at the top of the file:

    #
    # Modifications copyright (C) 2008 Canonical Ltd

It is a bit odd to be modifying code in bzrlib/util.  Perhaps we should
bring it into the main tree?

> 2) Having Bools encoded as "i1" is good, though you don't get your bools  
> back out again, which is a bit of a shame.

A little bit, but it's compatible with other bencode implementations (and
I've noticed that other bencode.py modules in other projects serialise bools
as ints too).  So I think this is the best option.

> 3) I was a bit surprised that you used an "if self.yield_tuples" inside  
> the "decode_list" function, rather than having a separate  
> "decode_list_as_tuple()" function, which got inserted into the  
> dictionary when "yield_tuples=True". I don't think it matters, just was  
> more what I was expecting.

I guess “if self.yield_tuples” is what I was expecting ;)

I think defining two seperate functions is going to be more code, so I'm
going to leave it as is.

> 4) Would we want to use __slots__ on this object, since it seems we only  
> have a couple attributes, but we access them often.

__slots__ is really just a memory saving device, and we don't construct many
BDecoders.  In fact, IIRC __slots__ tends to slow down attribute access
slightly.

> 5) As for other names "bdecode" is fairly obvious, but "bdecode_tuple"  
> sounds like it is decoding a single tuple. Perhaps "bdecode_as_tuples"?  

Ok, done.

> Or maybe a different formulation entirely "bdecode(X, as_tuples=True)".

Hmm, much of a muchness I think.  I'll leave it as is.  This is the sort of
thing that's fairly easy to change later in the unlikely event that it turns
out matter.

> I think your patch is ok, and it is fine if you want to merge it as is.  
> I just wanted to offers some other things that I thought of, which would  
> be a different way of accomplishing this (might be cleaner/clearer,  
> might not).

That's a lot of what reviews are for :)

-Andrew.