storage branch - remaining issues?

[Robert Collins]

just auditing to merge ... not asserting *who* did what...

On Wed, 2006-01-18 at 11:35 -0600, John Arbash Meinel wrote:
> 
> I would like to see us get rid of errors='replace'. It just covers up
> bugs, which should be fixed elsewhere.
> 
> Also, rather than using controlfile() I really think we should use
> get()
> and get_utf8(), to be consistent with put() and put_utf8().

done

> It seems that 'find_branch' has been deprecated long enough
> (especially
> since it raises a full exception). It should just be removed.

done

> needs_read_lock and needs_write_lock should get the same treatment
> that
> the 'deprecated' code has (where we set func.__name__, etc)

done

> Ultimately, we should not be doing: 't =
> get_transport(unicode(base))',
> but I think that can wait for my encoding branch, since part of my
> work
> is to make sure that transports really do require and return urls.
> Which they will then internally translate into unicode as needed.

deferred to your branch

> We have the same bug in:
> +    def peek_lock_mode(self):
> +        """Return lock mode for the Branch: 'r', 'w' or None"""
> +        raise NotImplementedError(self.is_locked)
> +

done

> This needs to raise NotImplementedError(self.peek_lock_mode).
> 
> It looks like there is a PEP8 violation in branch.py before 'def
> update_revisions' an extra space was added.

done

> If LockableFiles is not going to know what files are going to be
> created
> there (and it can't since Repository has a different set than Branch),
> we need a better api for creating stuff than:
> self.controlfiles._transport.mkdir_multi()

TODO's

> Also, BzrBranch is creating the 'revision-store' and 'weaves'
> directories, but really these should be part of the Repository
> creation.
> So probably Repository needs some initialization functions as well.

Deferred to branch-formats

> Branch.print_file() which calls Repository.print_file() which calls
> Tree.print_file() is a bad api.
> It writes directly to sys.stdout, which doesn't give us any
> opportunity
> to write to a new handle from the command line.
> It ties deeply nested code into assuming that sys.stdout is valid.

As discussed before, not a new problem - deferred with FIXME

> Inside 'put_utf8' we are iterating over the input file object, and
> encoding it into the output.
> However, file objects should only be bytestreams. They should not be a
> unicode string.
> I would say that put_utf8 should *only* accept strings not files. In
> fact, it should only accept unicode strings. But I would allow ascii
> regular strings as well.

well, it could be a little more relaxed than that, but certainly for the
transition period anality makes sense. Done.

> Which also means that we don't need 'file_iterator' since this is the
> only place it is used. We might still need IterableFile for other
> things, though.
> 
> There are the FIXME in bzrlib/store/__init__.py which say that
> allowing
> gz files should be part of TextStore, not a generic part of the store
> interface. Though right now only TextStore inherits and actually
> conforms to the Store interface. WeaveStore is a very different beast.
> (And it would be good if WeaveStore allowed gzipped files anyway).

Deferring - TODO's already and the issue is present in bzr.dev .

> Not all of these are critical, but I would at least like them made
> into
> a TODO if they aren't changed.
> 
> Overall, good job on factoring out Storage. Just some cleanup work
> left.

Merging it now that those are TODOd or done.

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/20060125/9c6aab26/attachment.pgp