[MERGE] deprecate find_previous_heads

[Robert Collins]

On Mon, 2007-09-03 at 12:41 -0400, Aaron Bentley wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> bb:tweak
> 
> Robert Collins wrote:
> > +    * Deprecated method ``find_previous_heads`` on
> > +      ``bzrlib.inventory.InventoryEntry``. This has been superceded by the use
> 
> Actually, "superseded" uses an "s" in the middle.

http://en.wiktionary.org/wiki/supercede. Alternative spellings since
1400's :).

I'll change it as you have other edits you've asked for too.

> > +    * New public method ``heads`` on the ``bzrlib.graph.Graph`` class. This is
> > +      a simple rename of a previously internal method which implements the same
> 
> I think "previously-internal" makes the grouping clearer.

K.

> >          """
> > +        candidate_lca = set(keys)
> 
> ^^^ It's not a blocking issue, but it would be nice to rename
> candidate_lca.  It's historical raisins now.

I can do that, I was keeping the diff minimal for clarity was all.

> Also, should Graph.heads make any ordering guarantees?  If not, we
> should document that.

I think it is fine not to make any at this point, I'll note that on the
api.

> 
> >         - The kind has not changed (* not sure why this is a clause *)
> 
> Well, gannotate says John did it to fix bug 90111.
> 
> It makes perfect sense if you're viewing the versionedfile merely as a
> content store, because if the leftmost ancestor is a different kind,
> your delta compression will be horrible.  But if the rightmost ancestor
> is of the same kind, you'll typically get much better delta compression
> if it becomes the leftmost ancestor.
> 
> I think this is yet another example of the impedence mismatch between
> "versionedfiles as content stores" and "versionedfiles as file ancestry".

I agree. In packs I have decoupled the fields: the file ancestry graph
is stored separately to the compression chain. The knit layer doesn't
know how to use that separation yet though.

What should we do at this point? I'm inclined to not worry about the
compression and use the correct graph - that is remove the kind check as
I'm convinced its bogus. Compression wise that will hit each branch that
had the link once and only once.

> > +        # --- what follows is now encapsulated in repository.get_graph.heads(), 
> > +        #     but that is not accessible from here as we have no repository
> > +        #     pointer.
> 
> We do have a way to get versionedfiles, and an idea how to convert
> versionedfiles into Graphs, but it's probably not worth it for a
> deprecated method.

Right, except I'm changing to use the Repository graph not the per file
graph for heads generation at the same time. That is technically
unrelated but as you're using that in reconcile it makes sense to me to
be consistent about this.

> Given that you're introducing a new public method, it would be nice to
> test it.  Technically, you're not reducing test coverage, but now a
> significant chunk of the testing is testing its use in a deprecated
> method.  When that deprecated method goes away, its tests will, too.
> And then test coverage will be reduced.

I dithered on this. heads() on graph is easy to check, so I'll add some
tests for that from johns heads patch which was rejected.

The per-tree tests actually to me seem better as repository tests,
because each repository only has one type of tree and its only
historical trees that last-modified applies to. So I'll port them over,
I had been a bit conflicted about this.

-Rob


-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- 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/20070904/770900c6/attachment.pgp