[MERGE] [BUG #4663] Fix ``bzr log -r`` to support selecting merge revisions.

[Andrew Bennetts]

Kent Gibson wrote:
[...]
> >> +        if isinstance(start_revision,RevisionInfo):
> >
> > PEP 8 says there ought to be a space after the comma.
> >
> jam has mentioned that as well, but I still haven't found the relevant
> section in PEP8.
> (Sorry - waving PEP8 at me is like a red rag to a bull.)
> The only section that gets close refers to avoiding extraneous
> whitespace, not enforcing its presence.  While all the example code in
> PEP8 does include the space you refer to, I still don't believe it
> says it is a must.

Hmm, you're right, the current version of PEP 8 doesn't seem to say it
explicitly.  It does fairly clearly imply it by all of its examples though.

> But rest assured that my future patches will include said space, since
> that seems to be the accepted bzr style.

Thanks!

[...]
> > It would be good to add a comment about the filter stack (especially
> > the ordering), just to make the way the code works a bit clearer.
> True enough, though I generally prefer tests to comments.  I suspect
> if you change the order the unit tests will break.  But if so it is
> due to luck more than good planning.
> So probably should add an explicit unit test to check stacked filters.

The key thing is to make sure that the intent of the code — and the tests — is
clear.  If someone changes the order of the filtering, and they see test
failures, will it be clear to them why they are failing?

So yeah, I think being explicit (whether in a comment, or by adding a specific
unit test with a clear purpose) would be good idea.

> > You have minor conflicts with current bzr.dev; I'll resolve them and
> > send the merge request.
> Well there wasn't two months ago when I wrote it :-).  I wouldn't be
> at all surprised if there are conflicts with my unit test changes for
> the verbose merge revisions.

Right, that's partly why I tidied up the conflicts rather than asking you to do
it... it didn't seem fair to take so long to review it and then force you to
tidy up the conflicts that were my fault (for not reviewing sooner)!

(The other reason was so that it would land in time for 0.18, it would have been
a shame if the only thing that stopped this branch landing in time was a few
fairly simple conflicts.)

-Andrew.