[merge] Bundles handle non-integer timezones

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Aaron Bentley wrote:
> John Arbash Meinel wrote:
>>> Attached is a patch to handle 2 bugs in the current bundle code.
>>>
>>> 1) More graceful handling of 'bzr pull sftp://host/path' (no trailing
>>> slash). I submitted this a while ago, but haven't gotten any feedback.
> 
> The implementation stuff looks okay, but the mutter lines look longer
> than 79 chars.  I'm torn about whether the serializer should convert the
> SFTP error into a NotABundle.  I guess it's fine where it is.

What would you prefer. We need something or you get:

$ bzr merge sftp://host/path/to/branch
bzr: ERROR: Failure
$ bzr pull sftp://host/path/to/branch
bzr: ERROR: Failure

*Very* unhelpful.

I'm open to suggestions, as to alternatives to make this better.

> 
>>> 2) Handle timezones that are not integer hour differences. This involves
>>> 1 test to show that unpack_highres_date is doing the right thing
>>> (doctest), and 1 test to prove that bundle is using the right function
>>> (there used to be 2 functions).
> 
> 
> +            mutter('bundle date %s => %s, %s', self.date,
> self.timestamp, self.timezone)
> 
> This is the kind of thing that looks like >79 chars.
> 
> 

They weren't supposed to stay in. I've removed them.

...


> -    offset = int(offset / 100) * 3600 + offset % 100
> +    offset = int(offset / 100) * 3600 + (offset%100) * 60
> 
> It might be clearer to do
> 
> hours = int(offset / 100)
> minutes = int(offset % 100)
> offset = hours * 3600 + minutes * 60

Sure.

> 
> Either way, some spaces around the % would be welcome.  Note also that
> the int around offset/100 is redundant.
> 
> +1 with either spaces around the % or hours/minutes.
> 
> Aaron

I realize it is redundant, but I think it is reasonable to make it clear
that I am counting on the integer division truncation effect.

I suppose I could also do: hours = offset // 100
Which is also defined as integer division even with float arguments:
>>> 4.0 // 10.0
0.0

However, that isn't necessarily obvious to a user what is going on. '//'
is pretty rare. And to a casual glance makes people think it is a comment.

int() of an integer should be fast, and I think helps code clarity.

Thanks for the review, I'll clean up the function and submit.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.2 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFEsuKgJdeBCYSNAAMRAhSYAKCvi9Oaz6AnoAZKF2iLQg0huTw3SgCfVD4V
Uk24iaVBPxG+J7i9vryy/dI=
=5/OV
-----END PGP SIGNATURE-----