Merge lp:~amanica/bzr/325618_log_returns_too_much into lp:bzr

Proposed by Marius Kruger on 2009-11-01
Status: Merged
Approved by: Ian Clatworthy on 2009-12-04
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~amanica/bzr/325618_log_returns_too_much
Merge into: lp:bzr
Diff against target: 103 lines (+54/-1)
4 files modified
NEWS (+3/-0)
bzrlib/branch.py (+14/-1)
bzrlib/tests/blackbox/test_log.py (+15/-0)
bzrlib/tests/per_branch/test_iter_merge_sorted_revisions.py (+22/-0)
To merge this branch: bzr merge lp:~amanica/bzr/325618_log_returns_too_much
Reviewer Review Type Date Requested Status
Ian Clatworthy 2009-11-01 Approve on 2009-12-04
bzr-core 2009-11-01 Pending
Review via email: mp+14275@code.launchpad.net
To post a comment you must log in.
Marius Kruger (amanica) wrote :

Ian Clatworthy wrote on 2009-10-20
> 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.

Thanks a lot, without feedback I couldn't really continue. I've also been a bit occupied (holiday/work/studies).

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

ehm, I mis-analysed the problem and fixed it the wrong way the first time around.
In stead of chopping off all parents after the stop-revision, I now allow
the "merged revisions" of the stop-revision. I think that clears up all the
controversy and all the tests I mangled before could be restored to their
previous glory.
BTW. your phrasing here in the feedback helped the penny to drop. thanks again.

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

Great, I added a draft spec now, to keep track of all this:
http://bazaar-vcs.org/DraftSpecs/NewLogBehaviour

> In your opinion, is it possible to fix this bug without changing semantics?
Yes. this merge-proposal should be self-sufficient now. i.e. I don't think
it needs any of the other proposed changes to be useful and bearable.

For the last time thanks for the feedback, it inspired me to start completing this stuff, finally.

Ian Clatworthy (ian-clatworthy) wrote :

Hi Marius,

This is looking a much better approach. The big issue right now is that the new test passes *without* the code change which means that the new test isn't triggering the bug you're fixing, right?

Also, I'm not 100% sold on the proposed change yet though for a few reasons:

* I'd prefer to keep the term left_parent (as opposed to mainline_stop_rev) because a user can log a "development line" which isn't the mainline, e.g. log -ra.b.x..a.b.y

* The inner if statement can go at the same level as the outer one???

* Do we need to test reached_top_revision_id in the inner if clause???

Does all that make sense?

FWIW, I'm about to look at a similar bug in log so I may end up using your code to do that. Perhaps we can pool our efforts and solve this batch of bugs together in coming days?

review: Needs Fixing
Andrew Bennetts (spiv) wrote :

Btw, I see that the reporter of bug 484109 says this branch works better for them (than trunk), so that's a good sign that this patch is on the right track.

Marius Kruger (amanica) wrote :

2009/11/20 Ian Clatworthy <email address hidden>:
> Review: Needs Fixing
> This is looking a much better approach. The big issue right now is that the new test passes *without* the code change which means that the new test isn't triggering the bug you're fixing, right?

whoops. I think this tested my first approach which would have stopped
at rev 1.1.2 .
Anyway with the new approach I had to change it to something which used to work.
Since I wrote it already I think it can stay, so I'm leaving it in.
You can delete it when merging if you want.

>
> Also, I'm not 100% sold on the proposed change yet though for a few reasons:
>
> * I'd prefer to keep the term left_parent (as opposed to mainline_stop_rev) because a user can log a "development line" which isn't the mainline, e.g. log -ra.b.x..a.b.y

mainline made more sense to me in order to understand the concept, but
I see your point.
I changed it to left_parent again (the fact that it is actually the
revision we want to stop logging at
was important for me to understand the code, so I added a comment to
that effect).

> * The inner if statement can go at the same level as the outer one???

not sure which inner you refer to here, but assume "if rev.parent_ids" ,
yes it can move up but I'm avoiding unnecessary calls to
repository.get_revision() which looks expensive to me.

> * Do we need to test reached_top_revision_id in the inner if clause???

yes, I only want to extend the whitelist with the parents of stop_rev.

2009/11/26 Ian Clatworthy <email address hidden>:
> Here's what I'd like you to do: add tests for the changes you've made
> down at the branch.iter_merge_sorted_revisions() level. That's actually
> the important tests in this case, rather than higher level log ones. If
> branch.iter_merge_sorted_revisions() works, then log will work. :-)

your right, since we're using TDD.
I was just copying existing tests and thinking about the behaviour I'd
like to see,
which is by the way the BDD[1] way which I came to love.

> The current tests can be found here:

> bzrlib/tests/per_branch/iter_merge_sorted_revisions.py

added two tests here now, of which one fails without my fix.

> I hope the existing tests are well written and easy to copy-and-paste
> from.

it took some digging but it is nice and clean.

> Let me know if you run into problems. I'm around this week but
> back on chemo next week (so around but brain dead then).

Hope it goes well.
Thanks for your efforts and patience.

launchpad doesn't want me to push now, so I'll leave a `sleep 3600 &&
bzr push` on and go to bed. Also attaching patch for good measure.

[1] http://en.wikipedia.org/wiki/Behavior_driven_development

--
sorry it took so long.
<>< Marius ><>

1# Bazaar merge directive format 2 (Bazaar 0.90)
2# revision_id: marius.kruger@enerweb.co.za-20091203235357-\
3# 1po7aapt4nljf9h9
4# target_branch: file:///stf/prj/bzr/bzr.repo/bzr.dev/
5# testament_sha1: 82dca80bec3cd38186a5f8a0ca7ad460cc301ed6
6# timestamp: 2009-12-04 02:29:00 +0200
7# base_revision_id: pqm@pqm.ubuntu.com-20091120185110-7muay17qmdqvrg7z
8#
9# Begin patch
10=== modified file 'NEWS'
11--- NEWS 2009-11-20 16:42:28 +0000
12+++ NEWS 2009-12-03 23:53:57 +0000
13@@ -35,6 +35,9 @@
14
15 * ``bzr ignore /`` no longer causes an IndexError. (Gorder Tyler, #456036)
16
17+* ``bzr log -n0 -rN`` should not return revisions beyond its merged revisions.
18+ (#325618, #484109, Marius Kruger)
19+
20 * ``bzr mv --quiet`` really is quiet now. (Gordon Tyler, #271790)
21
22 * Lots of bugfixes for the test suite on Windows. We should once again
23
24=== modified file 'bzrlib/branch.py'
25--- bzrlib/branch.py 2009-10-15 02:11:18 +0000
26+++ bzrlib/branch.py 2009-11-21 16:12:55 +0000
27@@ -503,12 +503,25 @@
28 left_parent = stop_rev.parent_ids[0]
29 else:
30 left_parent = _mod_revision.NULL_REVISION
31+ # left_parent is the actual revision we want to stop logging at,
32+ # since we want to show the merged revisions after the stop_rev too
33+ reached_stop_revision_id = False
34+ revision_id_whitelist = []
35 for node in rev_iter:
36 rev_id = node.key[-1]
37 if rev_id == left_parent:
38+ # reached the left parent after the stop_revision
39 return
40- yield (rev_id, node.merge_depth, node.revno,
41+ if (not reached_stop_revision_id or
42+ rev_id in revision_id_whitelist):
43+ yield (rev_id, node.merge_depth, node.revno,
44 node.end_of_merge)
45+ if reached_stop_revision_id or rev_id == stop_revision_id:
46+ # only do the merged revs of rev_id from now on
47+ rev = self.repository.get_revision(rev_id)
48+ if rev.parent_ids:
49+ reached_stop_revision_id = True
50+ revision_id_whitelist.extend(rev.parent_ids)
51 else:
52 raise ValueError('invalid stop_rule %r' % stop_rule)
53
54
55=== modified file 'bzrlib/tests/blackbox/test_log.py'
56--- bzrlib/tests/blackbox/test_log.py 2009-06-10 03:56:49 +0000
57+++ bzrlib/tests/blackbox/test_log.py 2009-11-01 03:45:20 +0000
58@@ -536,6 +536,21 @@
59 """
60 self.check_log(expected, ['-n0', '-r1.1.1..1.1.2'])
61
62+ def test_merges_partial_range_ignore_before_lower_bound(self):
63+ """Dont show revisions before the lower bound's merged revs"""
64+ expected = """\
65+ 2 Lorem Ipsum\t2005-11-22 [merge]
66+ merge branch level1
67+
68+ 1.1.2 Lorem Ipsum\t2005-11-22 [merge]
69+ merge branch level2
70+
71+ 1.2.1 Lorem Ipsum\t2005-11-22
72+ in branch level2
73+
74+"""
75+ self.check_log(expected, ['--short', '-n0', '-r1.1.2..2'])
76+
77
78 class TestLogDiff(TestLog):
79
80
81=== modified file 'bzrlib/tests/per_branch/test_iter_merge_sorted_revisions.py'
82--- bzrlib/tests/per_branch/test_iter_merge_sorted_revisions.py 2009-07-10 05:49:34 +0000
83+++ bzrlib/tests/per_branch/test_iter_merge_sorted_revisions.py 2009-12-03 23:34:38 +0000
84@@ -84,6 +84,28 @@
85 ], list(the_branch.iter_merge_sorted_revisions(
86 stop_revision_id='rev-3', stop_rule='with-merges')))
87
88+ def test_merge_sorted_range_stop_with_merges_can_show_non_parents(self):
89+ tree = self.create_tree_with_merge()
90+ the_branch = tree.bzrdir.open_branch()
91+ # rev-1.1.1 gets logged before the end revision is reached.
92+ # so it is returned even though rev-1.1.1 is not a parent of rev-2.
93+ self.assertEqual([
94+ ('rev-3', 0, (3,), False),
95+ ('rev-1.1.1', 1, (1,1,1), True),
96+ ('rev-2', 0, (2,), False),
97+ ], list(the_branch.iter_merge_sorted_revisions(
98+ stop_revision_id='rev-2', stop_rule='with-merges')))
99+
100+ def test_merge_sorted_range_stop_with_merges_ignore_non_parents(self):
101+ tree = self.create_tree_with_merge()
102+ the_branch = tree.bzrdir.open_branch()
103+ # rev-2 is not a parent of rev-1.1.1 so it must not be returned
104+ self.assertEqual([
105+ ('rev-3', 0, (3,), False),
106+ ('rev-1.1.1', 1, (1,1,1), True),
107+ ], list(the_branch.iter_merge_sorted_revisions(
108+ stop_revision_id='rev-1.1.1', stop_rule='with-merges')))
109+
110 def test_merge_sorted_single_stop_exclude(self):
111 # from X..X exclusive is an empty result
112 tree = self.create_tree_with_merge()
113
114# Begin bundle
115IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWep9X6YAIFF/gFVUREBY///3
116fz+fjv////BgJZ68o8+z3fdn1221cvRfVXUVvdt131k7e7pwDHZ0fXffcGgW+dEo7YDmx9bs9Buz
117p7718NjCePW72XFvcOe7j7eduvd7Vz6ca+fHPvVkSipktZgha2bWrR7vd50okhKICMgIwUwCaU2o
1189TIepso9QAAaNA0ASSAAIIgiZEynqemo9IemoAGgAAABKEwiE0aU9BTTE0fpQPKGgAGgAAAASEhC
119aEmZGjRGhTHqTamm9SZABgjQAGmgRSIBNTGgmE1PST8mqMB6Ro1T0nhTTZQNAZPU/VBFIQAmU0xG
120h6k2o0eponqHqamgyAAaaGgC7WjGpGAEwnCa5UCSevrigoI6pbt9XEQ8X7xiT/To9jp/zH0r61/D
121pQTmvDYlSK0Ynbdhe5CAoV461+T1SpXZZz7Kc6rE/k3f+pe2BwttK0qOzP7H8/uPujZfqbwdRaeq
1222x1K1op1PT6J14h551njZh0Z1n9vZe/J3SZs3rDYctzUZ+rGuHkuXIPyFM6qc86/ZAuo66aFIU2O
123i6Z5nos+bM2smaf8477ZnQqxwttXwchFAjagYNrw3zKnbvnerGOui1MtMsspibJfseeeL1VoSdWX
124RdJGGD1iUa1ZnitlkowGxEaFr4S8iNMMzPpUIIA+HVUGVGpJva2UKYdqXjaS9zMVWmmwFlVkcHXN
125b5ZokRh5gQxiB2wxZR7wl7WF8NqBZRLGoLYc0EQrUU/Jw/ucWmXBo2ePTY9eQbBp+oSX+ACGbGNA
1262ohNgMYxpNpjaGJjTaQ2NjaGxWuv/aCZ+nCwiXmsC1YRphiISEtN1Wki012UD5toiydUda1r1xQw
127PCgqZXtOtLbCu1rM9pJa4lOoUFAHCg8HtJrRGcKKUjBWUVYYfGublo27SYmrtazxaVaOsFIu9los
128PWxNZDN0KmFTDDF1MZMjLdRyzCUzROJEcapLXxcbFPIYxjGToyGUstOlpydEskAzS0t4UA7OlPHr
129yXVxG5jme2caETwnv6knWHVYghYzXmRCVLEUaKe+HBL/CuS2ZhU18DY1n+eQ1SmxilW0RfvBJ7Ef
1307a6k2SeIZCfErr196Wx/1dAVi3nEj7ImH6eNxfzXh5MphQ/Kpcu2AaUGIoe7K7MsqClSb2KorrOw
131REcHXlDYm/qW3FyrIbdFvB0xYnPhHLFDcbn9FbAhVkzBqtV/dCujw4OjcZ9TcwbbwHzfih+PCSRx
132mN42A0JSRVRTp7AxvkLSWm2UxVkx8ByTFBdDvVFT/2buAESfmDjI4o+78HRejmZdR4vN4/6Zj17I
133JeV50QsVTTnVffXB1D2g12BrTu0z59rPBxZ4LHuSPc/dpb2eGnTsL7vnEeKV1oldf5S8kLqs3ujb
134ebjjbkZTlOdMaA6vGWxu9fl6MePRwzLvg52zXjxtLTjW4bVZDtj58E6xDpT3+R3wSREQEyZnazPX
135c3TViuebdLCxViFB7iBE+Bh8PlP5OkoiYU1hByDYeI82EvaMEZ5UG9txNyjFVRg6KNmPJnoIhVag
136uRgQehkdzHCwrJtZEceU/jUVXH1FoLHcyApNDzpiT7UlAl2B46vfoqQorpNZUbEahBHp2ML8VwFy
137icjYbag2Hx9xXYd+7yD63OD6xmFH4Ido+sWAfcwO3ifOawiJEIhbCeiCwkAhudcSTqJ1i4COsczQ
138dMtc0bfXQ1A6UaR5ozEt3oGCBWjijxDEOySBghkhUGIYWFkNZGv14Y9aBhcDxMhIAwQzo70YTaX5
139dvF6tlXXUX38JmAMiDfDXDsnXOsoU29VxZxOPMPR2kuQoMj7w5CBDBoTQwBg2pALHEOFqnOmdl5Q
140UHhV7jiXFEm1QlcQBAhdstkFnIE5iQLyNDxEls6Ok3a8Of5O7Dzzrtz6eMJIpISQkhJCSEkOXdry
141Oz6zfvNuDXXVSQkkkkkkJJJJJXXumSF9ojdRY/8V4es4nqbPH3zVXqAdDxUSrGSuJAwQpbCrOGUN
142IIhS+u+zHHRAVXjJjNbgWFs1W5BaXdZ5t7kKgzSiE0fvgpLZFKqd8pLMsSQCLK/jLOLTpOF9iBDk
143ayBDhO7QHY+EcdVjFPjsiOVZCM5+xxQukEojESMRvRmjcjFAZkxGu5DCVRK6VngjaDXQ3QWUSVzT
144ULS5GkpFkrAqt8zSS2VHDUcdF0+bq2cEJCLJJ1R6fd/AsjCjo02bMXN+ulyNZqsKIEUAuARZMukn
145mGS4OiaqRYmlRHGx5OuSQlUny4E+xxSeFRlBNAmJCOKCGgkABIRhKgDu55UgdCiRJmmpLmajZygn
146HS9auSMGy0WzIK72IvbQpBAzVkKC3iUMb6F8FwAFqAqTQDAhhBT8gbPY7nY1xHWBK5m9MkYDHGni
147dAFMp55Kb2KzqGhGChvp77bdwYwDnRDOTlblFiBS5ArJbqWTgiLyemFRZyR8keYB+oAgTcU2bMIN
148wfCOER4mktjsI46oJPLlWWIKNGo0Iw03E1GqaMglt2otvFW3PcXNSIwjZfpMDH6SC4TCCIqC/r7e
149FPy4ndmbHw3F/WgUvFelGw6SVCCb6zVjgU0qtJuJCZtRkRVEk4iB6tDtQK4MEgOp2I5pNo7EEYOk
150htLLFQYbxNCLGznZx5lAGSWRK5g6lxIQ0Sk6A2JsQM0Cfc7qjYbdFxMIHQGXRYFisDmQiYFTPMA4
151s94EVmSGoIMIXoq3EeIHpzw/t/0wpCmSreldVfWL76haNpxfC/SAzG5YfxXp7WrN0hgBmBw/Gyez
152Z6JBDJPBxEkAB44Hq7pmVAEw3zQHqgHy+sAvsRCjAujvi8DfaLuaaFyRBFWQ3gdM8YOuvShxerGX
153AuClwenOQWGE4yP+1ZPkwTkGh+R97PBggwROROICHr39teO3l7Ww9CcW8ZQshDhQ72axhYSAb2BH
154JAgxr39DeDk5odcMbMmDcfR1eaqdeWGoUJyMjekzEiO3FCf0W2DKGOnDvlVHOje63FzXX30HmhcP
155UIOP4ELig2oXqMbN9Tqvut3hTEccm9bNdwO15HZ5p2EqG7UnMWRPk74dG6dvN/iLmPe9Ag3CN9y6
156ml9+NCW8NrYooGCBkpCSNgjCMlatzc+PgczfMX6kA8+gpInqtdqxhHLBdPBVQMg0RFhCgeGZVMTP
157Y0WpdC9enHBtmeS42IVEaBajcURta7O/DGvTS4M2gMXiwASMZq8cxXOW+gRBVGU5WlwagjbRGJMy
158uNlCekOAGFVgzeKFm7y+KhNChOpztpRIUq8HjroUypVBOxKWGplQqZgGvGjDADVtZvT5UO4HHKSB
159SgAXJi+owJLe8jO9e5AbAngTkX878YxHyzvJJOwdDqccGc51gphV8vQsGtdfoBNZ2yxUyZMlmXHe
160X4weIPnQ4I59U0d0keKNenVLctmu+Xl2jrRdro1CqxduWNCU8ZIAZGV2gE765PlRWLa4EqYBygF0
161lxO3FenZ6KUQwDIaS5QnYgZMKKC5mKEmc8xV4j8P5UYDiZYA8gc127eM+9H7EmQnBhjzB/0uog5l
1623jNIVKY+UCnM80i7mlQTIH3HoOCngkzjA1GANipQFUVjIK9R4+hAc9JY8vGM0+VyunYq3xCfHCch
163YvvFYO6yZnkhfVNrOXNaCCE5czkJa65N/J1e7XGdSd4hTlbhDcFjokH222LbELBtM7C5PmQauUHY
164KPmTHkDWurGzYjs0Yr3QfjNhzx/4pLYfMPmFxFUK7k40+vOGZnXl5JDUhuj4p3kYgNR8U6LPJ1H8
1651O+WkBcj6PBmYZWQLJcADs8gqFKU2ix3rFpzvsRQke3yWNMTO2XbTV98zv69KvJOjgWPhAzeUkGZ
166IJvkoz4HTGr7uxM8kanDiinBA0PxHSPU6O9CPTjFIQ6ddKKMDjtTI37wSKMsXxxbrLWxS2JJU4I0
167kSTcG77eLxGarcbdvwo3MTaG0NobQ2htD28r/AAZdiFp9MaYdGvUX5bjBAw98kC0rVShhNpDxQz4
168g9eeRtO3Fa8XXhS4vPE6MIa9sYkpaSzF9Wd49xjRu++HEiRGRk31duB42HYUdAH1oVOQD3FYjqj7
169QCr15dWk6N21pOAqk1yo0dOmtkAWCSsQqgJwTbKbrQ29l5LAL0eo3dtAxRchZ4USgDwMbyYNE7IN
170RmHckm30eFuLo3dpCYV14Oiu7TB+KXu0RyDhyr3Jqm11uRp+/jaCFE36CxH9hCYwQtV5vi8yYiZj
171GR2u+16qEbWZBwyvKFYsyfTG6Mi0SJdwljpDXkJiNoyNB3ktSgbJClJgvRCqTJsQqtivApuAK3cC
172b8FsceTKFAUQUYGqkyfYvZgZkzAzAzAzJmTHk63qsfUw7RnBMmwHy5rtiCMCKOJrilLKTO2RRs8C
173Ju4GZju5aCEm26rDg/G5vUYoDfnmJ3o5iWe0S/xR2lRoZ9VdIjCRVS4SES09Ho/zN40yNEFWaSSZ
1747cde00pxEZQQhBgw07ExfeSZER1EDOHRMSFmL/X3fZi2hiQw+QyF8bojWJf0M0IDR17bMIWS8THt
175V/R7GbFHac4UwVmsd9c0Lz6eWhnWXXXkbkYnaOIju86nShJ2blAt2KNea/Dbyxj0HGsESjvRAdbZ
176JIhqQ/krbgQ8niZz5IdFYbG8kprAPlqyURxGfFpl2FEk4AaWpWuTPuQqhIPx+tC+1Crr2vk06Xv4
177smSMgqJdWbxMM0ceKtJjQW5CyQqMoTSgCNJHDIFky0zGHcWwjNoWNMUkhpAm/aV9JPOoWxGq2uld
178CQRskjF4d6BMRe1SCzrTqw3kw2aXR+AU65iVISEHGl7CUN0M2pqRQegi8sdYj3QCFAeXfEUUfAuN
179JrMfGm6WFU66qSEt0NylyaEqw43zfB9QtTIHHBL77G9AJAAV74WjGi3zWHMSE8vQgQtZMnAaDlPJ
180SmnfoQdGalqDzHqUPQjyRYeYOSj82hJjDzDqqJi2+QmaPYj4oWo8X2hsesGZnxQy8HjbsnMqc8/D
181s5QuHWy2GaKmpy6DXS+Kydxd9kGkAeDI9j4FEKpXw/3ENQbrmwmIkwxu30NVn8ApT+Ki6Uvky+qt
1829SOwx3BCeUxfl3hai43Pio4Ua0QKXLBNDxSA6wXtoMOQJYLOkr3lLFyB3Whi7TFPNSLazMS8WMbI
183MYoDihE3aQ91JF6F66I5IUH/ehTF0hjCGENLE305HrMSzMOQCigOqh7oC23uUOHQKDzmA84GeSOz
18480ScDZZoPJniOpklrjgrDxu+PXZMUQpKKvXVMxkzpnlr26FkvjL38JzvfaFgA+0FhC5xx+c36nLJ
185aWIBVsSmD6FnaFnaJWlGeuJV9EjNITCAZgGyuldrSYFlclYgAW8hG5rEeh8PXq3vK9QAPUYQrDIE
1862nTpPb0JzjGoak/nydaiQTNR9LRT75f9ZGRQwPuYp9t3+qz8BwiBmgjw7a6akhxGI104WHbjepxb
187jNtCNFmNh7UFaWL5qypyfJ95mNNP09I63P6EB1ABDkB3BA3QjZE6xVTzJ53sYlCR3rmcJeB1WYgt
188DyknabQOWLS4eaWyjQWOhaUhNWUREy4oHXBC7xYqcH+mimmHh9z5ezlhSZjPrQJKsgKEJEZuqmSA
1899LSEIE5DxQIUOxAqQInRQnRAtoAkimh2oElUmgSAAhEK0CK1UitADDMkBEA3crHrA80khpH52JiY
190DZxEY5ICKA022Jg0mNLiAvcNpsbGxsbGx0NUmhREFBUmxtttsbbbcBhwGQwB7UvkEl3wR73jQDA0
191OS2Cg48fBQWwEoTMMRTREhgGCFiIhIQSIUj+87DoKf2RfoiVKNJUlfvPsTEfsfsSTzoEB9joXpWF
1927jC/6mVRrUfqo+CLqPBEqL6lG8S9Am6DQqfIn5y4kFXWbX319J3k+P6zuNyQrnIY/WQZun6DnY80
193mwY202WQHelQDUTEMUQMGiBGDMfNVIAPOIIUQNAhDgfKgP24sDT0ND8x8Qb0B5JYKcGNkHEEC+tF
194gRkwRmCwhnxa0CRNgqP6B7nfkCYQa/yRU7fXPn3kid/tQn31d5oBEqO4Swt7zFyJMiUU0N/vL7y4
195TMWn4zEQvVnj7n8Ru5ZaMZLegonM81kHIh44xoscPtnMBzO/gQsuJMOrv51eXkaIeEF9MSvm5Ern
196VjogVYydF5MLM8lShMxGLvzbsfzfERZcnrBG5ui5AkaLydYIbmo9rRS25dYw14OW2wHndRNb9IXM
197lp3h3qT1F8UvXx4q6bce6R56EbA9BB9iACq/OoLwCB+4+DnLnS/9C87T1MG0YJBDEmRMgmRMgmRM
198gmRMgmRMgmSYJD+AHNVwEP139CiXJSxmnjmoLQrexlsX/hM4iq+fwRLBQa0KC+rchrPSFdLskgcj
199YSQJXcikHg/mj4n8+R2EQwR40LxQZTIPYfE5z1lzEgHtkWx5a0PK7cc8Sox41TMLPKhBRdUx8cls
200FgTstrV1F5jIlxQuMS0GSdKiViqiZLj/u5gWKVGHGi3LiTy95uFj5fYVlJ3BwMapLBzM3us+TtOI
201kt46y+Hyr14IaT4MzHBxKdj9hKfqaS4G6VQnihm7e9Dw9yXmJ8WhWJNon0kaUCci18zD7iM3ZVSV
2028ASdzzC4mGkuQ+xERBOmJ1C8iRt4HHfxHhzxZTzac5calOfMaNfTq0dI5zsrn9w+4FvnsqPKwpDE
2030KV2tci6T6nB2MWKvmYNM5qTODDtMPqRE8uXwckhsIKdEI3iav4SBKPhBR0UiC9vwUL2KHp4qdSO
204+6j403za269y7H/Wn9gvP6zfb2bHmxucka/PNTXte7ZOjycm6oqqh6azYZlOl64VDSJa6+rcJWAf
205usELqWiEpMb3EquRFb8kD2S7aFuPxCSOOMr45xQR8mS0hLWlUF4XsxMuQDux2d/Htzww0zZJJNSx
206E0ubgOqMOvvQCy2EEaou/aTHT3Fu5tzHhvyqxguwjHWLWzzCRaz1MY34Ae6uFBB3SUDJdibeABNy
207c6HGDRH6ax8gAT1m1fPYjycTe+3mp55QHEzs8CEFQj+6OQPXURhDY9DOrOd7pxOF2RRE16hdiCiW
208TlGOKhFi9jEcvIg+3m8jkRLaTUyc+ojkvBMrul0F4BQWRwyHYlQ3BBNnHqREQy79kGZ7/QSmCN+0
209HcmBRD4ODAViPZaj0mRGe47ccCntxLT4GX0edZ7gSUhJSZTv1mHKBLmFJmrBHaYcfqNHA0CHBU3V
2109yRASX1CXz7/MPA3AnHiTBK7zblhrjfY34NZ6JbqCWJWKaxieY8yVw1ucECz3uwlSY9IjwhXSO+E
211foM9CuxdwVYYvpqeuGGMcp0E6WvqpxiZUxli3x200WV1VudhCGjIydrf7wl/U+0i5G4t2cXou/nu
212OaFRbAjQevYCq2k6uliK8Yo55gtVCzNxcqYIDn/Bq8FjNida3fCRDGb4O1nX4Js79RNQSATvLxM1
213hISAN8EQDBsbG2mmmNsCCyPA9YTCSTSAKVzIDkEFpYbR0My0SrpMJ9MB4Cg659W7hDHdzrQl8FHb
214qVDVI4fN0ca6dvqsi+xN9aknBUBSKx0sTggkECsS3AYZzzEMSG1/f7jZfLtc7nHOOg0sGJarDYgG
215juHWzlxqH9Lm7OqcuDEA3gGpwJaxOhxYJY57BPRaeTOl5yXPvMmUA1IFBAO9IINuOddS0zbfCWdO
216db+tqALa6rKC8mZK+A9KEApC9BsWQfKMJa8lpe3OXrQCQgG5u8A1cEuPVEE3hpxqj3GtpHL5Cg6t
217wYwIMRcoMAIdHu5oGHeEYjMMor8aCExQfWWFlAsMBXpndjMIrQLRQbgsgN6BczELhCBCGhUEEPQC
2183QgRCpDmXNNQAoIbZBh+nzuQXcbskDfxW/zghhiEmymvfmAb7ZlPZ4ePhy4FjOehgh2phznMZEFX
2191L8yBZY7MaczuEq9bzK+ZkQOYKY0sTDwbzLEYGE7QC+mW4UXQtotiZwYwarIhOBRIrm0qVa9YVPu
220FAkUPz2OFVuY4dYFfBCig98Cl2/ZEVHfliU4nckh+CTj3uC6K7pRJI6TP58iLPoJN4JdR2e6SRbj
221whZEXjzBO4V4oB7IDKhx2oxdQ3pAhqQI9m3x9lRTvkuAWIFHI5nFDoSIIiAAj2+pGl5UF0BoHdyB
222XwBi4gihEAEAh+GI2IRKBQcqUHRywDUshSVpkD0KC5NljjM8DI6DyBjYoDJ0HkQpCVhpo9opQOwI
223TYJL1IToAG+gLOzzKthDKId6NezaW1wNcJSdF2/ATij2AHoQwpBAsAMSBaARmkgZNepGklNDPsFF
2249FEhCjgob0CEUOnzR5TQogVukfVigWfSff/WTKIi9VOnUHFguR7k6UMOzVQXNESaIW60jL8JBMjp
225Y9qAG8acTt7Crp45S7ooWgenwQOSBegd2al0IoxDark3wPiUVwSTN+809SllLzEPQJoUV2NIJd+C
226zQ1g9iPMGxGkahkjgj8t5trTaQh5I80PEQ0DejIiGXIAu0oAUUHaJoEr1Cg+RHSb8cmRKCEBIgRg
227kSAg7EYQQrVSGCCAA0oEkDgDCg1HDm3e89SFWcbwSquQrMhV6z4ENdUlkAcjzRluQ5ewZ1kGcx8U
228DFA+oq8cPtIN3kIaR0DAyGpApAkfrvdyOMkYwlBvDsQKeHRWDDqjxNtA85kCgHaCS5ofET3oc0db
229ELAS53clBYOHgS3KC8TiDV0mRMiGQcBkHEMg4hkTIhkHAZBwGQc9yDxSA+KjVeYbjmcfhb2iWJZd
230xLx+bj59OwOR+098gMEwJBlWgfZmoGzIQ0ULCwQsVSKzy1R8kYa0C5MfrR+ILWvxhzKG9SKdbpMb
231bHTTKKP9DPawEcEpQ96PUHJGkV5lp2E1XTjVfKmNmolz2AFUYeTLkG7IAHCgoW0d8waA3gzPItFB
2327gk1jXUDagrJ0QKdQYVjiNYWF7YegQdZRyTm0D54AyELkcDRAlsvQkXMhUkQY4UGqFQJAm4GJgjT
233pq3Z/KZJbiZBAYfpRW+kUGk9iPcjzR8owuVGBsbEDkSNOoe0WRCKfSHWVpRjVQMCFDP0lsEQBngQ
234HEd8vSgV0NX1j9ahfsfY1olaY5AvIkgSsxsSQAxTaiCM4a2agc9oHGZaKsrFP477F8wtMcrKohOs
235a2wO2OaPagfJojKusrVL7SOZEQ/QgSkCRvIIYkEETAAkcwvb1U8gcHTkYl7CB3obp2nt5jT32Y/o
236BNATghCBdxPuD4Ddl1CcEQx6Q2I1flz209HkQ0AJ6nqfHs6z+p3BY5UerKCGmZB11SVDDebJQfCT
2373AHpEYCh+pCgEoQ+AmZCVZACNELcgBsUTbxDGW4iRXkby4mybCCk0tT5vm9JvsPiaKesN+DIgrvD
238eJJL2fR9JcD0JBGRfUmxs3kpMeGQvHYoVA+xqKfAgIvqQFhJRJoBnCcMRKhiIt/HETN7lODrFcIt
239iSitIZnWhCgbEPJCFDh1N2ohkYGF0SCFiQFsyaBOz73gHgidYoOI0FZYKD6kZCrkyYY0Sc99RlZV
240aHyxdll1xffwuXI86WIZqjVOElTUSBo/EvItQp6SxqJlgTmge8T2CFCButuxQFl359K9i/ejwA9u
2410Pn6it7GMTY2DbbbYm4iIIIIIFiHiCUvpYJTok71FeugJ3CXDraGwbBsG02hsbSbSbTY2mxsGxsb
242yBZHUMwB5E4CDeGs4kDeUkB8eDwX0T0szfBramm00MuMg6QAZruO8PIgj9O/qY6hmG7dza8Y2aKH
243OZSDJhlZKuupFJMypApoZniQ+b3gGUfmjbbCBZbdDQCzAMIeK7VAREikHBw5A+o8OUKSAeRhOSAf
244Kgy6gyOZtDwEFim+z6vsD7E1RJEJFECpN0DMF9Z7DZ3NDagcKKGnI4IHzQgV74+r5a4AWkBigcRV
245LRMBA0hA4HQWFZQVnVWSiY2WOXroAApEO8sZglhZMFrvALgTPvAO4Am+J6nxBPdJA8tcAmREREZg
246/djRRXuk/jE09ACg1HigbgkalA7MIvBN9aM96MyU9olZpBcU1Fasvh/HFxt0mZIIRthJJ1h2oTEM
2479yWXpQLIXaCMZIwCr8CA3K4Mj7UfUXjOIkQx2o0AKXZPWRwhbGYoNsisadEcnYDoTT4lA2o3o0bg
248zWasT2o50dhqEsqN9qOgHmjvQ9iPWjuR1I3GsAhReIBi0KOfIiElAwaAaBTFG9Gj2fm1RwyAMROt
249D7UZodKMlMEIL0ZI4h4ig53HkdQJEX/L2OxA8EJzBqhD4gGhKRT/4u5IpwoSHU+r9MA=
Ian Clatworthy (ian-clatworthy) wrote :

Marius Kruger wrote:

> your right, since we're using TDD.
> I was just copying existing tests and thinking about the behaviour I'd
> like to see,
> which is by the way the BDD[1] way which I came to love.

I'm all for BDD. (I prefer it to TDD fwiw.)

> launchpad doesn't want me to push now, so I'll leave a `sleep 3600 &&
> bzr push` on and go to bed. Also attaching patch for good measure.

Thanks. I've reviewed this now and it looks ready to merge IMO.

Ian C.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-12-04 08:07:56 +0000
3+++ NEWS 2009-12-04 10:09:11 +0000
4@@ -35,6 +35,9 @@
5
6 * ``bzr ignore /`` no longer causes an IndexError. (Gorder Tyler, #456036)
7
8+* ``bzr log -n0 -rN`` should not return revisions beyond its merged revisions.
9+ (#325618, #484109, Marius Kruger)
10+
11 * ``bzr mv --quiet`` really is quiet now. (Gordon Tyler, #271790)
12
13 * ``bzr serve`` is more clear about the risk of supplying --allow-writes.
14
15=== modified file 'bzrlib/branch.py'
16--- bzrlib/branch.py 2009-12-03 02:24:54 +0000
17+++ bzrlib/branch.py 2009-12-04 10:09:11 +0000
18@@ -503,12 +503,25 @@
19 left_parent = stop_rev.parent_ids[0]
20 else:
21 left_parent = _mod_revision.NULL_REVISION
22+ # left_parent is the actual revision we want to stop logging at,
23+ # since we want to show the merged revisions after the stop_rev too
24+ reached_stop_revision_id = False
25+ revision_id_whitelist = []
26 for node in rev_iter:
27 rev_id = node.key[-1]
28 if rev_id == left_parent:
29+ # reached the left parent after the stop_revision
30 return
31- yield (rev_id, node.merge_depth, node.revno,
32+ if (not reached_stop_revision_id or
33+ rev_id in revision_id_whitelist):
34+ yield (rev_id, node.merge_depth, node.revno,
35 node.end_of_merge)
36+ if reached_stop_revision_id or rev_id == stop_revision_id:
37+ # only do the merged revs of rev_id from now on
38+ rev = self.repository.get_revision(rev_id)
39+ if rev.parent_ids:
40+ reached_stop_revision_id = True
41+ revision_id_whitelist.extend(rev.parent_ids)
42 else:
43 raise ValueError('invalid stop_rule %r' % stop_rule)
44
45
46=== modified file 'bzrlib/tests/blackbox/test_log.py'
47--- bzrlib/tests/blackbox/test_log.py 2009-06-10 03:56:49 +0000
48+++ bzrlib/tests/blackbox/test_log.py 2009-12-04 10:09:11 +0000
49@@ -536,6 +536,21 @@
50 """
51 self.check_log(expected, ['-n0', '-r1.1.1..1.1.2'])
52
53+ def test_merges_partial_range_ignore_before_lower_bound(self):
54+ """Dont show revisions before the lower bound's merged revs"""
55+ expected = """\
56+ 2 Lorem Ipsum\t2005-11-22 [merge]
57+ merge branch level1
58+
59+ 1.1.2 Lorem Ipsum\t2005-11-22 [merge]
60+ merge branch level2
61+
62+ 1.2.1 Lorem Ipsum\t2005-11-22
63+ in branch level2
64+
65+"""
66+ self.check_log(expected, ['--short', '-n0', '-r1.1.2..2'])
67+
68
69 class TestLogDiff(TestLog):
70
71
72=== modified file 'bzrlib/tests/per_branch/test_iter_merge_sorted_revisions.py'
73--- bzrlib/tests/per_branch/test_iter_merge_sorted_revisions.py 2009-07-10 05:49:34 +0000
74+++ bzrlib/tests/per_branch/test_iter_merge_sorted_revisions.py 2009-12-04 10:09:11 +0000
75@@ -84,6 +84,28 @@
76 ], list(the_branch.iter_merge_sorted_revisions(
77 stop_revision_id='rev-3', stop_rule='with-merges')))
78
79+ def test_merge_sorted_range_stop_with_merges_can_show_non_parents(self):
80+ tree = self.create_tree_with_merge()
81+ the_branch = tree.bzrdir.open_branch()
82+ # rev-1.1.1 gets logged before the end revision is reached.
83+ # so it is returned even though rev-1.1.1 is not a parent of rev-2.
84+ self.assertEqual([
85+ ('rev-3', 0, (3,), False),
86+ ('rev-1.1.1', 1, (1,1,1), True),
87+ ('rev-2', 0, (2,), False),
88+ ], list(the_branch.iter_merge_sorted_revisions(
89+ stop_revision_id='rev-2', stop_rule='with-merges')))
90+
91+ def test_merge_sorted_range_stop_with_merges_ignore_non_parents(self):
92+ tree = self.create_tree_with_merge()
93+ the_branch = tree.bzrdir.open_branch()
94+ # rev-2 is not a parent of rev-1.1.1 so it must not be returned
95+ self.assertEqual([
96+ ('rev-3', 0, (3,), False),
97+ ('rev-1.1.1', 1, (1,1,1), True),
98+ ], list(the_branch.iter_merge_sorted_revisions(
99+ stop_revision_id='rev-1.1.1', stop_rule='with-merges')))
100+
101 def test_merge_sorted_single_stop_exclude(self):
102 # from X..X exclusive is an empty result
103 tree = self.create_tree_with_merge()