[MERGE][bug #300055] Fix log forward revision range for specific file-id

[John Arbash Meinel]

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

Vincent Ladeuil wrote:
> bzr was back tracing in a very narrow case because we used
> reverse_by_depth in a way it was not designed to support:
> reversing a revision list already reversed.
> 
> If that sounds unclear... the root cause is that reverse_by_depth
> groups revisions merged into a mainline revisions *after* that
> mainline revision.
> 
> This works as long as the list includes mainline revisions for
> any merged revision. 
> 
> The bug exposed a case where that wasn't verified.
> 
> The fixes included in that patch make that case (and other similar
> ones) "work".
> 
> The deeper issue, IMHO, is that displaying merged revisions
> *after* the mainline one is wrong. The display should respect the
> order required by the user (--forward or not).
> 
> Since there are other issues with our revision displays, I'd like
> to address that point in a followup patch and just get these
> fixes merged as is.
> 
> Also note that the patch is pretty verbose (noisy ?) because I
> updated the tests to our current standards, fixing one bogus test
> in the process (the code and the comment were disagreeing and the
> comment was right :-/).
> 
> Here is an excerpt highlighting the most important points:
> 
> === modified file 'bzrlib/log.py'
> --- bzrlib/log.py	2008-10-01 05:40:45 +0000
> +++ bzrlib/log.py	2008-11-25 08:41:28 +0000
> @@ -245,9 +245,6 @@
>      if not mainline_revs:
>          return []
>  
> -    if direction == 'reverse':
> -        start_rev_id, end_rev_id = end_rev_id, start_rev_id
> -
>      generate_single_revision = False
>      if ((not generate_merge_revisions)
>          and ((start_rev_id and (start_rev_id not in rev_nos))
> @@ -260,6 +257,9 @@
>          generate_merge_revisions = generate_single_revision
>      view_revs_iter = get_view_revisions(mainline_revs, rev_nos, branch,
>                            direction, include_merges=generate_merge_revisions)
> +
> +    if direction == 'reverse':
> +        start_rev_id, end_rev_id = end_rev_id, start_rev_id
>      view_revisions = _filter_revision_range(list(view_revs_iter),
>                                              start_rev_id,
>                                              end_rev_id)

^- You are still passing "direction" into 'get_view_revisions', which
means that the "view_revs_iter" should be coming out reversed.

I'm pretty sure that means that "filter_revisions_touching_file_id" will
be wrong. Though perhaps because of how reverse-by-depth works it still
gives the right result?


...

- -    def test_simple_log(self):
- -        eq = self.assertEquals
- -
+        log.show_log(b, lf, verbose=True, start_revision=1, end_revision=1)
+
+        def assertInvalidRev(start, end):
+            self.assertRaises(errors.InvalidRevisionNumber,
+                              log.show_log, b, lf,
+                              start_revision=start, end_revision=end)
+


^- We generally don't do local functions for this, can you put it as a
class-level function and call it that way?

...

+        self.assertEquals(lf.logs, [])

^- *I* have taken to using "assertEqual" without the 's'. I don't think
there is a standard, though.

Also, if you are going to be changing it, we use the "true" value as the
first parameter. So we do:

self.assertEqual(10, variable_to_check)

rather than

self.assertEqual(variable_to_check, 10)

In some ways the latter form "flows" more naturally if you read it as a
sentence, but I guess the xUnit standard is to put the correct value
first. And regardless, we should try to be consistent, as it makes test
failures easier to understand (you know which side of "!=" is the
correct value, and which is the wrong value.)

Normally I wouldn't care, but if you are going to be changing all these
lines anyway...

...

- -        show_log(b, lf, verbose=True)
+        log.show_log(wt.branch, lf, verbose=True)
         committed_msg = lf.logs[0].rev.message
- -        self.log("escaped commit message: %r", committed_msg)
         self.assert_(msg == committed_msg)

^- This should certainly be changed to "self.assertEqual". I don't think
we use self.assert_ in standard practice. At a minimum those should be
changed to "self.assertTrue".

BB:tweak

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

iEYEARECAAYFAkksI2AACgkQJdeBCYSNAAP7tACgqSrq9m3FnBjSGuv9ZgI2GyNU
xskAnibvVlK5wqogD1+24Mq2dI3YoXko
=FLEx
-----END PGP SIGNATURE-----