[MERGE] log -n/--levels

[Ian Clatworthy]

Vincent Ladeuil wrote:
>>>>>> "Ian" == Ian Clatworthy <ian.clatworthy at internode.on.net> writes:


>     Ian> +    * ``bzr log`` supports a new option called ``--level-count N``.
> 
> Why not just '--levels' as your submission subject said ?

Done.

>     Ian> +            Option('level-count',
>     Ian> +                   short_name='n',
>     Ian> +                   help='Number of levels to display - 0 for all, 1 for flat.',
>     Ian> +                   argname='N',
>     Ian> +                   type=_parse_level_count),
> 
> Can't you just use type=int here ?
> 
> On the other hand you way want to check for negative values
> too...

Maybe. I've been under the impression that type=int doesn't work given
how we wrap the optional handling.

>     Ian> +    def get_levels_to_display(self):
> 
> I'd be happy with a simple 'get_levels' if there was a doc string there.

Done.

>     Ian> +        if getattr(self, 'supports_merge_revisions', False):
>     Ian> +            if self.levels is None or self.levels == -1:
>     Ian> +                return getattr(self, 'preferred_levels', 0)
>     Ian> +            else:
>     Ian> +                return self.levels
>     Ian> +        return 1
>  
>     Ian>  # TODO: uncomment this block after show() has been removed.
>     Ian>  # Until then defining log_revision would prevent _show_log calling show() 
> 
> Isn't it time to address that TODO ?

Done.

>     Ian> @@ -829,11 +859,33 @@
>  
>     Ian>  class ShortLogFormatter(LogFormatter):
>  
>     Ian> +    supports_merge_revisions = True
>     Ian> +    preferred_levels = 1
> 
> I find that very readable. I think you should do the same
> (declare the attribute) for the base class instead of relying on
> the getattr(self, 'preferred_levels', 0) in
> get_levels_to_display().

Done.

>     Ian>      supports_delta = True
>     Ian>      supports_tags = True
>     Ian> -    supports_single_merge_revision = True
>     Ian> +
>     Ian> +    def __init__(self, *args, **kwargs):
>     Ian> +        super(ShortLogFormatter, self).__init__(*args, **kwargs)
>     Ian> +        self.revno_width_by_depth = {}
>  
>     Ian>      def log_revision(self, revision):
>     Ian> +        # We need two indents: one per depth and one for the information
>     Ian> +        # relative to that indent. Most mainline revnos are 5 chars or
>     Ian> +        # less while dotted revnos are typically 9 chars or less. Once
>     Ian> +        # calculated, we need to remember the offset for a given depth
>     Ian> +        # as we might be starting from a dotted revno in the first column
>     Ian> +        # and we want subsequent mainline revisions to line up.
>     Ian> +        depth = revision.merge_depth
>     Ian> +        indent = '    ' * depth
>     Ian> +        revno_width = self.revno_width_by_depth.get(depth)
>     Ian> +        if revno_width is None:
>     Ian> +            if revision.revno.find('.') == -1:
>     Ian> +                revno_width = 5
>     Ian> +            else:
>     Ian> +                revno_width = 9
> 
> wow, wow, wow, real-life example: 2613.137.64 (and I didn't even
> check that this is the widest...)
> 
> Either you take greater margins or you forget about that way to
> line things up... I suppose a two passes approach will not be a
> candidate :-)
> 
> I presume you'd want to stay with fixed numbers so that means at
> least bumping them ?

I bumped the minimum width up to 11 for dotted revnos.

> Final note, that kind of constant can really do with examples in
> comments just before them as in:
> 
>                           # mainline revno: 12345
>     Ian> +                revno_width = 5
>     Ian> +            else:
>                           # dotted revno: 12345.1.3
>     Ian> +                revno_width = 9

Done.

> 
>     Ian> === modified file 'bzrlib/tests/blackbox/test_log.py'
>     Ian> --- bzrlib/tests/blackbox/test_log.py	2009-01-15 14:05:13 +0000
>     Ian> +++ bzrlib/tests/blackbox/test_log.py	2009-01-23 03:36:13 +0000
>     Ian> @@ -256,14 +256,14 @@
>     Ian>      def assertUseShortDeltaFormat(self, cmd):
>     Ian>          log = self.run_bzr(cmd)[0]
>     Ian>          # Check that we use the short status format
>     Ian> -        self.assertContainsRe(log, '(?m)^A  hello.txt$')
>     Ian> -        self.assertNotContainsRe(log, '(?m)^added:$')
>     Ian> +        self.assertContainsRe(log, '(?m)^\s*A  hello.txt$')
>     Ian> +        self.assertNotContainsRe(log, '(?m)^\s*added:$')
>  
> ??
> 
>     Ian>      def assertUseLongDeltaFormat(self, cmd):
>     Ian>          log = self.run_bzr(cmd)[0]
>     Ian>          # Check that we use the long status format
>     Ian> -        self.assertNotContainsRe(log, '(?m)^A  hello.txt$')
>     Ian> -        self.assertContainsRe(log, '(?m)^added:$')
>     Ian> +        self.assertNotContainsRe(log, '(?m)^\s*A  hello.txt$')
>     Ian> +        self.assertContainsRe(log, '(?m)^\s*added:$')
>  

Deltas can now be indented so there may be leading whitespace on
the line preceding the expected text.

Ian C.