[MERGE][FIXES: 38458] diff always uses working tree for file ids

[Martin Pool]

On  5 Jul 2006, Aaron Bentley <aaron.bentley at utoronto.ca> wrote:
> This patch allows diff to use three trees for converting paths to file
> ids: the FROM tree, the TO tree, and the working tree.

> 
> What I've done:
> 1. split transform.find_interesting into tree.specified_file_ids
> 2. add ability to specify an arbitrary number of trees to specified_file_ids
> 3. make specified_file_ids *optionally* raise PathsdNotVersionedError,
> instead of unconditionally raising NotVersionedError
> 4. make compare_trees use specified_file_ids, instead of is_inside_any and
> 5. add extra_trees parameter to compare_trees and show_diff trees, teach
> diff_cmd_helper to use it
> 6. add require_versioned to compare_trees
> 
> If this seems like a bit much for the the bug it fixes, it's because I
> think specified_file_ids has the potential to become our standard method
> of converting paths to file_ids across one or many trees.  We have
> several different ways of doing this, and I'd like to unify them.

Yes, me too.

>      specific_files
>          If true, only check for changes to specified names or
> -        files within them.  Any unversioned files given have no effect
> -        (but this might change in the future).
> +        files within them.  By default, unversioned files are ignored.
> +
> +    extra_trees
> +        If non-None, a list of more trees to use for looking up file_ids from
> +        paths
> +
> +    require_versioned
> +        If true, an all files are required to be versioned, and
> +        PathsNotVersionedError will be thrown if they are not.
>      """

This could do with a little more explanation of how file ids are looked
up, because it's something people will want to know to use it properly:

  Files are mapped to their ids by looking first in the new tree, then
  in the old tree, then in each of the extra trees (if any).

> @@ -186,8 +201,6 @@
>  
>      # TODO: Rather than iterating over the whole tree and then filtering, we
>      # could diff just the specified files (if any) and their subtrees.  
> -    # Perhaps should take a list of file-ids instead?   Need to indicate any
> -    # ids or names which were not found in the trees.
>  

Nice to see this done.

> === modified file 'bzrlib/tests/test_diff.py'
> --- bzrlib/tests/test_diff.py	2006-06-20 05:32:16 +0000
> +++ bzrlib/tests/test_diff.py	2006-07-05 04:19:47 +0000
> @@ -275,6 +275,16 @@
>  
>  ''')
>  
> +    def test_show_diff_specified(self):
> +        """A working-tree id can be used to identify a file"""
> +        self.wt.rename_one('file1', 'file1b')
> +        old_tree = self.b.repository.revision_tree('rev-1')
> +        new_tree = self.b.repository.revision_tree('rev-4')
> +        out_file = StringIO()
> +        show_diff_trees(old_tree, new_tree, to_file=out_file, 
> +                        specific_files=['file1b'], extra_trees=[self.wt])
> +        self.assertContainsRe(out_file.getvalue(), 'file1\t')
> +
>  
>  class TestPatienceDiffLib(TestCase):

If there is not already one, could you please add a test that you can
diff a directory to get changes to files under that directory, or at
least add a todo about this.

> +def specified_file_ids(filenames, trees, require_versioned=True):
> +    """Find the ids corresponding to specified filenames.
> +    
> +    :param filenames: The filenames to find file_ids for
> +    :param trees: The trees to find file_ids within
> +    :param require_versioned: if true, all specified filenames must occur in
> +    at least one tree.
> +    :return: a set of file ids for the specified filenames
> +    """

This is a good function to add but the name doesn't feel quite right -
if nothing else it doesn't start with a verb.  Maybe
lookup_names_across_trees?

Could this be split into two functions - one which looks up the names
themselves, and another which expands all of their children?  That would
make it smaller and perhaps allow for interesting combinations later on.
Just an idea.

Also this needs something in NEWS to say it was fixed.

Thanks and +1

-- 
Martin