[MERGE] Fix for bug 183831

[Geoff Bache]

Oops, forgot to reply to all...

>> 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.
>
> bzr itself includes plugins, so a complete run of the test suite must
> include plugins.  We also want to encourage plugin authors to keep their
> tests passing.

Well OK, maybe we need some form of "bzr --no-nondefault-plugins
selftest" then. If something doesn't come by default with Bazaar then
it surely shouldn't be included by default in a test run of Bazaar
(whether it's officially called a "plugin" or not isn't so interesting
in this context). A user could have all sorts of plugins installed, in
different stages of development, including some highly "alpha" ones
(like I have). You can't hope that all the plugins ever written will
always be compatible with 17000 tests that take half an hour to run,
can you?

>> 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.
>
> I don't understand why failing plugin tests would prevent the newbie's
> tests from working.

"Their tests" as in "their runs of the tests". It would be nice if I
as a newbie could run the tests and know that failures were my fault,
rather than having to also run them on unchanged code and compare the
output. It took some time
for me to establish that the test failures were caused by my
(non-default) plugins, I just wondered if you were interested in
helping the next person in my shoes be aware of this issue more
directly.

>
>>>> 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...
>
> Okay, but I don't understand why you need to check for the existence of
> symbolic links during relpath in order to avoid add traversing a
> symlink.  For example, if you look at the comments in 124859, you'll see
> my "terminal path element" suggestion for dealing with this situation.
> I'd like to hear what you think of it.
>

I don't really understand it. I take it you refer to this:

Robert Collins: They [symlinks] are first class entities, but that
does not preclude following them at appropriate times.

You: Okay, let me say that "when the symlink is the terminal path
element" is not an appropriate time."

which to me is not self-explanatory, especially as Robert apparently
didn't pick up on it.

In any case, this seems to be part of a long discussion about what to
do when two different Bazaar working trees are linked via symlinks.
Somebody suggested that that bug is a duplicate of this one, but it
really isn't as far as I can see.
The situation I'm trying to fix is far simpler and (I would guess) far
more common and is nothing to do with how Bazaar version-controls
symbolic links. In this case the symbolic link is just part of the
full path to the working tree (usually because of disk volume mounting
in my case).

>> Isn't the fact that it fixes this issue enough for it to be "better"...?
>
> When you're talking about changing the behaviour of a low-level function
> in order to achieve a high-level effect, no it's not.

Well, this is quite a serious bug in my view, it's been open for
nearly a year and has been independently reported 4 times (5 counting
the first comment). I've personally run into it at least 3 times
independently, one of which forced me to implement an ugly workaround
for it in my test tool's integration with Bazaar.

To me, it seems that fixing it at a low level will probably mean it
stays fixed, whereas if you address it at higher levels you have to
address a lot of different contexts separately to ensure
osutils.relpath is never passed a symbolic link and its target as
arguments. Which is going to be a lot more complicated and possibly
lead to variants of the bug reappearing in the future.

Unless, of course, a measurable slowdown can be demonstrated (see later).

>> 3 samples doesn't sound very
>> much to me for something that only takes 2 seconds anyway.
>
> Best of 3 is our default practice for benchmarking.

Is there any documentation of why? It still doesn't sound very much to
me even if it is your default practice. It surely ought to depend on
the size of the operation? I've done a lot of benchmarking at work and
have come to the view that cpu time changes less than 2% should be
viewed as noise.

Anyway, I did "time repeat 100 bzr branches" and discovered that my
fix improved total time by about 1% (!) I became a bit suspicious and
added a print statement to osutils.relpath, which did not write
anything. It seems osutils.relpath isn't called at all by "bzr
branches".

Maybe we need another benchmark...

/Geoff