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

[Robert Collins]

On Tue, 2008-08-05 at 09:50 +1000, Andrew Bennetts wrote:
> 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?

It will be passed None because thats the default value of the keyword
parameter to cmd_commit.run()

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

They can't be because one is passed into iter_entries where None has a
different meaning to [].

> [...]
> > +        # 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?

If you look up a call in the call stack, we copy from the basis
everything that was not modified. The inner loop here is considering
only modified paths; and we want to skip considering an excluded path as
a modified path.

> In short, clarify this comment please.

Suggestions welcome; it may be my brain but it seems entirely clear and
the extra prose I put above totally redundant given how commit as a
process operates.

> > === modified file 'bzrlib/tests/blackbox/test_commit.py'
> [...]

> > +        # If b was ignored it will still be 'added' in status.
> 
> I think you mean s/ignored/excluded/ here.

Uhm, for precision yes; I'll change that.

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

We don't really have a 'X is modified' trivial call on tree at the
moment; status was the easiest way to do it.

> > +
> > +    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.
..
> This is another reason to be asserting the effect of the command being test via
> direct object calls rather than via command output...

Well, not really - iterators are subject to ordering issues too. I'll
fix.

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

It's explicitly covered by subtree_of_selected.

Thanks for the review,
Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080805/022f3b61/attachment-0001.pgp