Merge lp:~amanica/bzr/log_returns_too_much into lp:~bzr/bzr/trunk-old
- log_returns_too_much
- Merge into trunk-old
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 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Vincent Ladeuil | Approve | ||
Ian Clatworthy | Pending | ||
Review via email: mp+8349@code.launchpad.net |
This proposal has been superseded by a proposal from 2009-07-10.
Commit message
Description of the change
Marius Kruger (amanica) wrote : | # |
Vincent Ladeuil (vila) 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> _DefaultLogGene
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(
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/
amanica> --- bzrlib/
amanica> +++ bzrlib/
amanica> @@ -277,7 +277,7 @@
amanica> # tags don't propagate if we don't merge
amanica> self.run_bzr('merge ../branch1', working_
amanica> branch2_
amanica> - log = self.run_bzr("log -n0 -r-1", working_
amanica> + log = self.run_bzr("log -n0 -r-2..-1", working_
amanica> self.assertCont
amanica> log = self.run_bzr("log -n0 -r3.1.1", working_
amanica> self.assertCont
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) 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
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> _DefaultLogGene
>
> 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://
iEYEARECAAYFAkp
kJcAnj5aW/
=8wKN
-----END PGP SIGNATURE-----
Marius Kruger (amanica) wrote : | # |
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> _DefaultLogGene
>
> 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-
def _log_revision_
# Get the base revisions, filtering by the revision range.
# Note that we always generate the merge revisions because
# filter_
calls vvv
def _generate_
delayed_
# 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_
rebase_
"""Calculate revisions to view including merges, newest to oldest.
calls vvv
def iter_merge_
"""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(
> 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...
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:/
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://
Updating diff...
An updated diff will be available in a few minutes. Reload to see the changes.
Preview Diff
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 | 68 | does not already have a control directory. The flag ``--use-existing-dir`` | 68 | does not already have a control directory. The flag ``--use-existing-dir`` |
6 | 69 | will allow operation to proceed. (Alexander Belchenko, #307554) | 69 | will allow operation to proceed. (Alexander Belchenko, #307554) |
7 | 70 | 70 | ||
8 | 71 | * ``bzr log -r123`` should not return 123's parents too. | ||
9 | 72 | (#325618, Marius Kruger) | ||
10 | 73 | |||
11 | 71 | * ``bzr ls DIR --from-root`` now shows only things in DIR, not everything. | 74 | * ``bzr ls DIR --from-root`` now shows only things in DIR, not everything. |
12 | 72 | (Ian Clatworthy) | 75 | (Ian Clatworthy) |
13 | 73 | 76 | ||
14 | 74 | 77 | ||
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 | 400 | log_count += 1 | 400 | log_count += 1 |
20 | 401 | if log_count >= limit: | 401 | if log_count >= limit: |
21 | 402 | return | 402 | return |
22 | 403 | # As per Bug #325618 we don't want to show parent revisions | ||
23 | 404 | # outside of the specified range. | ||
24 | 405 | # So we need to filter the revisions here again since | ||
25 | 406 | # the lower levels also return the revision parents, | ||
26 | 407 | # which is needed for filtering on file_ids. | ||
27 | 408 | if rqst.get('direction') == 'forward': | ||
28 | 409 | if self.end_rev_id == rev_id: | ||
29 | 410 | return | ||
30 | 411 | else: | ||
31 | 412 | if self.start_rev_id == rev_id: | ||
32 | 413 | return | ||
33 | 403 | 414 | ||
34 | 404 | def _format_diff(self, rev, rev_id): | 415 | def _format_diff(self, rev, rev_id): |
35 | 405 | diff_type = self.rqst.get('diff_type') | 416 | diff_type = self.rqst.get('diff_type') |
36 | 406 | 417 | ||
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 | 277 | # tags don't propagate if we don't merge | 277 | # tags don't propagate if we don't merge |
42 | 278 | self.run_bzr('merge ../branch1', working_dir='branch2') | 278 | self.run_bzr('merge ../branch1', working_dir='branch2') |
43 | 279 | branch2_tree.commit(message='merge branch 1') | 279 | branch2_tree.commit(message='merge branch 1') |
45 | 280 | log = self.run_bzr("log -n0 -r-1", working_dir='branch2')[0] | 280 | log = self.run_bzr("log -n0 -r-2..-1", working_dir='branch2')[0] |
46 | 281 | self.assertContainsRe(log, r' tags: tag1') | 281 | self.assertContainsRe(log, r' tags: tag1') |
47 | 282 | log = self.run_bzr("log -n0 -r3.1.1", working_dir='branch2')[0] | 282 | log = self.run_bzr("log -n0 -r3.1.1", working_dir='branch2')[0] |
48 | 283 | self.assertContainsRe(log, r'tags: tag1') | 283 | self.assertContainsRe(log, r'tags: tag1') |
49 | @@ -500,13 +500,6 @@ | |||
50 | 500 | timestamp: Just now | 500 | timestamp: Just now |
51 | 501 | message: | 501 | message: |
52 | 502 | merge branch level2 | 502 | merge branch level2 |
53 | 503 | ------------------------------------------------------------ | ||
54 | 504 | revno: 1.2.1 | ||
55 | 505 | committer: Lorem Ipsum <test@example.com> | ||
56 | 506 | branch nick: level2 | ||
57 | 507 | timestamp: Just now | ||
58 | 508 | message: | ||
59 | 509 | in branch level2 | ||
60 | 510 | """ | 503 | """ |
61 | 511 | self.check_log(expected, ['-n0', '-r1.1.2']) | 504 | self.check_log(expected, ['-n0', '-r1.1.2']) |
62 | 512 | 505 | ||
63 | @@ -536,6 +529,18 @@ | |||
64 | 536 | """ | 529 | """ |
65 | 537 | self.check_log(expected, ['-n0', '-r1.1.1..1.1.2']) | 530 | self.check_log(expected, ['-n0', '-r1.1.1..1.1.2']) |
66 | 538 | 531 | ||
67 | 532 | def test_merges_partial_range_ignore_before_lower_bound(self): | ||
68 | 533 | """Dont show revisions before the lower bound""" | ||
69 | 534 | expected = """\ | ||
70 | 535 | 2 Lorem Ipsum\t2005-11-22 [merge] | ||
71 | 536 | merge branch level1 | ||
72 | 537 | |||
73 | 538 | 1.1.2 Lorem Ipsum\t2005-11-22 [merge] | ||
74 | 539 | merge branch level2 | ||
75 | 540 | |||
76 | 541 | """ | ||
77 | 542 | self.check_log(expected, ['--short', '-n0', '-r1.1.2..2']) | ||
78 | 543 | |||
79 | 539 | 544 | ||
80 | 540 | class TestLogDiff(TestLog): | 545 | class TestLogDiff(TestLog): |
81 | 541 | 546 | ||
82 | @@ -876,7 +881,7 @@ | |||
83 | 876 | self.assertNotContainsRe(log, 'revno: 1\n') | 881 | self.assertNotContainsRe(log, 'revno: 1\n') |
84 | 877 | self.assertNotContainsRe(log, 'revno: 2\n') | 882 | self.assertNotContainsRe(log, 'revno: 2\n') |
85 | 878 | self.assertNotContainsRe(log, 'revno: 3\n') | 883 | self.assertNotContainsRe(log, 'revno: 3\n') |
87 | 879 | self.assertContainsRe(log, 'revno: 3.1.1\n') | 884 | self.assertNotContainsRe(log, 'revno: 3.1.1\n') |
88 | 880 | self.assertContainsRe(log, 'revno: 4 ') | 885 | self.assertContainsRe(log, 'revno: 4 ') |
89 | 881 | log = self.run_bzr('log -n0 -r3.. file2')[0] | 886 | log = self.run_bzr('log -n0 -r3.. file2')[0] |
90 | 882 | self.assertNotContainsRe(log, 'revno: 1\n') | 887 | self.assertNotContainsRe(log, 'revno: 1\n') |
Fixed this by adding a stop condition to rator.iter_ log_revisions( )
_DefaultLogGene
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