[MERGE] Fix for bug 183831

[Aaron Bentley]

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

Thanks for your submission.  Sorry no one has reviewed this earlier.

bb:comment

Geoff Bache wrote:
> I've made a small change to osutils.relpath so that it can handle
> symbolic links in a better way. The main point is to fix the myriad
> problems that can occur when Bazaar is given a repository path
> containing symbolic links

Actually, that's not a repository path.  That's a working tree path.  In
Bazaar, a working tree has a branch and a branch has a repository.  The
working tree's branch and repository are not necessarily co-located with it.

>, see
> https://bugs.launchpad.net/bzr/+bug/183831 and the various duplicates.
> 
> The fix is available as a branch on launchpad,
> https://code.launchpad.net/~geoff.bache/bzr/trunk. I guess that's
> sufficient, let me know if not.

Generally, we prefer merge directives (i.e. "bzr send" output).  We also
accept patches.  An email without any diff makes the review harder.

> I added a new blackbox test, test_add_with_symlink, which is skipped
> on platforms that don't have symbolic links.

New tests that require symlinks should do
self.requireFeature(SymlinkFeature)

> I haven't added any unit
> tests because I don't know how meaningful they would be.

A unit test should always be added, unless you're working on a command
class itself.  Many things use osutils.relpath, so it's not appropriate
to only test "add".

> I ran all the
> tests and got 2 failures, which are the same 2 failures I get with the
> code unchanged.

That's odd.  Since our suite is always run before we commit to bzr.dev,
there's no reason it should fail.

> It could be advanced that this change might slow things down: after
> all it adds complexity to quite a low-level function, and
> os.path.samefile will hit the file system whereas the old function was
> purely string manipulation.

What would really help here is an explanation of why calling
os.path.samefile is better than string manipulation.

> In practice I doubt it will make much
> difference as it shouldn't be called too repeatedly, but I'm open to
> being told otherwise.

It is called repeatedly.

> Time to run all tests and all benchmarks was not
> noticeably different in any case.

Time to run "bzr branches" on bzr.dev (best of 3)
real	0m2.105s
user	0m1.976s
sys	0m0.128s

Time to run on your branch, with bzr.dev merged in (best of 3)
real	0m2.122s
user	0m2.008s
sys	0m0.108s

So that's 1.008x slower in overall time, 1.18x slower in system time.
Other tests will show more or less degradation, depending on what they do.

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

iEYEARECAAYFAknRFd4ACgkQ0F+nu1YWqI0+swCfbc1A3kg42pIVZX4pDD4NiEEE
MhsAnA0QvQ3eUJOhzQ35zM8s5xY4UC16
=V2pP
-----END PGP SIGNATURE-----