[MERGE] left align log output if it only contains merge revisions

[Kent Gibson]

John Arbash Meinel wrote:
> John Arbash Meinel has voted +1 (conditional).
> Status is now: Semi-approved
> Comment:
> I actually misunderstood what Kent was doing originally. But why are
> you doing:
> +        depths = list(set([d for r,n,d in view_revisions]))
> +        depths.sort()
> +        min_depth = depths[0]
>
> rather than:
> min_depth = min(d for r,n,d in view_revisions)
>

Cos I keep forgetting min can be applied to lists.  I vaguely recall
having a
discussion with you on this before, but obviously it didn't stick.

> I don't see any reason to go through a set and a sort just to
> extract the minimum of a list. (I assume you used a set because you
> didn't want to sort the full list, but the built in 'min()' function
> is quite a bit better).
>
> I would also prefer:
>
> if min_depth != 0:
>
> to
> if min_depth:
>
> I realize they are the same thing, but for me the former is a lot
> more obvious of what you are trying to check. (There isn't any
> reason that min_depth would be None or False).
>

Fair enough.  I tend to go for the approach that requires less typing,
but in this case the explicit comparison is more readable.

>
> As for flushing left or not... I'm starting to lean more into the
> 'it is wasting space on the left', rather than specific significance
> for mainline revs. The dotted revnos would probably make it clear,
> and this only actually takes effect when you are obviously
> displaying a merged branch.
>

Updated bundle with your suggestions attached.
There are a couple of minor conflicts when it is merged with bzr.dev
(NEWs, of course, and one conflict with vila's blackbox test cleanup.) 
I hope you don't mind that I've left those.

I'd also like to clean up the log tests a bit more, e.g. move the merge
indentation tests from blackbox into the internal unit tests, and
replace the asserts in the blackbox tests with log normalization as I
already have in the internal tests, but I'd like to do that independent
of this patch.

> I really wonder how this code handles:
>   bzr log -r 123.4.5..123.7.8
>

It effectively takes a slice of a full bzr log with those revisions as
the limits.

Cheers,
Kent.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: la2.patch
Type: text/x-patch
Size: 18596 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070707/bb2423c7/attachment.bin