[MERGE] Fix for bug 183831

[Geoff Bache]

>> Now I'm even more confused. As far as I can see bug 183831 (the bug
>> *I'm* trying to fix) is caused by failing to traverse an external
>> symlink to correctly find the relative path to the repository.
>
> Not quite.  bug 183831 is caused by *inconsistently* traversing a
> symlink.  The bug is titled "bzr add <softlink> follows the link", so I
> assumed the solution was for "bzr add <softlink>" to stop following the
> link.
>
> 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.

A key point is that the symlink is entirely outside of what bzr is
controlling (as will be its target, usually). So in this case it is
correct to traverse it, as you have to do so to even reach the working
tree.

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

> I had misunderstood the nature of the complaint.  I that bar was being
> traversed when determining the path to add.  Instead, it was being
> traversed when the working tree was opened.  That misunderstanding is
> probably why my suggestions didn't make any sense to you.
>
> 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)?

In any case, neither of the above makes sense to me as a general rule.
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)

I've been primarily worrying about (A), which to me is clearly far
more common. I'd guess that for every person wanting to
version-control a symlink to their own parent directory (your example
below) there are a large number that happen to have their working tree
on a disk volume mounted via a symbolic link and want to add normal
files. Unfortunately, it's clearly harder than I thought to fix (A)
without affecting the behaviour in (B). Though I get the feeling from
your long discussion with Robert on this, all is not well with (B)
either in today's bzr...

>
> Here's an example of the problem with your approach:
> $ bzr init foo
> Created a standalone tree (format: pack-0.92)
>
> $ ln -s . foo/bar
> $ bzr add foo/bar --no-recurse
> $ bzr status foo
> ?   bar@
>
> Or, if we permit recursion,
> $ bzr init foo
> Created a standalone tree (format: pack-0.92)
>
> $ ln -s . foo/bar
> $ touch foo/baz
> $ ~/bzr/bzr.ab.integration/bzr add foo/bar
> adding bar
> adding baz
>
> This is especially bad if you do revert:
> $ bzr revert foo/bar
> - -   /
>
> - -   baz
> Conflict: can't delete  because it is not empty.  Not deleting.
>

OK. It looks like these two scenarios could usefully be added as
blackbox tests to prove this behaviour if you see them as important
use-cases. (Can't say I can see the point of adding symlinks to one's
own parent directory, but maybe that's beside the point...)

> This basically causes situations where you can't add a symlink at all--
> either you add too much or too little.  And I think that's worse than
> the bug you are trying to fix.
> However, I'd support traversing the symlink in tree_files_for_add.  And
> I'd be really happy with changing WorkingTree so that it doesn't
> traverse the symlink in the first place.

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?

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.

You seem to be imagining a grand fix which will make all symbolic link
behaviour correct in one go. If you have time and motivation to create
such a fix fairly soon, I say go ahead.

If not, one option would surely be to accept a more limited fix for
the time being, 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.

Regards,
Geoff