[MERGE] Shelf 2/5: ShelfCreator
aaron at aaronbentley.com
Tue Oct 21 01:37:21 BST 2008
John Arbash Meinel wrote:
> Aaron Bentley wrote:
> + if names != names or parents != parents:
> + self.renames[file_id] = (names, parents)
> + yield ('rename', file_id) + paths
> + if kind != kind :
> + yield ('change kind', file_id, kind, kind,
> + elif changed:
> + yield ('modify text', file_id)
> ^- If I understand correctly, if you have a rename object that is also
> changed (kind or text) then you'll get yield 2 results for that entry,
Correct and intended. This allows the user to decide whether to shelve
the rename separately from shelving the change.
> I'm not 100% sure about the api for ShelfCreator. It seems that you
> iterate over it and make calls into 'shelf_XXX'. My concern is that it
> is acting a bit as a hybrid, rather than clearly an iterating class, or
> a class you make calls on.
> Perhaps if we just changed __iter__ to ".iter_XXX". I'm not strict about
> it, just feels a bit odd.
If that makes you happy, I can do it.
> + def read_tree_lines(self, tree, file_id):
> + """Read text lines from a tree.
> + (Tree.get_file_lines is not an official API)
> + """
> + return osutils.split_lines(tree.get_file_text(file_id))
> ^- When your other patch goes through, I'd rather not reproduce
> get_file_lines() here (I don't know any other way to remember to do that
> then mentioning it during review.)
> + def make_shelf_filename(self):
> + """Generate a filename for a shelf."""
> + transport = self.work_tree.bzrdir.root_transport.clone('.shelf2')
> + transport.ensure_base()
> + return transport.local_abspath('01')
> ^- Is this the responsibility of 'ShelfCreator' or should it be
> something you request of the WT. It feels like a WT function to me, not
> to mention I think we decided to put it into .bzr/checkout/shelf/*
> rather than .shelf2.
This is the bare minimum required to pass the test suite. It doesn't
even produce a unique name, just .shelf2/01 eternally. It is removed
when ShelfManager is introduced. But I can change it anyhow.
More information about the bazaar