[MERGE] Fix for bug 183831

[Aaron Bentley]

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

Geoff Bache wrote:
> OK, here comes a new patch, which changes only tree_files_for_add, and
> adds two blackbox tests around symbolic link usage for "bzr add".

Thanks for your patch.  To keep the process moving along, I've made some
updates myself, rather than asking you to do them.  It also gets it into
Bundle Buggy, which is how we track patches.

> So the current setup converts the correct relative path to an absolute
> path all ready for smart_add to convert it back again.

That certainly seems reasonable given these constraints.

> === modified file 'bzrlib/builtins.py'
> --- bzrlib/builtins.py	2009-04-20 08:37:32 +0000
> +++ bzrlib/builtins.py	2009-04-23 19:44:34 +0000
> @@ -78,16 +78,24 @@
>                                       (e.path, file_list[0]))
>  
>  
> +"""
> +Add handles files a bit differently so it a custom implementation.
> +In particular smart_add expects absolute paths, which it immediately converts
> +to relative paths. Would be nice to just return the relative paths like internal_tree_files
> +does but there are a large number of unit tests that assume the current interface to 
> +mutabletree.smart_add
> +"""
>  def tree_files_for_add(file_list):
> -    """Add handles files a bit differently so it a custom implementation."""

I moved the docstring back into the function, because Python needs the
docstring to be inside the function in order to associate it with the
function.

The end of your docstring seemed more like a TODO or fixme than API
documentation, so I converted it to a comment.

>      if file_list:
> -        tree = WorkingTree.open_containing(file_list[0])[0]
> +        tree, relpath = WorkingTree.open_containing(file_list[0])
> +        relfile_list = [relpath] + osutils.canonical_relpaths(tree.basedir, file_list[1:])

I reformatted the code to our maximum 79-character width, but then I
couldn't see any advantage to using canonical_relpaths and abspath on
file_list[1:].  Neither traverses symlinks, and the previous code used
the file_list without changes.  So I changed it to copy file_list and
replace only file_list[0]

>          if tree.supports_views():
>              view_files = tree.views.lookup_view()
>              if view_files:
>                  for filename in file_list:
>                      if not osutils.is_inside_any(view_files, filename):
>                          raise errors.FileOutsideView(filename, view_files)
> +        return tree, map(tree.abspath, relfile_list)

After changing tree_files_for_add to mutate file_list, there was no need
for separate return lines.

> +    def test_add_symlink_to_abspath(self):
> +        self.requireFeature(SymlinkFeature)
> +        self.make_branch_and_tree('.')
> +        os.symlink(os.path.expanduser("~"), 'link')

It's usually a bad idea to use paths outside the temp directory.  It
looks okay in this case, but I thought it would make sense to maintain
our normal pattern.

Can we get a second review?

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

iEYEARECAAYFAknw3w4ACgkQ0F+nu1YWqI0yWgCghz5LbBKTXCtKC1+1I9rNV1x6
BusAnjoSe9+PylIDKqnvNfUnPEYaJrWr
=hMJz
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: symlink-add.patch
Type: text/x-patch
Size: 8631 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090423/64daaf15/attachment.bin