'bzr rm' leaves directories behind?

[Martin Pool]

On 6/22/07, Robert Collins <robertc at robertcollins.net> wrote:
> On Thu, 2007-06-21 at 19:26 +0200, Marius Kruger wrote:
> > thank you Robert for the review.
> > see my comments below
> >
> > On 6/19/07, Robert Collins <robertc at robertcollins.net> wrote:
> > > Hi, I hadn't reviewed your previous patches. I've had a quick look
> > at
> > > this and have a few comments...
> > >
> > > self.run_bzr("commit", "-m", "empty")
> > > is preferred over
> > > self.run_bzr("commit -m empty")
> >
> > There were a lot of discussion about this for [MERGE] bzr rm should
> > delete the working file (Bug #82602)
> > and at least Martin sort of agreed with me that in principle:
> >         me: in general don't you think 'bzr rm xyz' is nicer than
> >         ['bzr','remove,'xyz']?
> >
> >         Martin: i do
> > But in any case this patch is not trying to change things either way,
> > It just keeps using
> > the same syntax which is already used throughout the file.
> > This being a bug fix, I'd rather not try to sort out what is preferred
> > usage of run_bzr now.
>
> It's fair enough that you don't want to be distracted from your bugfix
> for a general,problem, however that general issue was sorted out about 2
> years ago :).
> In particular : ("bzr rm file with spaces") cannot be correctly
> processed with a trivial string splitting helper inside the test suite.
> To avoid confusing readers - to optimise for folk reading the code - we
> should make it very clear when splitting is and is not happening. For
> these reasons we chose a couple of years back to make the recommended
> method be a list of strings, no magic involved.

I'm still in favour of taking only a list of strings.  I think at the
time I was willing to get the rm fix in and we would clean this up
later, since it was at least consistent within the tests -- probably
that was greedy.  We should clean up some of the several variations on
run_bzr in the test suite.

For this patch, maybe Marius can just change any affected lines back
to using lists?  Or is it not possible to reach that behaviour from
this class anymore?

> > > +    def _assertRemoveErrorContainsRe(self, e, file_detail_re):
> > >
> > > According to the comment this is a workaround for AssertContainsRe
> > not
> > > supporting DOT_ALL. Is there some reason you didn't just
> > fix/enhance
> > > AssertContainsRe?
> >
> > I am usually that sort of developer which fixes everything there and
> > then,
> > but reviewers complain to me that I 'fix' too many unrelated things in
> > one patch, which made reviewing difficult.
> > However I think this patch should rather go in than sooner rather than
> > later,
> > so I'll commit to enhance AssertContainsRe in a follow up patch if you
> > like.
>
> I think the complaints were being raised because you were presenting a
> large patch with a bunch of things all at once. To split them up - to
> have a single patch that fixes AssertContainsRe, and another that uses
> the fixed version - can make this easier to manage. That said, I'm happy
> to follow this patch through review, and I'd definately be happier with
> AssertContainsRe fixed (and the fix tested) than with an untested helper
> function which is itself larger than the fix to AssertContainsRe.

You can specify the DOTALL (. matches \n) with (?s) at the start of
the regexp.  Why not just do that and leave this method out?

-- 
Martin