storage branch - remaining issues?

[Martin Pool]

On 18 Jan 2006, John Arbash Meinel <john at arbash-meinel.com> wrote:
> 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.

I agree it should be avoided, but there might be some specific
exceptions such as displaying log or patch output to a terminal.

> Also, rather than using controlfile() I really think we should use get()
> and get_utf8(), to be consistent with put() and put_utf8().

I agree.

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

OK, good.  Perhaps that syntax should just be accepted as the absolute
path "//absolute/path".  (It might do something different on unix
systems that have UNC-like //host/path escapes, but making that
accessible is probably good.)

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

That's done in my storage branch after your previous review but  I had
neglected to push it up.  It's now at 

  http://people.ubuntu.com/~mbp/bzr.mbp.storage/

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

Also fixed in my branch.

-- 
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060119/4296c8b2/attachment.pgp