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

Proposed by Marius Kruger on 2009-07-07
Status: Superseded
Proposed branch: lp:~amanica/bzr/log_returns_too_much
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 90 lines
To merge this branch: bzr merge lp:~amanica/bzr/log_returns_too_much
Reviewer Review Type Date Requested Status
Vincent Ladeuil 2009-07-07 Approve on 2009-07-09
Ian Clatworthy 2009-07-09 Pending
Review via email: mp+8349@code.launchpad.net

This proposal has been superseded by a proposal from 2009-07-10.

To post a comment you must log in.
Marius Kruger (amanica) wrote :

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) wrote :
Download full text (3.2 KiB)

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

Read more...

Vincent Ladeuil (vila) wrote :

    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) wrote :

-----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) wrote :
Download full text (5.0 KiB)

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

Read more...

lp:~amanica/bzr/log_returns_too_much updated on 2009-07-10
4517. By Marius Kruger on 2009-07-10

update comment as per review

Marius Kruger (amanica) wrote :

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) wrote :

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) wrote :

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 updated on 2009-07-19
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

Updating diff...

An updated diff will be available in a few minutes. Reload to see the changes.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-07-09 01:44:00 +0000
3+++ NEWS 2009-07-10 00:35:27 +0000
4@@ -68,6 +68,9 @@
5 does not already have a control directory. The flag ``--use-existing-dir``
6 will allow operation to proceed. (Alexander Belchenko, #307554)
7
8+* ``bzr log -r123`` should not return 123's parents too.
9+ (#325618, Marius Kruger)
10+
11 * ``bzr ls DIR --from-root`` now shows only things in DIR, not everything.
12 (Ian Clatworthy)
13
14
15=== modified file 'bzrlib/log.py'
16--- bzrlib/log.py 2009-06-10 03:56:49 +0000
17+++ bzrlib/log.py 2009-07-10 00:35:27 +0000
18@@ -400,6 +400,17 @@
19 log_count += 1
20 if log_count >= limit:
21 return
22+ # As per Bug #325618 we don't want to show parent revisions
23+ # outside of the specified range.
24+ # So we need to filter the revisions here again since
25+ # the lower levels also return the revision parents,
26+ # which is needed for filtering on file_ids.
27+ if rqst.get('direction') == 'forward':
28+ if self.end_rev_id == rev_id:
29+ return
30+ else:
31+ if self.start_rev_id == rev_id:
32+ return
33
34 def _format_diff(self, rev, rev_id):
35 diff_type = self.rqst.get('diff_type')
36
37=== modified file 'bzrlib/tests/blackbox/test_log.py'
38--- bzrlib/tests/blackbox/test_log.py 2009-06-10 03:56:49 +0000
39+++ bzrlib/tests/blackbox/test_log.py 2009-07-10 00:35:27 +0000
40@@ -277,7 +277,7 @@
41 # tags don't propagate if we don't merge
42 self.run_bzr('merge ../branch1', working_dir='branch2')
43 branch2_tree.commit(message='merge branch 1')
44- log = self.run_bzr("log -n0 -r-1", working_dir='branch2')[0]
45+ log = self.run_bzr("log -n0 -r-2..-1", working_dir='branch2')[0]
46 self.assertContainsRe(log, r' tags: tag1')
47 log = self.run_bzr("log -n0 -r3.1.1", working_dir='branch2')[0]
48 self.assertContainsRe(log, r'tags: tag1')
49@@ -500,13 +500,6 @@
50 timestamp: Just now
51 message:
52 merge branch level2
53- ------------------------------------------------------------
54- revno: 1.2.1
55- committer: Lorem Ipsum <test@example.com>
56- branch nick: level2
57- timestamp: Just now
58- message:
59- in branch level2
60 """
61 self.check_log(expected, ['-n0', '-r1.1.2'])
62
63@@ -536,6 +529,18 @@
64 """
65 self.check_log(expected, ['-n0', '-r1.1.1..1.1.2'])
66
67+ def test_merges_partial_range_ignore_before_lower_bound(self):
68+ """Dont show revisions before the lower bound"""
69+ expected = """\
70+ 2 Lorem Ipsum\t2005-11-22 [merge]
71+ merge branch level1
72+
73+ 1.1.2 Lorem Ipsum\t2005-11-22 [merge]
74+ merge branch level2
75+
76+"""
77+ self.check_log(expected, ['--short', '-n0', '-r1.1.2..2'])
78+
79
80 class TestLogDiff(TestLog):
81
82@@ -876,7 +881,7 @@
83 self.assertNotContainsRe(log, 'revno: 1\n')
84 self.assertNotContainsRe(log, 'revno: 2\n')
85 self.assertNotContainsRe(log, 'revno: 3\n')
86- self.assertContainsRe(log, 'revno: 3.1.1\n')
87+ self.assertNotContainsRe(log, 'revno: 3.1.1\n')
88 self.assertContainsRe(log, 'revno: 4 ')
89 log = self.run_bzr('log -n0 -r3.. file2')[0]
90 self.assertNotContainsRe(log, 'revno: 1\n')