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

2009/7/9 Vincent Ladeuil <email address hidden>
>
> >>>>> "amanica" == Marius Kruger <email address hidden> writes:
>
> Thanks a lot for working on that !

cool, np

>    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 ?

uhm, I don't know all the history but it could be that the code path
change in this
revision changed what you saw log returning.

So the reason why we need to do it at this level is that
in order to filter on the files we need all the merges,
but on a higher level we can choose to hide those merge again
after we do that filtering.
(I updated the comment now to explain this better)

(bzr diff -c 3936.3.30 --diff-options="-Fdef.*(" was usefull here)

def _log_revision_iterator_using_per_file_graph(self):
        # Get the base revisions, filtering by the revision range.
        # Note that we always generate the merge revisions because
        # filter_revisions_touching_file_id() requires them ...

calls vvv

def _generate_all_revisions(branch, start_rev_id, end_rev_id, direction,
    delayed_graph_generation):
    # On large trees, generating the merge graph can take 30-60 seconds
    # so we delay doing it until a merge is detected, incrementally
    # returning initial (non-merge) revisions while we can.

calls vvv

def _graph_view_revisions(branch, start_rev_id, end_rev_id,
    rebase_initial_depths=True):
    """Calculate revisions to view including merges, newest to oldest.

calls vvv

    def iter_merge_sorted_revisions(self, start_revision_id=None,
            stop_revision_id=None, stop_rule='exclude', direction='reverse'):
        """Walk the revisions for a branch in merge sorted order.

>    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 ?

done

>
>    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 ?

well here line r-1 is r4 and r-2 is r3, but the tag is on r3.1.1
previously log -n0 -r-1 would also return r3.1.1
so I had to add more revisions so that it can still find the tag,
since -r3.1.1 is tested below I thought this is fine.
I could have done `log -n0 -r3.1.1..-1` or `log -n0 -r3.1.1..`
Since this test is to see if merges display tags, I don't think
the revision filtering is too important.

Let me know if you want this changed, I'll wait for your feedback
before I re-submit this (for review).

> 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.

thanks for the review!