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

[Marius Kruger]

[Marius Kruger (amanica)]

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)]

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)]

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)]

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

[Ian Clatworthy (ian-clatworthy)]

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.

[Ian Clatworthy (ian-clatworthy)]

review: Approve

=== modified file 'NEWS'
--- NEWS	2009-12-04 08:07:56 +0000
+++ NEWS	2009-12-04 10:09:11 +0000
@@ -35,6 +35,9 @@
 
 * ``bzr ignore /`` no longer causes an IndexError. (Gorder Tyler, #456036)
 
+* ``bzr log -n0 -rN`` should not return revisions beyond its merged revisions.
+  (#325618, #484109, Marius Kruger)
+
 * ``bzr mv --quiet`` really is quiet now.  (Gordon Tyler, #271790)
 
 * ``bzr serve`` is more clear about the risk of supplying --allow-writes.
 
=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2009-12-03 02:24:54 +0000
+++ bzrlib/branch.py	2009-12-04 10:09:11 +0000
@@ -503,12 +503,25 @@
                 left_parent = stop_rev.parent_ids[0]
             else:
                 left_parent = _mod_revision.NULL_REVISION
+            # left_parent is the actual revision we want to stop logging at,
+            # since we want to show the merged revisions after the stop_rev too
+            reached_stop_revision_id = False
+            revision_id_whitelist = []
             for node in rev_iter:
                 rev_id = node.key[-1]
                 if rev_id == left_parent:
+                    # reached the left parent after the stop_revision
                     return
-                yield (rev_id, node.merge_depth, node.revno,
+                if (not reached_stop_revision_id or
+                        rev_id in revision_id_whitelist):
+                    yield (rev_id, node.merge_depth, node.revno,
                        node.end_of_merge)
+                    if reached_stop_revision_id or rev_id == stop_revision_id:
+                        # only do the merged revs of rev_id from now on
+                        rev = self.repository.get_revision(rev_id)
+                        if rev.parent_ids:
+                            reached_stop_revision_id = True
+                            revision_id_whitelist.extend(rev.parent_ids)
         else:
             raise ValueError('invalid stop_rule %r' % stop_rule)
 
 
=== 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-12-04 10:09:11 +0000
@@ -536,6 +536,21 @@
 """
         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's merged revs"""
+        expected = """\
+    2 Lorem Ipsum\t2005-11-22 [merge]
+      merge branch level1
+
+          1.1.2 Lorem Ipsum\t2005-11-22 [merge]
+                merge branch level2
+
+              1.2.1 Lorem Ipsum\t2005-11-22
+                    in branch level2
+
+"""
+        self.check_log(expected, ['--short', '-n0', '-r1.1.2..2'])
+
 
 class TestLogDiff(TestLog):
 
 
=== modified file 'bzrlib/tests/per_branch/test_iter_merge_sorted_revisions.py'
--- bzrlib/tests/per_branch/test_iter_merge_sorted_revisions.py	2009-07-10 05:49:34 +0000
+++ bzrlib/tests/per_branch/test_iter_merge_sorted_revisions.py	2009-12-04 10:09:11 +0000
@@ -84,6 +84,28 @@
             ], list(the_branch.iter_merge_sorted_revisions(
                 stop_revision_id='rev-3', stop_rule='with-merges')))
 
+    def test_merge_sorted_range_stop_with_merges_can_show_non_parents(self):
+        tree = self.create_tree_with_merge()
+        the_branch = tree.bzrdir.open_branch()
+        # rev-1.1.1 gets logged before the end revision is reached.
+        # so it is returned even though rev-1.1.1 is not a parent of rev-2.
+        self.assertEqual([
+            ('rev-3', 0, (3,), False),
+            ('rev-1.1.1', 1, (1,1,1), True),
+            ('rev-2', 0, (2,), False),
+            ], list(the_branch.iter_merge_sorted_revisions(
+                stop_revision_id='rev-2', stop_rule='with-merges')))
+
+    def test_merge_sorted_range_stop_with_merges_ignore_non_parents(self):
+        tree = self.create_tree_with_merge()
+        the_branch = tree.bzrdir.open_branch()
+        # rev-2 is not a parent of rev-1.1.1 so it must not be returned
+        self.assertEqual([
+            ('rev-3', 0, (3,), False),
+            ('rev-1.1.1', 1, (1,1,1), True),
+            ], list(the_branch.iter_merge_sorted_revisions(
+                stop_revision_id='rev-1.1.1', stop_rule='with-merges')))
+
     def test_merge_sorted_single_stop_exclude(self):
         # from X..X exclusive is an empty result
         tree = self.create_tree_with_merge()