Code review comment for lp:~amanica/bzr/log_returns_too_much

>>>>> "amanica" == Marius Kruger <email address hidden> writes:

    amanica> Marius Kruger has proposed merging
    amanica> lp:~amanica/bzr/log_returns_too_much into lp:bzr.

Thanks a lot for working on that !

    amanica> Requested reviews:
    amanica> bzr-core (bzr-core)

    amanica> Fixed this by adding a stop condition to
    amanica> _DefaultLogGenerator.iter_log_revisions()

    amanica> It can't be done at a lower level because some of
    amanica> the other filters depend on having all the
    amanica> log-revisions' parents.

    amanica> This causes a behaviour change as explained in the bug:
    amanica> doing a `bzr log -n0 -r 2345` would no longer return its parents eg 2338.3.1

Hurrah !

This has bugged me for a long time. I'm pretty sure it's related
(but not only) to 3936.3.30, could you have a look and share your
thoughts ?

<snip/>

    amanica> === modified file 'bzrlib/log.py'
    amanica> --- bzrlib/log.py 2009-06-10 03:56:49 +0000
    amanica> +++ bzrlib/log.py 2009-07-07 20:22:22 +0000
    amanica> @@ -400,6 +400,14 @@
    amanica> log_count += 1
    amanica> if log_count >= limit:
    amanica> return
    amanica> + # we need to filter the revisions here again since
    amanica> + # the lower levels also return the revision parents
    amanica> + if rqst.get('direction') == 'forward':
    amanica> + if self.end_rev_id == rev_id:
    amanica> + return
    amanica> + else:
    amanica> + if self.start_rev_id == rev_id:
    amanica> + return

Can you add the bug number in that comment as a reminder ? Your
rationale sounds good, we may want to revisit that part in the
future (Ian intended to push his design further but delayed it
post-2.0).

    amanica> === modified file 'bzrlib/tests/blackbox/test_log.py'
    amanica> --- bzrlib/tests/blackbox/test_log.py 2009-06-10 03:56:49 +0000
    amanica> +++ bzrlib/tests/blackbox/test_log.py 2009-07-07 01:13:29 +0000
    amanica> @@ -277,7 +277,7 @@
    amanica> # tags don't propagate if we don't merge
    amanica> self.run_bzr('merge ../branch1', working_dir='branch2')
    amanica> branch2_tree.commit(message='merge branch 1')
    amanica> - log = self.run_bzr("log -n0 -r-1", working_dir='branch2')[0]
    amanica> + log = self.run_bzr("log -n0 -r-2..-1", working_dir='branch2')[0]
    amanica> self.assertContainsRe(log, r' tags: tag1')
    amanica> log = self.run_bzr("log -n0 -r3.1.1", working_dir='branch2')[0]
    amanica> self.assertContainsRe(log, r'tags: tag1')

That sounds wrong, can you explain the link with your
modification ? Is that because before your fix an additional
revision was output ?

If that's true then your fix is *really* needed and demonstrates
that 'log' was definitely not correct. If it's not, it be good to
understand what happened here.

I feel convinced by your rationale and the test modifications,
except for the one noted above for which I'd like an explanation,
otherwise I'd like Ian to be the second reviewer here.

reviewer ian-clatworthy
review approve