[RFC] _show_log supporting unmerged revisions (first step towards lp #4663)

[John Arbash Meinel]

Wouter van Heyst wrote:
> Struggling with #4663 Robert proposed two views of doing the right
> thing. 
> 
> One is showing the log of the mainline, from the revision that *merged*
> BEGIN to the revision that *merged* END
> 
> Initially I thought this was not the right thing. If you want to see the
> log of a subset from a large merge, you don't want to drown in all the
> other numerous revisions.
> 
> The other view is along the mainline *of END*. Same start as above, but
> stop at END and treat revisions since the last merged revision as
> mainline also. This allows viewing the log as it would look from an
> arbitrary HEAD. Necessarily you will not see mainline revisions that are
> commited before the merge of END but are not in its ancestry.
> 

I favor this second method. It may do weird things when you switch the
direction, but I think it gives the best results.
The real use case (I think) is just doing:

bzr log -r revid:foo

With no range, because you just want the message about a specific entry.
If you used the former method, that would give you all the stuff about
the revision which merged the revision you care about. Rather than just
a single entry.

There are some bugs in your code which I'll bring up:

> # Bazaar revision bundle v0.7
> #
> # message:
> #   Make _show_log handle merged revisions
> # committer: Wouter van Heyst <larstiq at larstiq.dyndns.org>
> # date: Tue 2006-06-13 17:12:38.152195930 +0200
> 
> === modified file bzrlib/log.py
> --- bzrlib/log.py
> +++ bzrlib/log.py
> @@ -189,50 +189,100 @@
>      else:
>          searchRE = None
>  
> -    which_revs = _enumerate_history(branch)
> -    
> +    revision_history = branch.revision_history()
> +
> +    def ismerged(rev):
> +        """Check if a revision is in the mainline of a branch."""
> +
> +        return rev in revision_history

Checking existence in a list is slow, especially if you have 1500
entries. Better to use a set.
At the same time, the function isn't named well. This is really
"is_mainline()".

> +
> +    def last_mainline_ancestor(rev):
> +        """Return the last mainline ancestor of rev."""
> +
> +        an = reversed(branch.repository.get_ancestry(rev))
> +        revno = None
> +        for ancestor in an:
> +            if ismerged(ancestor):
> +                revno =  branch.revision_id_to_revno(ancestor)
> +        if revno is None:
> +            raise errors.RevisionNotPresent(rev, branch)
> +        return revno
> +
> +    def find_merging(merged, revno=None):
> +        """Find the mainline revno that merged us.""" 
> +
> +        for merging in revision_history[revno:]:
> +            if merged in branch.repository.get_ancestry(merging):
> +                return branch.revision_id_to_revno(merging)
> +
> +

I didn't realize that list[None:] worked. It seems to be interpreted as
though you didn't pass anything (list[None:None] == list[:]).

In general, the above seems to be a little convoluted and hard to
follow. It would be nice if we could get a simpler definition, so that
it can be maintained.
It might just be that I'm confused about you using the term 'ismerged()'
when it doesn't really mean merged.

John
=:->


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060616/127480c5/attachment.pgp