Code review comment for lp:~jspashett/bzr/322807_add_root_drive_windows

Revision history for this message
John A Meinel (jameinel) wrote :

This turns out to not be the right fix.

Specifically, 'canonical_relpath' is meant to work even for nested values. For example:

canonical_relpath('C:/foo', 'C:/foo/bar/baz')
should return 'bar/baz', while your form only returns "baz".

There is also a few other things, like getting better testing of this function. It is slightly tested in bzrlib/tests/test_osutils.py but not particularly well (only indirectly via canonical_relpath() tests.)

When I started looking at writing tests, I then found that it requires 'abs_base' to exist, or you get an exception during list_dir(). Which is somewhat reasonable for real-uses, but makes testing harder than it needs to be.

So I'll go ahead and look at this for a bit, and propose an alternative patch, if that is ok with you.

review: Needs Fixing

« Back to merge proposal