Tree Transform passing all tests, plus abuse

[John A Meinel]

Aaron Bentley wrote:
> I've uploaded a new version of Tree Transform, with many of these issues
> addressed.  In particular, win32 support has been tested and fixed.
> 
> John A Meinel wrote:
>>> In TreeTransform.__init__() you use a plain os.mkdir(), we should
>>> probably trap for some sort of exception, at least so we can raise a
>>> more informative one.

You haven't pushed your latest bzr.ttransform work. So I can't review
your latest changes. (I only have up to revno 1441)

> 
> This is done.
> 
>>> You do the same thing in finalize(), where you don't handle any
>>> exceptions. This may be fine, just wanted to bring it up.
> 
> I'm now throwing an informative exception.
> 
>>> There are some PEP8 issues in transform.py
> 
> I think they're fixed.  Please let me know if I've missed some.
> 
>>> In test_transform.py you Branch.initialize and then chdir('..'). I think
>>> it would be better if we create a subdirectory branch, rather than
>>> changing directory outside of the Test working directory.
> 
> Done.
> 
>>> Some of the function names on TreeTransform aren't quite intuitive.
>>> 'get_id_tree' and 'get_trans_id', 'get_tree_file_id', etc
> 
> I still don't have a good idea what changes to make here.

I think we just need to come up with a consistent naming convention.

> 
>>> The documentation for final_kind() (and a few other functions) uses """\
>>> rather than putting the documentation on the same line.
> 
> Changed.
> 
>>> Internally you use "os.path.join" rather than "osutils.pathjoin". Which
>>> means we would end up with '\' separated paths.
> 
> I haven't changed this.  I'm not sure about the wisdom of using
> non-native path separators.  I believe this is a very different case
> from using '\' in transports.
> 
> Is there a reason not to use the native path separators?

As long as the paths never sneak out of your tree-transform code, it
won't be a problem. The issue is that anything we display to the user,
or process in other commands should use '/'.

About the only way that I made sure this happened correctly was by
replacing all of the os.path.join functions with osutils.pathjoin, which
forces the separators to be '/'.

I just don't want to have to worry about whether this path is forward
slashed or backslashed when I'm handling it. And just know that if I'm
inside bzrlib, it is forward slashed.

In my testing, the only thing that fails with forward slashes in
spawning an external command. For that, you have to give back slashes.
IIRC this is because win32 supports doing stuff like cd.., or dir/p
where you don't separate the command from its arguments. So spawning the
command path/to/foo is actually spawning the command 'path' with
parameters /to and /foo. (Yes, it is really ugly and crappy, but that is
the way it is done).

Windows says '/' can't be a part of the path, so it must be an option (I
guess). I'm not sure how it handles the '..' case. But "dir.." also
works. And dir../p is the same as "dir .. /p".

> 
>>> The changes include your "defaults" changes. I like the idea of
>>> defaults, but I don't know if the changes have been reviewed.
> 
> I've yanked all the changes that originated in bzr.ab and weren't merged
> in your integration branch.
> 
>>> For 'supports executable' should we use os.name != 'nt', or sys.platform
>>> == 'win32'
> 
> I went with sys.platform != 'win32'.
> 
> So I hope the remaining issues are
> 1. some method names could be improved
> 2. uses native path separators
> 
> Is there anything else?
> 
> Aaron

Push your work, then I'll look over it. I don't know of anything else
offhand.

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 249 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060214/6a97c581/attachment.pgp