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

Robert Collins wrote:
[...]
> > 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.

It wasn't obvious to me that I needed to look up the call stack (there were
another 45 lines in _populate_from_inventory, so I tried looking at them first).
Also “considered” is a very vague word that didn't give me any hints about what
else needed to be done with entry, and “copy-from-basis” looks like a phrase I
should be able to find elsewhere in the file, but it's unique to this comment.
In fact “step” doesn't appear anywhere else in this file either.

So trying to follow the clues in this comment to understand what it was about
lead me down several dead ends before I just gave up and made a review comment.
The primary improvement I can think of is more consistent terminology

So, how about:

            if exclude and is_inside_any(exclude, path):
                # Skip: _update_builder_with_changes will record an unmodified
                # version of excluded paths.

[...]
> > > +        out, err = self.run_bzr(['added'])
> > > +        self.assertEqual('b\nc\n', out)
[...]
> > 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.

But it's easier to apply sorted() to an iterator than it is to data that has
been serialised and formatted for stdout.

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

That test is explicitly about excluding subtrees, not about -x taking precedence
over a specifically included path.  Implementation-wise the tests would likely
be very similar, but I think ideally unit tests ought to have a single clear
purpose.  There's a risk (probably quite small in this case, so I'm not too
worried) that a future refactoring of the test code would fail to notice the
dual-purpose of this test and so inadvertently reduce test coverage.

-Andrew.