[MERGE] faster log - hooray!

[John Arbash Meinel]

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

Ian Clatworthy wrote:
...

> I didn't dig deep when the class matching failed. I simply jumped to
> the conclusion that the wrapper/lazy-delegate class was getting in
> the way, made the change to explicit importing and it all worked. :-(

Right, the specific issue is that doing:

lazy_import(globals(), """
from foo import bar
""")
...
if x == bar:
  ...

At this point, we have not accessed any attributes of bar, nor given it
any indication we have touched it. So the lazy object doesn't know it
needs to trigger. I worked hard such that doing "bar()" or "bar.X", etc
all trigger an import. I probably shouldn't have tried so hard.

Specifically, doing:

from module import klass

klass()

Works from a lazy import, because I override __call__. However

isinstance(foo, klass)
and
foo.__class__ == klass

Doesn't work because there is no attribute access I can trigger on.

Anyway, if we just stick to lazy importing *modules* we don't have any
problems. Because modules aren't useful on their own. You always (99.9%)
access an attribute.


...

...

...


>> +    else:
>> +        # We're following a development line starting at a merged revision.
>> +        # We need to adjust depths down by the initial depth until we find
>> +        # a depth less than it. Then we use that depth as the adjustment.
>> +        # If and when we reach the mainline, depth adjustment ends.
>> +        depth_adjustment = None
>> +        for (rev_id, merge_depth, revno, end_of_merge
>> +             ) in view_revisions:
>> +            if depth_adjustment is None:
>> +                depth_adjustment = merge_depth
>> +            if depth_adjustment:
>> +                if merge_depth < depth_adjustment:
>> +                    depth_adjustment = merge_depth
>> +                merge_depth -= depth_adjustment
>> +            yield rev_id, '.'.join(map(str, revno)), merge_depth
>>
>> ^- I think this actually needs to be a 2-pass arrangement. Where you
>> compute the overall depth_adjustment, and then adjust everything by that
>> value. Otherwise I think if you did:
>>
>> bzr log --long -r 1..1.2.3
>>
>> It would start logging 1.2.3 with a depth_adjustment of N, but then
>> finish logging mainline with a depth-adjustment of 0. Or was that the
>> specific goal? (logging it as though 1.2.3 was the mainline, but using
>> the numbering from the current branch tip?)
> 
> My explicit goal is that logging -r 1..1.2.3 will show 1.2.3 at depth 0,
> it's merged revisions nested appropriately. We're genuinely treating
> 1.2.3 as the "tip" of the development line we're logging here. Using
> log --short -r1..1.2.3 will show that development line as flat and
> log --long -r1..1.2.3 should show the same line with any nested
> revisions below each merge.
> 
> A multiple pass algorithm isn't required IMO. It also scales O(result)
> eliminating any chance of incremental output.

Sure, though I'll mention that this is a very concrete UI change, and
should be documented appropriately. I don't have any specific issues
with it, as I don't really care.

> 
>> +def calculate_view_revisions(branch, start_revision, end_revision,
>> direction,
>> +        specific_fileid, generate_merge_revisions,
>> allow_single_merge_revision):
>> +    """Calculate the revisions to view.
>> +
>> +    :return: An iterator of (revision_id, dotted_revno, merge_depth)
>> tuples OR
>> +             a list of the same tuples.
>> +    """
>> +    # This method is no longer called by the main code path.
>> +    # It is retained for API compatibility and may be deprecated
>> +    # soon. IGC 20090116
>>
>> ^- If we aren't using it anymore, I'd rather deprecate it now rather
>> than waiting.
> 
> I can't just yet because I'm pretty sure its used inside bzrlib for
> missing.py say. We also need to replace it with a public API we *do*
> want to support for bzrlib clients. There's a preview of what I'm
> thinking of (LogGenerator.iter_log_revisions()) in my log-dir branch
> above BTW, but that requires some debate with others (e.g. Vincent)
> and some evaluation by potential users like LoggerHead and qlog before
> it's made public. Oh, and dozens and dozens of interface tests. :-)
> 
> I'll be sure to raise this in the covering letter for my log-dir patch.
> 

The only instance of "calculate_view_revisions" I find in bzrlib is the
definition. I believe missing uses its own code (iter_log_revisions(),
IIRC). And a call in a test case.


>>      def test_merges_nonsupporting_formatter(self):
>> +        # This "feature" of log formatters is madness. If a log
>> +        # formatter cannot display a dotted-revno, it ought to ignore it.
>> +        # Otherwise, a linear sequence is always expected to be handled
>> now.
>> +        raise KnownFailure('log formatters must support linear
>> sequences now')
>>
>> ^- I don't understand this change.
> 
> The new code is less dumb than the old code so an overly restrictive test
> now fails. I need to ping the author that introduced this "feature" to
> see if his/her log formatter can be changed to make this (to me) weird
> limitation disappear. I can only guess that the driver was something like
> wanting a fixed-width revno column and the old dotted scheme (1.1.1.1.1)
> caused problems for logs of size > 1. Anyhow, removing this is on my
> list, just not near the top yet.

I believe the specific concern was that doing "bzr log --short -r 1.1.1"
used to raise a ValueError/IndexError exception without a nice message.
If we can show the revision now, then I think we can just nuke the whole
test.

> 
> Once again, thanks for the insightful review. Apologies in sending this
> delayed response.
> 
> Ian C.
> 

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

iEYEARECAAYFAkmJoycACgkQJdeBCYSNAAOZLQCeKPle7jWNOY9+aMzseZpvE8R0
ZE4AoMJIflE5afTap6lffYIOxEN5SJOK
=pXD0
-----END PGP SIGNATURE-----