[MERGE] Fix for bug 183831

[Aaron Bentley]

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

Geoff Bache wrote:
>> The example does not provide enough steps to reproduce the bug.
> 
> True, but I wrote a blackbox test illustrating the case I was trying
> to fix, and various of the duplicate bugs have better examples than
> the original one.

But when I was originally trying to understand your merge request, the
most accessible description was that bug, and I got a mistaken
impression from it.

>>  A
>> fuller example, as I now understand it, would be:
>> $ bzr init foo
>> Created a standalone tree (format: pack-0.92)
>>
>> $ ln -s foo bar
>> $ bzr add bar
>> bzr: ERROR: bzrlib.errors.PathNotChild: Path "/home/abentley/bar" is not
>> a child of path "/home/abentley/foo"
>>
> 
> Well, OK, though this is a bit pathological isn't it? I'd suggest
> rather a reference case where the correct behaviour would be to add a
> file, rather than to do nothing, such as in my blackbox test.
> 
> (i.e., add "touch foo/baz", and then try to do bzr add bar/baz, with
> the same result)

Or "touch foo/baz" and then "bzr add foo".

>> Still, there are two potential solutions to this problem:
>> 1. Do not traverse the symlink at all.
>> 2. Always traverse the symlink.
>>
>> I think that 1 is a better kind of solution, because it avoids throwing
>> information away.  But even if we were to pursue 1, I don't think that
>> changing the behaviour of osutils.relpath is the right approach, because
>> it will change the behavior in other places, and it's not at all clear
>> that this is correct.
> 
> One of the above references should presumably read (2)?

Yes.  "But even if we were to pursue 2..."

> In any case, neither of the above makes sense to me as a general rule.

I see your patch as an example of "always traverse the symlink".

> In your example above, symlinks should be traversed. In your example
> below, they should not. I'd suggest the following rules:
> 
> A) Where the given path is not itself a symlink, but contains symlink
> references (my blackbox test), bzr should always follow the symlinks
> and behave the same as if the real path had been provided.
> B) Where the given path is itself a symlink, it should only be
> traversed if the symlink is not under a working tree (or it should
> never be traversed, for simplicity)

See, I think this is where you're compromising a good idea.  Traversal
of symlinks *should* be informed by the presence or absence of working
trees, i.e. it should not be at the lower layer of osutils.

> Would you support a modification of osutils.relpath that replaces
> paths which are not themselves links with their "real path", and
> leaves actual links alone?

No, because it would still be a modification of osutils.relpath, and I
think that's the wrong layer.  However, this is otherwise similar to my
suggestion of avoid traversing symlinks that are terminal path elements.

Also, if we go back to my example, "bar" is a terminal path element, so
you wouldn't traverse it and you'd still get a PathNotChild error.  So I
don't think this is a great answer.  Getting PathNotChild in this
context is ludicrous.

> In other words, is the above case the real reason you're opposed to
> this change or are you still going to be opposed to it even if there
> is no concrete case to address? I'm happy to try and fix this but I
> don't want to spend the time if you're going to reject it anyway.

The real reason is that I think you're doing it at the wrong layer, and
that this will have undesirable side-effects.  So if you submitted a
patch that addressed all the concerns I've listed, I'd be inclined to
look for other undesirable side-effects.

> You seem to be imagining a grand fix which will make all symbolic link
> behaviour correct in one go.

No, I think it would be gradual.  If anything, I would characterize your
approach as "a grand fix which will make all symbolic link behaviour
correct in one go".  I don't think such an approach is realistic.

> If not, one option would surely be to accept a more limited fix for
> the time being

I don't see your fix as limited at all.  I see it as a sweeping change.

> and plan on a better fix that takes into account all
> sorts of weird situations with symbolic links inside repositories and
> the whole long discussion between you and Robert in the bug you first
> highlighted. That's surely better than another year of bzr spitting
> python stacks at beginner users who are just trying to add normal
> files to their repository.

If you had tried to fix just add, I think your change would have landed
already.

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

iEYEARECAAYFAknnP5oACgkQ0F+nu1YWqI0ovQCbBwlMPyj9no9OLcX5oCJi5Y2Z
Rc4An0EFBx69IFKrQdEBPjLk9Ks+IJN6
=C5pB
-----END PGP SIGNATURE-----