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

[Vincent Ladeuil]

>>>>> "jam" == John Arbash Meinel <john at arbash-meinel.com> writes:

<snip/>

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

Yes.

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

Yes, it does give the right result.

If we change the order as suggested in:

    vila> The deeper issue, IMHO, is that displaying merged
    vila> revisions *after* the mainline one is wrong. The
    vila> display should respect the order required by the user
    vila> (--forward or not).

.. it will need to be upgraded accordingly though.

<snip/>

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

Well, it was used only in this test, but changing it to a method
doesn't make it less readable, so I did as suggested.

    jam> ...

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

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

grep says: assertEquals: 724, assertEqual: 6314

assertEqual wins, but some work still to be done :) 

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

    jam> self.assertEqual(10, variable_to_check)

    jam> rather than

    jam> self.assertEqual(variable_to_check, 10)

    jam> In some ways the latter form "flows" more naturally if you read it as a
    jam> sentence, but I guess the xUnit standard is to put the correct value
    jam> first. And regardless, we should try to be consistent,

Key point: consistency. I remember having a hard time reading the
tests months ago before realizing the lack of consistency
regarding that particular point. Looks like I'm less disturbed
these days or I would have noticed these ones :)

    jam> as it makes test failures easier to understand (you know
    jam> which side of "!=" is the correct value, and which is
    jam> the wrong value.)

You're preaching to the choir :)

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

Sure.

Done. It was a far more aggressive change than I thought, but
worth it :)

    jam> ...

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

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

Done.

        Vincent