[MERGE] Review feedback : test for PointlessCommit and that the example given in the help (excluding a subtree of a specified tree) does in fact work.

[Andrew Bennetts]

I like this change.  I found a test bug, and I have a couple of other comments,
so,

bb:tweak

Robert Collins wrote:
[...]
> === modified file 'bzrlib/builtins.py'
> --- bzrlib/builtins.py	2008-07-29 08:40:05 +0000
> +++ bzrlib/builtins.py	2008-08-04 07:29:51 +0000
> @@ -87,13 +87,27 @@
>      if file_list is None or len(file_list) == 0:
>          return WorkingTree.open_containing(default_branch)[0], file_list
>      tree = WorkingTree.open_containing(osutils.realpath(file_list[0]))[0]
> +    return tree, safe_relpath_files(tree, file_list)
> +
> +
> +def safe_relpath_files(tree, file_list):
> +    """Convert file_list into a list of relpaths in tree.
> +
> +    :param tree: A tree to operate on.
> +    :param file_list: A list of user provided paths or None.
> +    :return: A list of relative paths.
> +    :raises errors.PathNotChild: When a provided path is in a different tree
> +        than tree.
> +    """
> +    if file_list is None:
> +        return None

I'm not sure why file_list can be None.  Won't it be passed an empty list if no
-x options are given?

> === modified file 'bzrlib/commit.py'
[...]
> -        specific_files = self.specific_files
> +        exclude = self.exclude
> +        specific_files = self.specific_files or []

It might be nice if the handling of None for specific_files and exclude were
done the same way.  Not a big deal though.

[...]
> +        # NB: entries will include entries within the excluded ids/paths
> +        # because iter_entries_by_dir has no 'exclude' facility today.
>          entries = work_inv.iter_entries_by_dir(
>              specific_file_ids=self.specific_file_ids, yield_parents=True)
>          for path, existing_ie in entries:
> @@ -739,6 +757,10 @@
>                  if deleted_dict is not None:
>                      # the path has a deleted parent, do not add it.
>                      continue
> +            if exclude and is_inside_any(exclude, path):
> +                # Skip - it is to be considered by the final copy-from-basis
> +                # step.
> +                continue

I'm not sure what “it is to be considered by the final copy-from-basis step”
means, but it sounds like it is contradicting the part of the comment that says
it will “Skip” it.  I certainly don't see any other code that operates on the
“entries” variable, and searching in the file for “copy” or “basis” didn't
enlighten me.  Perhaps there's something about commit that I'm missing?

In short, clarify this comment please.

> === modified file 'bzrlib/tests/blackbox/test_commit.py'
[...]
> +    def test_commit_exclude_excludes_modified_files(self):
> +        """Commit -x foo should ignore changes to foo."""
> +        tree = self.make_branch_and_tree('.')
> +        self.build_tree(['a', 'b', 'c'])
> +        tree.smart_add(['.'])
> +        out, err = self.run_bzr(['commit', '-m', 'test', '-x', 'b'])
> +        self.assertFalse('added b' in out)
> +        self.assertFalse('added b' in err)
> +        # If b was ignored it will still be 'added' in status.

I think you mean s/ignored/excluded/ here.

> +        out, err = self.run_bzr(['added'])
> +        self.assertEqual('b\n', out)
> +        self.assertEqual('', err)

I thought the idea with blackbox tests was to do minimum amount of work through
run_bzr; i.e. just use run_bzr for the command we are testing.  So I'd expect
the assertion that 'b' is still in the 'added' status to be expressed more
directly.  It's not a big deal here (although it is a bit slower), and I guess
it's a little more convenient.

> +
> +    def test_commit_exclude_twice_uses_both_rules(self):
[...]
> +        # If b was ignored it will still be 'added' in status.
> +        out, err = self.run_bzr(['added'])
> +        self.assertEqual('b\nc\n', out)

s/ignored/excluded/ again, and also the comment should mention c as well as b.

Is it safe to assume the output of 'bzr added' is in alphabetical order?  I
don't see any code that makes that true.

Ah, and in fact that test does fail for me (but only if I run selftest with -v!):

Traceback (most recent call last):
  File "/home/andrew/warthogs/bzr/review-tmp/bzrlib/tests/blackbox/test_commit.py", line 359, in test_commit_exclude_twice_uses_both_rules
    self.assertEqual('b\nc\n', out)
AssertionError: not equal:
a = 'b\nc\n'
b = 'c\nb\n'

This is another reason to be asserting the effect of the command being test via
direct object calls rather than via command output...

> === modified file 'bzrlib/tests/workingtree_implementations/test_commit.py'
[...]
> +    def test_commit_exclude_exclude_changed_is_pointless(self):

“exclude_exclude”?

Hmm, the new help text says:

> +    When excludes are given, they take precedence over selected files.

But I don't see an explicit test for this case.  I guess it is implicitly
covered.

-Andrew.