'bzr rm' leaves directories behind?

[Robert Collins]

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.

('bzr', 'remove', 'file with spaces') is unambiguous and clear - and
these things are much more importance than 8 extra characters.

> > 
> > In fact run_bzr("string with spaces") should not work at all. 
> 
> It does work because for this test case run_bzr_captured is
> overwritten to 
> allow this. My intention was to get this functionality into the super
> class, 
> but when this code landed, that was considered too risky. 

Currently I'm -1 on letting that be in the super class, see my above for
discussion on why. I'm also -1 on extending our uses of that idiom, for
precisely the same reasons - if I had a little more time I'd do an audit
for the remaining sites of the really old style and nuke them all.

> Because of the method
> > 
> > 
> > +    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.

> > 
> > Other than that it looks ok to me. 
> > +1 conditional on the above things being corrected/discussed.
> 
> hope this is satisfactory, 
> and thanks again for the review, as I was starting to think you guys
> are not speaking to me for 
> introducing this bug in the first place. 

We're getting there. Sorry that you felt ignored - I don't think anyone
intended that!

-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/20070622/774af19d/attachment.pgp