[MERGE] bzr rm should delete the working file (Bug #82602)

[Marius Kruger]

hi all,
lets try again shall we


     ``bzr remove`` and ``bzr rm`` will now remove the working file, if
      it could be recovered again.
      This has been done for consistency with svn and the unix rm command.
      The old ``remove`` behaviour has been retained in the new option
      ``bzr remove --keep``, which will just stop versioning the file,
      but not delete it.
      ``bzr remove --force`` have been added which will always delete the
      files.
      ``bzr remove`` is also more verbose.





On 4/13/07, John Arbash Meinel <john at arbash-meinel.com> wrote:
>
> Ok. Again, we'd like to see this land by Monday. So ping me if you need
> some help. I may not be around a whole lot over the weekend, but other
> people will be.
>
Even though I worked late and even through the night, I probably missed the
bus :(
Hopefully we are a lot closer now though:



>     John Arbash Meinel wrote:
>         As the name might imply,
>         +++ bzrlib/tests/blackbox/test_too_much.py
>
>         Should not have tests added to it. If you find the need to update
> things
>         in there, it probably means that they need to be split out into
> one of
>         the other blackbox test files. If the test under focus is already
>         handled elsewhere, then the test_too_much.py test can just be
> removed.
>

I reverted and just did the minimum to get the test to pass again.
I tried to remove the 'remove' and 'rm' tests but a lot of the tests after
it
depend on it. We should probably see if we can remove the whole thing if
these
things are tested elsewhere.
[DONE]

    John Arbash Meinel wrote:
>         Lines like this:
>         +                        message="deleted %s"%f
>
>         should be:
>           message = "deleted %s" % f
>
>         And I prefer:
>           message = "deleted %s" % (f,)
>
>         Just in case 'f' isn't what you think it is. (If f is a tuple, the
>         former command will fail with a "Not all arguments handled" error,
>         rather than just formatting a string with a tuple in it).
>

[DONE]


    Aaron Bentley wrote:
>         If we're going to delete it, let's just delete it.  Being "smart"
> is
>         often stupid.  I can't imagine why I'd want "bzr rm" to delete the
> file,
>         but if I did, I'd want it to delete the file ALL THE TIME.  If
> you're
>         trying to save me from deleting modified files, then error out if
> you
>         must, but don't give me inconsistent behavior.
>

[DONE]


    John Arbash Meinel wrote:
>         Also I'm worried about:
>         +                if osutils.lexists(f):
>         +                    if force:
>         +                        # recursively delete f
>         +                        if osutils.isdir(f):
>         +                            osutils.rmtree(f)
>         +                        else:
>         +                            os.unlink(f)
>
>         I didn't see that you checked that files underneath the directory
> under
>         care are not modified.
>
>         So if I do "bzr rm dir" it should fail if "dir/subdir/foo" is
> currently
>         modified. We also should have explicit tests for this, to make
> sure it
>         stays correct.
>

[DONE]


    Aaron Bentley wrote:
>         Also, it seems like you have a tri-valued parameter here:  The
> deletion
>         policy may be "auto", "keep", or "force".  So it would be best to
> expose
>         this as a RegistryOption, not a pair of boolean options.  That
> way, "bzr
>         rm --force --keep" will select "keep" as the deletion policy, and
> an
>         alias of "rm=rm --keep" can be overriden with "bzr rm --auto".
>
>         Creating a RegistryOption doesn't have to be onerous; see the
>         from_kwargs factory.
>

[DONE]



>     John Arbash Meinel wrote:
>         Actually, I prefer "run_bzr" to run_bzr_captured....
>
>         run_bzr is just a wrapper around _captured, which allows you to
> supply:
>
>         run_bzr('params', 'to', 'bzr')
>
>         rather than
>
>         run_bzr_captured(['params', 'to', 'bzr'])
>
>         If only because it is less to type and keep track of.
>
>         > ok I'll convert to run_bzr_captured,
>         > but 'self.runbzr' seemed more suited to lazy people like me...
>         > [DONE]
>         >
>         > * these tests follow this pattern:
>         >        (out, err) = self.run_bzr_captured(['remove', '--keep',
> 'a'])
>         >        self.assertEquals(out.strip(), "")
>         >        self.assertEquals(err.strip(), "removed a")
>         > which we might want to do better:
>         >        self.run_bzr_captured(['remove', '--keep', 'a'],
> out_regex='',
>         > err_regex="removed a")
>         > also can't I make it like runbzr, to auto expand a sting?
>         >  (your probably gona say no as you don't like isinstance magic
> :(
>
>         We are more okay with testing magic, as long as it ends with easy
> to
>         understand tests.
>
>         We have "(out, err) = self.run_bzr_error([regex1, regex2],
> 'command',
>         'to', 'run')"
>
>         Does that fit what you want?
>

Yes but I also want it with run_bzr (so I moved it there).
And I want to be able to say self.run_bzr("commit -m 'howzit!'")
  so I added that too.
[DONE]

    John Arbash Meinel wrote:
>          I think this should have a NOTES WHEN UPGRADING: section in NEWS.
>         Because it means that 'bzr rm' will change how it works.
>
>         I know there have been mention of people who are already using
> 'bzr rm'
>          and expecting it to leave the file alone (James Troup mentioned
> this for
>         versioning files in /etc). So we need to make sure to convey this
> change
>         well. (We aren't destroying the data, you can 'bzr revert' or 'bzr
> rm
>         --keep', but people need to know about it).
>

> Should I just put the whole thing in a the NOTES WHEN UPGRADING section ?
...
You didn't respond so I just did that.

regards
marius


-- 



I code therefore I am.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.ubuntu.com/archives/bazaar/attachments/20070416/8a769c12/attachment-0001.htm 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rm_delete_working_file3.patch
Type: application/octet-stream
Size: 281580 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070416/8a769c12/attachment-0001.obj