PING [MERGE] Root entry has a revision id

[John Arbash Meinel]

Aaron Bentley wrote:
> John Arbash Meinel wrote:
>>> Aaron Bentley wrote:
>>>
>>>> I haven't heard anyone's opinion of this, and I think it's a bit too
>>>> major for me to just merge without review.
> 
>>> I wasn't commenting on it, because it has too much of the root == None
>>> stuff, which you need to sort out between you and Robert.
> 
> This change doesn't prejudice our decision about whether the origin
> revision a root entry.  My motivation in introducing
> Inventory(root=None) is to regularize the code.  It reduces the number
> of places where we need to special-case the root entry.
> 
> So I don't think that this depends on the outcome of Robert's and my
> discussion.

I suppose you mean that while Inventory can have a root of None, Trees
may not be able to?

> 
>>> The one thing I didn't like was using 'NewCommitBuilder' as the class
>>> name. If it is truly meant to be temporary, then I think using
>>> '_CommitBuilder' would be better.
> 
> I can do that.

I don't really know what is best here. But since classes shouldn't
inherit from NewCommitBuilder, it should be private. I also don't want
to see another 'BzrNewError' situation where 'New' is permanently
encoded into the API for compatibility. 'new' is rarely a good word to
have in apis. :)

> 
>>> Though the way you were writing it, you really should have done the
>>> changes in NewCommitBuilder, rather than in the base class.
> 
> You mean, have CommitBuilder be the new class, deriving from
> _CommitBuilder, and overriding the record_root_entry to False?
> 
>>> Otherwise you are changing a base implementation, away from classes that
>>> are using it.
> 
> Since CommitBuilder is a public symbol, I didn't want to change its
> behaviour, if I could avoid it.  The way I've written these changes,
> classes that derive from CommitBuilder should continue behaving the same
> way, but they can set record_root_entry True when they update to the new
> API.
> 
> Aaron

Well, my point was that you should have a new class, which inherits from
CommitBuilder, and implements the correct API. As is, CommitBuilder
implements the correct API, so depending on what children override, they
may or may not get the right API (just their foo.supports_root_... will
not be set to True).

I'm not super concerned about plain CommitBuilder implementing the right
api. But to truly do api compatibility, CommitBuilder shouldn't change.

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/20060814/332f3e5a/attachment.pgp