[Merge][#111665] bzr rm should remove clean subtrees

[Marius Kruger]

Hi Alexander,
thanks for the review,
I tried to address you comments.

Martin mentioned that some people do not consider this a bug,
but I think since it currently does not work like advertised in all cases,
it is a bug (even if it is a bug in behavior you don't like :)

I must say even though I was indifferent to how rm should behave,
I fell in love with how it is working now (and not because I changed).

Alexander Belchenko   <bialix at ukr.net> wrote:
> > -    def getTree(self):
> > +    def getTree(self, files):
>
> > +    def getCommittedTree(self, files, message="Committing"):
>
> ^-- PEP-8 suggests to use names_with_underscores for functions and
> methods,
> and keep mixedCase only in sources where already too much such names
> for backward compatibility.
> I'm not sure what is our case: unittest methods heavily used mixedCase,
> but we use test_names_with_underscores. May be it's good to name
> helper functions in the same style: get_tree, get_committed_tree?
> I know that getTree was here before, it's just some thoughts about.
> This comment is only comment, it's not blocker to merge your patch.
done,
sorry I'm a java developer by day, and a python developer by night:)

> > +    def test_remove_unknown_ignored_files(self):
> > +        """unknown ignored files should be deleted."""
>
> ^-- s/unknown/Unknown/ please.
done

> > +        tree = self.getCommittedTree(['b/'])
> > +        ignores.add_runtime_ignores(["*ignored*"])
> > +
> > +        self.build_tree(['unknown_ignored_file'])
>
> > +        self.assertTrue(tree.is_ignored('unknown_ignored_file') !=
> None)
>
> ^-- self.assertNotEquals(None, tree.is_ignored('unknown_ignored_file'))
> is much better IMO.
done,
self.assertNotNone(tree.is_ignored('unknown_ignored_file'))
is much better IMO :) (people did't like it when I introduced it last time)


> > +        tree.remove('unknown_ignored_file', keep_files=False)
>
> > +        self.assertNotInWorkingTree('unknown_ignored_file')
> > +        self.failIfExists('unknown_ignored_file')
>
> ^-- I'm not sure, but in these tests such pair of checks is used very
> often.
> May be it's worth to refactor them out as handy method?
done,
I hope the camelCase method names are ok:
assertRemovedAndDeleted and assertRemovedAndNotDeleted

> >          #see if we can force it now..
>
> ^-- may I ask you to insert one space between # and comment text?
> For readability.
done

> > +        #move stuff from a=>b
>
> ^-- the same here, please.
done
> > \ No newline at end of file
>
> ^-- :-) newline at the end of file, please.
done
(I thought bzr is going the no newlines at the end of files)

> > === modified file 'bzrlib/workingtree.py'
> > --- bzrlib/workingtree.py     2007-07-25 21:25:00 +0000
> > +++ bzrlib/workingtree.py     2007-08-11 11:29:05 +0000
>
> > @@ -1806,11 +1804,16 @@
> >                      directory):
> >                  for relpath, basename, kind, lstat, abspath in
> file_infos:
> >                      if kind == 'file':
> > -                        if self.path2id(relpath): #is it versioned?
> > +                        #is it versioned or ignored?
>
> ^-- space between # and comment text, please.
done

>
> > +                        if self.path2id(relpath) or
> self.is_ignored(relpath):
> > +                            # add subtree content for deletion
> >                              new_files.add(relpath)
> >                          else:
> >                              unknown_files_in_directory.add(
> >                                  (relpath, None, kind))
> > +                    else:
> > +                        # make sure we delete subtrees
> > +                        new_files.add(relpath)
>
> ^-- I don't understand this part. You're explicitly checking
> for versioned and ignored files, but skip such check for
> directories and symlinks. Why?
Good catch, nested files, directories and symlinks should be handled the
same,
I think I did it like that previously as I only considered changed files
precious.

> >          files = [f for f in new_files]
>
> ^-- does list comprehension is faster than slicing operation?
my problem was that new_files is a set,
I implemented Andrew's suggestion:
  files = list(new_files)


hope this bundle is better
regards
marius
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rm_should_remove_clean_trees4.patch
Type: text/x-diff
Size: 36527 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070813/bd222154/attachment-0001.bin