[MERGE REVIEW] Move BzrBranch.get_revision_delta to Branch

[Jelmer Vernooij]

On Mon, 2006-05-08 at 16:28 -0400, Aaron Bentley wrote:
> Jelmer Vernooij wrote:
> > Any chance somebody could have a look at this patch and approve it or
> > comment? 
> 
> I don't like this method much, as I prefer the minimal-but-complete
> approach to object interfaces, and Branch definitely does not have that.
>     It has only one caller in the bzr codebase, so I'd welcome
> deprecating it, though others would doubtless disagree.
It's actually a very nice function as it allows branch implementations
that are not snapshot- but patch-based to improve performance by
overriding this function, although that would currently obviously only
work for 'bzr log'. What do you think?

> On the other hand, there are some issues with the method itself.  Fixing
> those issues would be nice, but not required for a merge.
> 
> > === modified file 'a/bzrlib/branch.py'
> > --- a/bzrlib/branch.py	
> > +++ b/bzrlib/branch.py	
> > @@ -517,6 +517,26 @@
> >          if parent:
> >              destination.set_parent(parent)
> >  
> > +    def get_revision_delta(self, revno):
> > +        """Return the delta for one revision.
> > +
> > +        The delta is relative to its mainline predecessor, or the
> > +        empty tree for revision 1.
> > +        """
> 
> The use of 'mainline' is confusing here.  Mainline usually refers to the
> main branch of a project, not to the revision-history of an individual
> branch. I think 'revision-history predecessor' would be more appropriate.
> 
> > +        assert isinstance(revno, int)
> > +        rh = self.revision_history()
> > +        if not (1 <= revno <= len(rh)):
> > +            raise InvalidRevisionNumber(revno)
> > +
> > +        # revno is 1-based; list is 0-based
> > +
> > +        new_tree = self.repository.revision_tree(rh[revno-1])
> 
> This looks redundant with get_rev_id.
Thanks, I'll get these two issues fixed.

Cheers,

Jelmer

-- 
Jelmer Vernooij <jelmer at samba.org> - http://samba.org/~jelmer/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060510/baecfc69/attachment.pgp