[MERGE] Fix for bug 183831

[Aaron Bentley]

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

Geoff Bache wrote:
> Well, yes, TDD isn't a complete solution in that you also need to
> engage your brain. But I disagree that you need a long period of
> worrying about side-effects after the tests go through.

I didn't mean to suggest that's true in the general case.  It might be
true in cases where you're changing the kind of operation a function does.

> To me, TDD
> means that if we're afraid we've broken something, we write more tests
> until we aren't afraid any more. We don't engage in speculation about
> what might possibly be broken.

Another thing testing can't ensure is good design.  That's what review
is for.

>> 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.
> 
> Sure, if you can sensibly do that, if the system is simple enough and
> if you know it well enough that analysing the scope and effects is
> straightforward.  That's 3 pretty big ifs though.

You can also ask for help.  You can also submit it for review and get
suggestions that way.  No one knows this codebase from stem to stern.

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

> Fundamentally, I am not aware of any reason why "add" or "log" should
> handle symbolic links in a different way to any other command.

They probably should not.  I'd be happy with a solution that addressed
all commands.  The reason I've opposed fixing this in osutils is because
that may affect other operations that just commands.  bzrlib, being a
library, should not be sensitive to the cwd.  It's easy to imagine using
relpath for remote paths or URL fragments, for example.

You have suggested that you are only trying to fix add, so
tree_files_for_add is the logical place to start.

> It
> seems the "general" code (in internal_tree_files) always follows
> symbolic links currently, irrespective of whether it's the "terminal
> path element" in your terminology or a working tree is present

In every case, it calls WorkingTree.open_containing, which will raise an
exception if there is no WorkingTree present.  So it does not return at
all if there is no working tree present.

> which
> is wrong in a lot of cases. What "log" does I can't really fathom,
> but it seems to work best of the three implementations from
> experimenting (I haven't succeeded in breaking it, whereas breaking
> the other two is easy). Maybe finding its symlink code would be a good
> idea, but I've failed to do that unfortunately. Can you help?

Sure.  The main code is in _get_info_for_log_files.  But the reason why
it is bullet-proof (at least for the first argument) is that it uses the
path returned by bzrdir.BzrDir.open_containing_tree_or_branch().
internal_tree_files and tree_files_for_add, are both ignoring the path
returned by WorkingTree.open_containing, supplying their own, and
getting it wrong.

> It seems wrong to me to just exacerbate this situation by inventing
> some more complicated symlink rules of my own that apply only to
> "add".

Believe me, I'd be much happier if tree_files_for_add didn't exist.  I
don't know why it does, but I suspect there is a better alternative to
all this copy-and-paste code.

I see a big difference in view handling:
- - it considers the view when files are explicitly supplied
- - it considers whether files are *contained* by view files , rather than
  whether they are, themselves, view files.
- - it raises an exception when files are outside the view instead of
  ignoring them.

Other differences:
- - no symlink traversal
- - does not return tree-relative paths

> To put it another way, just because code is called in cases other than
> the one we're trying to fix doesn't make it "overkill" to change it.

I agree completely.  But bzrlib is intended to be a general purpose
implementation of Bazaar, not tied to the commandline.

As such, it is used by
 - Bundle Buggy
 - Launchpad
 - Loggerhead
 - TortoiseBzr

and others.

Changing the overall behaviour of bzrlib in order to change the bzr
command UI is the part I object to.

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

iEYEARECAAYFAknstjwACgkQ0F+nu1YWqI291ACgh6mFODPRuNHQvhVAqM8Z+6Ln
VW0Ani9gw730wMfQ63bfz68uR6D+wdLW
=n4gE
-----END PGP SIGNATURE-----