[MERGE] Fix for bug 183831

[Aaron Bentley]

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

Geoff Bache wrote:
>> Aaron Bentley wrote:
> "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.

You can usually tell by the name of the failed test.

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

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

Oh.  In your initial email, you referred to "183831 and the various
duplicates", so I thought you agreed.

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

That is not the case that this bug discusses.  This bug discusses the
unwanted traversal of symlinks.  When "bzr add"ing a symlink, you do not
want bzr to follow the symlink and add the referenced file.  You want to
add the symlink itself.

Since you weren't trying to solve this bug, it was hard to see how your
solution could make sense.  In fact, traversing symlinks
inappropriately is the cause of bug 183831.

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

I don't mean to suggest your bug is not important, or that it shouldn't
be fixed;  I am happy to help you fix it.

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

Fixing it at the callsite means we can be reasonably confident it will
have no unwanted side-effects.  Changing the behaviour of the called
function means that we cannot easily predict the side-effects.  It calls
for greater caution.

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

I don't recall.

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

We take the fastest result in order to reduce the impact of noise.

> It seems osutils.relpath isn't called at all by "bzr
> branches".

Ah, okay.  Oops.

Aaron

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

iEYEARECAAYFAknVLyoACgkQ0F+nu1YWqI1iggCfQ91yXK42hAodIknE9/cQp8xJ
B7AAniGRDFI1FfUfon+maTHZm2j1Mv9V
=gQZZ
-----END PGP SIGNATURE-----