No subject

and erroring at the end - error at the "beginning" if there's risk of
it happening. I'll come back to this and investigate more later.

> Also, for *me* I always use --forward, which means that the whole check
> is moot. So I'd like to see the check updated to:
> 
> 
> +        if (direction == 'forward'
> 	     or (start_rev_id
>                  and not (_is_obvious_ancestor(branch, start_rev_id,
> +                                              end_rev_id))):
> +            try:
> +                result = list(result)
> +            except _StartNotLinearAncestor:
> +                raise errors.BzrCommandError('Start revision not found in'
> +                    ' left-hand history of end revision.')
> +        if direction == 'forward':
> +            result = reversed(result)
> +        return result

Done.

> Do you have a Usertest for my personal "use it many times every day"
> version:
> 
>   bzr alias log='log --short --forward -r-10..-1'
> 
> Being selfish, that is the command I care the most about being fast. I
> guess revision_id_to_dotted_revnos() uses the mainline check first, and
> thus all of the code paths it has to take turn out to be fairly fast. It
> doesn't really seem the right way to go about it, IMO.

Added. (I don't have the numbers in front of me but it was quite a
bit better on Emacs-merges with the new patch landed.)

> +    else:
> +        # We're following a development line starting at a merged revision.
> +        # We need to adjust depths down by the initial depth until we find
> +        # a depth less than it. Then we use that depth as the adjustment.
> +        # If and when we reach the mainline, depth adjustment ends.
> +        depth_adjustment = None
> +        for (rev_id, merge_depth, revno, end_of_merge
> +             ) in view_revisions:
> +            if depth_adjustment is None:
> +                depth_adjustment = merge_depth
> +            if depth_adjustment:
> +                if merge_depth < depth_adjustment:
> +                    depth_adjustment = merge_depth
> +                merge_depth -= depth_adjustment
> +            yield rev_id, '.'.join(map(str, revno)), merge_depth
> 
> ^- I think this actually needs to be a 2-pass arrangement. Where you
> compute the overall depth_adjustment, and then adjust everything by that
> value. Otherwise I think if you did:
> 
> bzr log --long -r 1..1.2.3
> 
> It would start logging 1.2.3 with a depth_adjustment of N, but then
> finish logging mainline with a depth-adjustment of 0. Or was that the
> specific goal? (logging it as though 1.2.3 was the mainline, but using
> the numbering from the current branch tip?)

My explicit goal is that logging -r 1..1.2.3 will show 1.2.3 at depth 0,
it's merged revisions nested appropriately. We're genuinely treating
1.2.3 as the "tip" of the development line we're logging here. Using
log --short -r1..1.2.3 will show that development line as flat and
log --long -r1..1.2.3 should show the same line with any nested
revisions below each merge.

A multiple pass algorithm isn't required IMO. It also scales O(result)
eliminating any chance of incremental output.

> I thought the original depth_adjustment work was just to handle that
> doing "bzr log -r 1.2.1..1.2.3" ended up indented everywhere, which
> seemed silly.

It was.

> +def calculate_view_revisions(branch, start_revision, end_revision,
> direction,
> +        specific_fileid, generate_merge_revisions,
> allow_single_merge_revision):
> +    """Calculate the revisions to view.
> +
> +    :return: An iterator of (revision_id, dotted_revno, merge_depth)
> tuples OR
> +             a list of the same tuples.
> +    """
> +    # This method is no longer called by the main code path.
> +    # It is retained for API compatibility and may be deprecated
> +    # soon. IGC 20090116
> 
> ^- If we aren't using it anymore, I'd rather deprecate it now rather
> than waiting.

I can't just yet because I'm pretty sure its used inside bzrlib for
missing.py say. We also need to replace it with a public API we *do*
want to support for bzrlib clients. There's a preview of what I'm
thinking of (LogGenerator.iter_log_revisions()) in my log-dir branch
above BTW, but that requires some debate with others (e.g. Vincent)
and some evaluation by potential users like LoggerHead and qlog before
it's made public. Oh, and dozens and dozens of interface tests. :-)

I'll be sure to raise this in the covering letter for my log-dir patch.

>      def test_merges_nonsupporting_formatter(self):
> +        # This "feature" of log formatters is madness. If a log
> +        # formatter cannot display a dotted-revno, it ought to ignore it.
> +        # Otherwise, a linear sequence is always expected to be handled
> now.
> +        raise KnownFailure('log formatters must support linear
> sequences now')
> 
> ^- I don't understand this change.

The new code is less dumb than the old code so an overly restrictive test
now fails. I need to ping the author that introduced this "feature" to
see if his/her log formatter can be changed to make this (to me) weird
limitation disappear. I can only guess that the driver was something like
wanting a fixed-width revno column and the old dotted scheme (1.1.1.1.1)
caused problems for logs of size > 1. Anyhow, removing this is on my
list, just not near the top yet.

Once again, thanks for the insightful review. Apologies in sending this
delayed response.

Ian C.