[MERGE] Fix for bug 183831

[Aaron Bentley]

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

Geoff Bache wrote:
>>> 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.
> 
> I think we've hit the crux now.

I raised that back on April 4:

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

I also stated, two mails back:

> But even if we were to pursue [a solution that always traversed the symlink], 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.

The case I presented was presented as "an example of the problem with
your approach".  I wasn't saying I wanted you to fix that case in
particular.

> Robert said something interesting earlier on this thread: "I'm pro
> this change because it fixes the bug and doesn't break other tests".
> Isn't that what Test-driven Development is about? We don't try and get
> every pathological case right at once: we proceed in small steps and
> fix one issue at a time.

TDD isn't always a complete solution.  TDD works when you're reasonably
sure what you need to test.  When you're changing the kind of operation
a function does, it's quite easy to expose gaps in coverage.

Say I patched osutils.pathjoin to replace all instances of 'ý' with 'y'.
 I wouldn't be surprised if all our tests passed.  Sure, we test that
our code works with unicode, but we don't do exhaustive tests with every
character.

> There are 17000 tests: if they all work then surely that's a pretty
> good indication we can proceed.

I don't think it is, because I don't think the behavior of symlinks with
relpath is well tested.  Because symlinks are not supported on all
platforms, we rarely use them when we're not specifically testing
symlinks.  And until now, there's been no reason to test relpath
behaviour with symlinks.

> How do we know when we're done
> otherwise? We could sit in our rooms and dream up undesirable side
> effects until next year, but we still won't think of everything that
> will happen, and we'll probably think of plenty of things that won't.

Whereas, if we make a change whose scope is limited to the problem we
are trying to solve, it is much easier to predict the effects.

> I don't disagree that I may be doing this at the "wrong layer" from a
> theoretical correctness point of view : I don't know the code
> structure well enough to judge, really.

I have tried to point out where I think this should be fixed:
tree_files_for_add.

> In my world, the interesting
> question is not "Is this correct in every conceivable respect?"

That's a strawman.  I've already said I'd support a solution that isn't
correct in every conceivable respect.

> but "Is it better than what we currently have?" From a behaviour point of
> view at least the answer would seem to me to be yes, in that it fixes
> an issue with normal sensible usage and may lead to complications in
> situations where it isn't particularly hard to trigger bugs with
> today's bzr anyway (i.e. complex symbolic link usage).

- From my perspective, it's overkill.

> From my point of view, the demands you're placing on me would be
> (more) reasonable if I were employed by Canonical or had a large
> amount of time to intimately get to know the inner workings of Bazaar
> with the ultimate aim of becoming a core committer or something.
> That's not the case here.

I have tried to point you in the right direction.  I think that the size
of the change would be comparable to the size of the change you've proposed.

> Someone in my position needs to be able to rely on the tests to prove
> whether they've gone wrong or not, and they will probably do things in
> other ways, maybe other "abstraction layers", than what you as an
> experienced person will do. The question is whether you then accept
> that and make a note to refactor when you have time, or stonewall them
> out.

I haven't even voted on your change.  I haven't done anything to prevent
this from being merged, other than voicing my concerns.  If you can find
another reviewer who thinks this is a good idea, then it will be merged.
 Our reviewers are listed here:
http://bundlebuggy.aaronbentley.com/project/bzr

I really should vote "resubmit" on it just on a technical basis, because
it doesn't have unit tests, and because you should be using
self.requireFeature(SymlinkFeature).

> In the latter case, you obviously reinforce the impression that
> this is a Canonical project, not a FOSS project.

The impression is wrong.  Most of my involvement with this project was
before I worked at Canonical, and my feelings were the same then.

>> If you had tried to fix just add, I think your change would have landed
>> already.
> 
> Well, wait a minute. I wrote a test for the specific usage of add in
> the bug I wished to fix, made it work, and ran 17000 tests.

Perhaps a better phrasing would have been, "If your fix had only changed
add, I think your change would have landed already."

Your change affected far more than add, and I thought this was
deliberate because, in your initial mail, you said, "The main point is
to fix the myriad problems that can occur when Bazaar is given a
repository path containing symbolic links".

> Your latest comment now indicates you won't
> support my change even when you can't think of any more pathological
> cases, because there still might be some unforeseen ones somewhere...

I don't think this is the right kind of change, and I've been saying so
since April 4.  I can't offer positive support when I think it's not the
right kind of change, but neither am I blocking others from merging it.

> and then you tell me it's me that wants to fix everything at once!

Yes.  Because you changing osutils.relpath instead of changing the add
operation, I thought you were trying to fix all other operations that
can have problems when Bazaar is given a working tree path containing
symbolic links.

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

iEYEARECAAYFAknnnfMACgkQ0F+nu1YWqI0fcwCggfW6QWPK/s2OtnSVMToYRaMZ
upMAniWs48ri1smJeAeBr+wlAz+9ZThM
=TGEo
-----END PGP SIGNATURE-----