storage branch - remaining issues?

[John Arbash Meinel]

Martin Pool wrote:
> 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.
> 

That is done at a different level. Internally we should have unicode.
And if that can't be encoded into the user's encoding, then 'log' should
use 'replace'. (Which is what I have done about 50% of in my encoding
branch, when I ran into the unicode filesystem issues).

But when we are reading things like commit logs, or anything under .bzr/
(which is what this function is doing) we should fail for anything that
isn't properly utf-8 encoded.

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

Yeah, I knew I mentioned those before, I'm glad to see they were already
done.

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/91dbb690/attachment.pgp