Tree Transform passing all tests, plus abuse

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Thanks for your review.

I've now tested it on win32, and I'm sorry to say there are a few cases
where we're getting bitten by the different rename behavior.  I'm going
to make it a conflict on *nix to create a new file without removing the
old one, first.

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.

I think this is sensible.  Will do.

| You do the same thing in finalize(), where you don't handle any
| exceptions. This may be fine, just wanted to bring it up.
|
| There are some PEP8 issues in transform.py
|
| 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.
|
| I'm still looking over the code, but I thought I would point out a
| couple of things first.


|
|
| A couple of more code statements...
|
| Some of the function names on TreeTransform aren't quite intuitive.
| 'get_id_tree' and 'get_trans_id', 'get_tree_file_id', etc

The design did evolve somewhat as I was working on it.  I'll explain
these in the hope that someone can come up with better explanations.

get_id_tree is 'get the transform id in of the file in the working
tree'.  get_trans_id will return any existing transform id for a file
id, or assign a new transform id if needed.

| I also think it isn't quite clear when you would use "create_path"
| versus "create_file"

You would usually use them in the same place.  create_path associates a
path with a new transaction id.  create_file schedules the creation of a
file on a transaction id.  new_file is the high-level function.

| I don't have a great suggestion, but I know it was a little bit
| difficult to read test_transform.

I know what you mean.

| The documentation for final_kind() (and a few other functions) uses """\
| rather than putting the documentation on the same line.

Is that a violation?  It reads more nicely to me, but I can adjust.

| Internally you use "os.path.join" rather than "osutils.pathjoin". Which
| means we would end up with '\' separated paths.

Is that a problem?  This is all local fs stuff.

| The changes include your "defaults" changes. I like the idea of
| defaults, but I don't know if the changes have been reviewed.

Yeah, when I merged those changes, I was assuming they were a slam dunk.
~ They can come out, if necessary.

| You fixed a lot of PEP8 in errors.py, and added a couple more errors:)

Again, those changes are in bzr.ab.

| For 'supports executable' should we use os.name != 'nt', or sys.platform
| == 'win32'
|
| I don't know if win9x are supported platforms,

I believe they're not.

| but supports_executable()
| would say that they do support the execute bit (when they don't).
|
|
| All this said, the actual code changes themselves look pretty good. And
| the proof is that the code works and does what we want. :)

Glad I'm on the right track.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFD7i5s0F+nu1YWqI0RAjACAJ9ulrbvnU1RplsizvUNspP98qd7hwCfc4gO
yzQ2k7UEqBMLOJmqQ3BwUns=
=6zqr
-----END PGP SIGNATURE-----