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

[Wouter van Heyst]

On Fri, Jun 16, 2006 at 01:08:01PM -0500, John Arbash Meinel wrote:
> 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.

That was my reasoning too.

> 
> 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()".

Done.

> 
> > +
> > +    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[:]).

I often make use of that, will try not to in bzr code. The function in
question is dead code in this instance of the patch, I removed
find_merging and inlined last_mainline_ancestor now that there is only
one user.

> 
> 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.

I agree the above functions are convoluted, I tried getting rid of them.
How bad is the rest of the patch? I dislike the cutting out of revisions
with the sorted_merge loop, nor the zero_depths calculation.

> It might just be that I'm confused about you using the term 'ismerged()'
> when it doesn't really mean merged.

Oops, I don't know how I ended up reversing the logic on that one. 
I'm working on an updated patch, will post that after dinner, and then
get to work on actually changing cmd_log.

Wouter van Heyst