storage branch - remaining issues?

[John Arbash Meinel]

Robert Collins wrote:
> the following things in the diff look like things we should address
> before merging:
> 
> +        elif mode == 'r':
> +            # XXX: Do we really want errors='replace'?   Perhaps it
> should be
> +            # an error, or at least reported, if there's
> incorrectly-encoded
> +            # data inside a file.
> +            # <https://launchpad.net/products/bzr/+bug/3823>
> +            return
> codecs.getreader('utf-8')(self._transport.get(relpath),
> errors='replace')
> 
> There were a couple of warts left, that I have fixed up: the repository
> was not taking out any read or write locks on most of its methods, and
> LockableFiles did not take a write lock out for put[_utf8].
> 
> Other than that, I think this is good to go. I'm pushing my tweaks now.
> 
> Rob

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().

I don't mind switching to the lftp spec so that we have:
sftp://user:pass@host:port/~/relative/path
and
sftp://user:pass@host:port/absolute/path

But I would like to keep:
sftp://user:pass@host:port//absolute/path

I know that I've started using it, and it would be really nice if I
didn't have every branch complaining on me all the time.

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

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

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.

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)
+

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.

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()

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.

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.

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.

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).

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.

John
=:->
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 256 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060118/8c9b2017/attachment.pgp