[MERGE] Fix for bug 183831

[Geoff Bache]

Hi Aaron,

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

OK, sure. I'll know for next time.

>> 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".

OK. But I'm unsure of what you mean by a unit test in this case. I can
see 2 possible routes:
1) Create symlinks for real in the file system. But many unit test
suites ban touching the file system: if everyone does this the unit
tests will be just as slow as the blackbox tests.
2) Write a fake version of os.path.samefile in the test. That will be
faster, but then it's very tied to my current implementation and the
use of that method.

In any case, further testing should surely wait until somebody has
pronounced on whether the performance is acceptable or not.

>
>> 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.
>

On the latest source I have 4 failures. All seem to depend on plugins
I have installed, and they work with "bzr --no-plugins selftest", an
option I only just discovered. To me it would seem useful if
--no-plugins was the default for
"bzr selftest", as it seems like there is plenty of scope for this
kind of thing to happen otherwise. Or at least for this issue to be
mentioned in the Guide to Testing Bazaar to warn the unsuspecting
newbie that they probably want to run with that flag if they want
their tests to work.

(The offending plugins are "scmproj" and "cvsps_import" if you're interested).

>> 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.

I don't really follow you.
I don't know any way to check for  the existence of symbolic links by string
manipulation...
Isn't the fact that it fixes this issue enough for it to be "better"...?

>> 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.

What is "bzr branches"? It's not a command my system recognises. I'd
like to test this out myself if I can: 3 samples doesn't sound very
much to me for something that only takes 2 seconds anyway.

Regards,
Geoff Bache