Merge lp:~amanica/bzr/log_returns_too_much into lp:~bzr/bzr/trunk-old

[Marius Kruger]

[Marius Kruger (amanica)]

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

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

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

[Vincent Ladeuil (vila)]

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

[Vincent Ladeuil (vila)]

    vila> reviewer ian-clatworthy
    vila> review approve

Hmpf,

 must start the command line with a space
 must start the command line with a space
 must start the command line with a space
 must start the command line with a space
 must start the command line with a space
 must start the command line with a space
 must start the command line with a space
 must start the command line with a space
 must start the command line with a space
 must start the command line with a space

 reviewer ian-clatworthy
 review approve

review: Approve

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
>>>>>> "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 ?
>

So here are my thoughts...

If we are going to do this then:

1) "bzr log -r 10" should show just the revision 10
2) "bzr log -c 10" should show all the changes that were merged into 10
3) "bzr log -r 9..11" should *not* show the revision for 9, but show
everything that was merged into 10 and 11.

Basically, we would unify it with similar commands (like diff) which for
"-r X..Y" mean "show what is in Y that is not in X".

John
=:->

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

iEYEARECAAYFAkpV+UAACgkQJdeBCYSNAAMtoACfcNBx8FNZ14hzpSFCIX1YWiNz
kJcAnj5aW/RNqVIAp1MZaPl6JYhsgMmZ
=8wKN
-----END PGP SIGNATURE-----

[Marius Kruger (amanica)]

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

[lp:~amanica/bzr/log_returns_too_much]

4517. By Marius Kruger on 2009-07-10

update comment as per review

[Marius Kruger (amanica)]

2009/7/9 John A Meinel <email address hidden>
>
> So here are my thoughts...
thanks

> If we are going to do this then:
>
> 1) "bzr log -r 10" should show just the revision 10

that is what this patch does

> 2) "bzr log -c 10" should show all the changes that were merged into 10

sounds like a must have feature otherwise its really difficult to
figure out what happened
(I resorted to bzr explorer & qlog to be able to successfully navigate this)
bzr diff seems to work that way too.

> 3) "bzr log -r 9..11" should *not* show the revision for 9, but show
> everything that was merged into 10 and 11.
>
> Basically, we would unify it with similar commands (like diff) which for
> "-r X..Y" mean "show what is in Y that is not in X".

That is https://bugs.launchpad.net/bzr/+bug/320119
which was more or less my next step.

I like your suggestions, it would make log&diff more consistent.
I'm keen to implement them as soon as time permits.

[Robert Collins (lifeless)]

On Thu, 2009-07-09 at 14:06 +0000, John A Meinel wrote:

> So here are my thoughts...
>
> If we are going to do this then:
>
> 1) "bzr log -r 10" should show just the revision 10
> 2) "bzr log -c 10" should show all the changes that were merged into 10
> 3) "bzr log -r 9..11" should *not* show the revision for 9, but show
> everything that was merged into 10 and 11.
>
> Basically, we would unify it with similar commands (like diff) which for
> "-r X..Y" mean "show what is in Y that is not in X".

+1

--

[Martin Pool (mbp)]

2009/7/10 Robert Collins <email address hidden>:
> On Thu, 2009-07-09 at 14:06 +0000, John A Meinel wrote:
>
>> So here are my thoughts...
>>
>> If we are going to do this then:
>>
>> 1) "bzr log -r 10" should show just the revision 10
>> 2) "bzr log -c 10" should show all the changes that were merged into 10
>> 3) "bzr log -r 9..11" should *not* show the revision for 9, but show
>> everything that was merged into 10 and 11.
>>
>> Basically, we would unify it with similar commands (like diff) which for
>> "-r X..Y" mean "show what is in Y that is not in X".
>
> +1

+1

--
Martin <http://launchpad.net/~mbp/>

[lp:~amanica/bzr/log_returns_too_much]

4518. By Marius Kruger on 2009-07-19

merge with bzr.dev. no conflicts. put news in its place.

Unmerged revisions

4518. By Marius Kruger on 2009-07-19

merge with bzr.dev. no conflicts. put news in its place.

4517. By Marius Kruger on 2009-07-10

update comment as per review

4516. By Marius Kruger on 2009-07-07

add news

4515. By Marius Kruger on 2009-07-07

when we go forward we need to stop at the end-revision not the start-revision

4514. By Marius Kruger on 2009-07-07

use Logger.start_rev_id in stead of reading/checking is again

4513. By Marius Kruger on 2009-07-07

stop logging at the stop-revision, and start updating the tests for it

4512. By Marius Kruger on 2009-07-06

add test_merges_partial_range_ignore_before_lower_bound

=== modified file 'NEWS'
--- NEWS	2009-07-09 01:44:00 +0000
+++ NEWS	2009-07-10 00:35:27 +0000
@@ -68,6 +68,9 @@
   does not already have a control directory.  The flag ``--use-existing-dir``
   will allow operation to proceed.  (Alexander Belchenko, #307554)
 
+* ``bzr log -r123`` should not return 123's parents too.
+  (#325618, Marius Kruger)
+
 * ``bzr ls DIR --from-root`` now shows only things in DIR, not everything.
   (Ian Clatworthy)
 

=== modified file 'bzrlib/log.py'
--- bzrlib/log.py	2009-06-10 03:56:49 +0000
+++ bzrlib/log.py	2009-07-10 00:35:27 +0000
@@ -400,6 +400,17 @@
                     log_count += 1
                     if log_count >= limit:
                         return
+                # As per Bug #325618 we don't want to show parent revisions
+                # outside of the specified range.
+                # So we need to filter the revisions here again since
+                # the lower levels also return the revision parents,
+                # which is needed for filtering on file_ids.
+                if rqst.get('direction') == 'forward':
+                    if self.end_rev_id == rev_id:
+                        return
+                else:
+                    if self.start_rev_id == rev_id:
+                        return
 
     def _format_diff(self, rev, rev_id):
         diff_type = self.rqst.get('diff_type')

=== modified file 'bzrlib/tests/blackbox/test_log.py'
--- bzrlib/tests/blackbox/test_log.py	2009-06-10 03:56:49 +0000
+++ bzrlib/tests/blackbox/test_log.py	2009-07-10 00:35:27 +0000
@@ -277,7 +277,7 @@
         # tags don't propagate if we don't merge
         self.run_bzr('merge ../branch1', working_dir='branch2')
         branch2_tree.commit(message='merge branch 1')
-        log = self.run_bzr("log -n0 -r-1", working_dir='branch2')[0]
+        log = self.run_bzr("log -n0 -r-2..-1", working_dir='branch2')[0]
         self.assertContainsRe(log, r'    tags: tag1')
         log = self.run_bzr("log -n0 -r3.1.1", working_dir='branch2')[0]
         self.assertContainsRe(log, r'tags: tag1')
@@ -500,13 +500,6 @@
 timestamp: Just now
 message:
   merge branch level2
-    ------------------------------------------------------------
-    revno: 1.2.1
-    committer: Lorem Ipsum <[e-mail]>
-    branch nick: level2
-    timestamp: Just now
-    message:
-      in branch level2
 """
         self.check_log(expected, ['-n0', '-r1.1.2'])
 
@@ -536,6 +529,18 @@
 """
         self.check_log(expected, ['-n0', '-r1.1.1..1.1.2'])
 
+    def test_merges_partial_range_ignore_before_lower_bound(self):
+        """Dont show revisions before the lower bound"""
+        expected = """\
+    2 Lorem Ipsum\t2005-11-22 [merge]
+      merge branch level1
+
+          1.1.2 Lorem Ipsum\t2005-11-22 [merge]
+                merge branch level2
+
+"""
+        self.check_log(expected, ['--short', '-n0', '-r1.1.2..2'])
+
 
 class TestLogDiff(TestLog):
 
@@ -876,7 +881,7 @@
         self.assertNotContainsRe(log, 'revno: 1\n')
         self.assertNotContainsRe(log, 'revno: 2\n')
         self.assertNotContainsRe(log, 'revno: 3\n')
-        self.assertContainsRe(log, 'revno: 3.1.1\n')
+        self.assertNotContainsRe(log, 'revno: 3.1.1\n')
         self.assertContainsRe(log, 'revno: 4 ')
         log = self.run_bzr('log -n0 -r3.. file2')[0]
         self.assertNotContainsRe(log, 'revno: 1\n')