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

Proposed by Marius Kruger on 2009-07-10
Status: Rejected
Rejected by: Vincent Ladeuil on 2009-12-18
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
John A Meinel Approve on 2009-07-10
bzr-core 2009-10-06 Pending
Ian Clatworthy 2009-07-10 Pending
Review via email: mp+8538@code.launchpad.net

This proposal supersedes a proposal from 2009-07-07.

To post a comment you must log in.
Marius Kruger (amanica) wrote : Posted in a previous version of this proposal

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 : Posted in a previous version of this proposal
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 : Posted in a previous version of this proposal

    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 : Posted in a previous version of this proposal

-----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 : Posted in a previous version of this proposal
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...

Marius Kruger (amanica) wrote : Posted in a previous version of this proposal

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 : Posted in a previous version of this proposal

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 : Posted in a previous version of this proposal

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

John A Meinel (jameinel) wrote :

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

Marius Kruger wrote:
> Marius Kruger has proposed merging lp:~amanica/bzr/log_returns_too_much into lp:bzr.
>
> Requested reviews:
> Ian Clatworthy (ian-clatworthy)
>

  review approve

Since this is breaking the compatibility, as long as it is a step
towards where we want to be for log, I'm happy to see it land. Though I
would probably wait for 1.18.

(I don't really want to see us break the meaning of 'bzr log -r X..Y'
two times in two releases back-to-back.)

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

iEYEARECAAYFAkpXepAACgkQJdeBCYSNAAMgLQCgtc74iSqMUmhWIreqX4btkFg8
HPYAoMoMdJsSeVG/QhrK3HWeFda1zenR
=bvCv
-----END PGP SIGNATURE-----

review: Approve
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.

Marius Kruger (amanica) wrote :

> Since this is breaking the compatibility, as long as it is a step
> towards where we want to be for log, I'm happy to see it land. Though I
> would probably wait for 1.18.
>
> (I don't really want to see us break the meaning of 'bzr log -r X..Y'
> two times in two releases back-to-back.)

I'm busy implementing what you suggested in the previous merge-request-review [1].
So if you want minimum disruption, you may defer landing this until I'm done with that.
They have some overlap and they cancel each other out a bit.

The following branch more or less implements your suggestions,
but I'd like some feedback before I go all the way to fix all the tests and update all the documentation etc: lp:~amanica/bzr/320119-log_exclusive_lower_bound/

I have 2 questions between your suggestions (marked with ending '?'):

1) "bzr log -r 10" should show just the revision 10 (https://bugs.edge.launchpad.net/bzr/+bug/325618)
* I assume we should still allow "bzr log -n0 -r 10" to show the parents!?
(when a range is given, we never show the parents any more as per Bug #325618)

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.
* This just becomes an alias for "bzr log -n0 -r 10". Do we need to retain it in that case? I assume so.

3) "bzr log -r 9..11" should *not* show the revision for 9 (https://bugs.launchpad.net/bzr/+bug/320119)

These changes break a lot of tests, I intend to only change the minimum
to get them to pass.

[1] https://code.launchpad.net/~amanica/bzr/log_returns_too_much/+merge/8349

Ian Clatworthy (ian-clatworthy) wrote :

Sorry for taking months and months to get to this review. It's been a crazy time for me. I'll try to find some time now to work with you towards a solution we're both happy with.

To be honest, I use qlog almost exclusively these days so I'm less passionate than what I was about how log behaves on the command line. I do agree that #325618 is a real bug, though I'm not sure I'm comfortable changing default behaviour to fix it. I certainly think "bzr log -n0 -rX" ought to show the merged revisions for X, so if this patch alters that, I'd vote against it.

On a semi-related topic, if we are going to allow/encourage alternative behaviour wrt revision range semantics in log, I like the idea of adding an option called something like --exclude-lower as Eric suggested.

In your opinion, is it possible to fix this bug without changing semantics?

Vincent Ladeuil (vila) wrote :

I'm pretty sure this has been merged from a different proposal, so I'll mark this one as rejected in lieu of superseded.

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-08-30 23:51:10 +0000
3+++ NEWS 2009-08-31 04:37:08 +0000
4@@ -388,6 +388,9 @@
5 commit that found a missing file will leave the tree unedited.
6 (Robert Collins, #282402)
7
8+* ``bzr log -r123`` should not return 123's parents too.
9+ (#325618, Marius Kruger)
10+
11 * ``bzr mv`` no longer takes out branch locks, which allows it to work
12 when the branch is readonly. (Robert Collins, #216541)
13
14
15=== modified file 'bzrlib/log.py'
16--- bzrlib/log.py 2009-06-10 03:56:49 +0000
17+++ bzrlib/log.py 2009-08-31 04:37:08 +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-08-31 04:37:08 +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')