Code review comment for lp:~ian-clatworthy/bzr/faster-log.old

John A Meinel wrote:
> I'll try to give a bit of a summary as to why I think this is faster,
> though it would be nice if you would give a similar summary when submitting.
>
>
[snip]
That's an excellent summary - thanks.
> IMO the batching of everything for one revision seems just about right.
>
> I'm a bit concerned, though, about handling Unicode Author names and
> UTF-8 'diff' strings. My guess is that this actually breaks it, and we
> just don't know it yet.
>
> Can you please ensure that we have a test with non-ascii file content
> (even better if it isn't even UTF-8 since we don't assert anything about
> file content encoding), and then also include a non-ascii Author name.
> That ensures that we want to output Unicode for the name, and something
> random for the diff content.
>
I'm confident diff strings are handled correctly because Alexander
submitted a bug fixing this for 1.13. Note that diff strings aren't
written to self.to_file (in the code), but to a wrapped version thereof.

I suspect we do have tests for Unicode authors but I'll confirm that
(probably tomorrow).
> 5) I'm a bit curious about this:
> === modified file 'bzrlib/osutils.py'
> --- bzrlib/osutils.py 2009-05-23 04:55:52 +0000
> +++ bzrlib/osutils.py 2009-05-25 05:46:01 +0000
> @@ -686,6 +686,8 @@
> return offset.days * 86400 + offset.seconds
>
> weekdays = ['Mon', 'Tue', 'Wed', 'Thu', 'Fri', 'Sat', 'Sun']
> +_default_format_by_weekday_num = [wd + " %Y-%m-%d %H:%M:%S" for wd in
> weekdays]
> +
>
> ^- Does this mean that we are skipping localization? So someone with
> de_DE.UTF-8 will no longer see the German short-names for days of the week?
>
Yes it does. There's a separate function - format_local_date IIRC - that
doesn't use English day names. I'll run annotate and find out why we're
doing this but, at a wild guess, it might be related to ensuring tests
pass regardless of locale?

FWIW, I'm *not* changing current behaviour here. If we what to
change/fix the code, I'd like to make that a separate patch if that's OK.

Ian C.