Code review comment for lp:~mbp/bzr/368931-rename-case-2.0

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

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/368931-rename-case-2.0 into lp:bzr/2.0.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #368931 Rename may fail when file and directory have the same name differing by case
> https://bugs.launchpad.net/bugs/368931
>
>
> This finishes off Chris Jones's patch in https://bugs.edge.launchpad.net/bzr/+bug/368931 by adding a test.
>
> This function could be faster but I'm not sure if it's actually a hot spot.
>
> This finishes our last high-priority bug with a patch! There are still 34 more with lower importance.
>

I agree with your TODO and NOTE comments.

Also using "iter_children()" throws out all sorts of information. It
returns only file_ids, when it already looked up everything as a
InventoryEntry. Also, falling back to 'id2path' is wasteful given that
we already have the containing directory, and all we need is the child
name. (Which is on the IE we just through away from iter_children.)

I would also guess that there is still a bug if "Tree" is versioned but
"tree" is not (or vice versa).

It sort of depends if this is only supposed to work on versioned paths,
or work on both versioned and unversioned files.

(If tree is versioned but Tree is not, and you do "bzr status Tree",
what should it match?)

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkt0iSoACgkQJdeBCYSNAANrWgCgxtR1cfkQPV0VEEKqD6P186ad
uxgAn1rpE4/PtDIKhNPxC4joAiVBjTW2
=PvyE
-----END PGP SIGNATURE-----