Patchwork [RESEND] keyword: use wvfs.rmtree to remove kwdemo directory

login
register
mail settings
Submitter Christian Ebert
Date May 1, 2015, 11:21 p.m.
Message ID <7d7445e0ea622532c20a.1430522460@1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.ip6.arpa>
Download mbox | patch
Permalink /patch/8838/
State Superseded
Commit 4ecbd88cde9a0a5019ebf77d1c1d4398398f4583
Headers show

Comments

Christian Ebert - May 1, 2015, 11:21 p.m.
# HG changeset patch
# User Christian Ebert <blacktrash@gmx.net>
# Date 1429364366 -7200
#      Sat Apr 18 15:39:26 2015 +0200
# Node ID 7d7445e0ea622532c20a8a8fa5a00e582fe96c38
# Parent  2ee10789d66b9ca500a7bfb1b3e031e04cdc4fb7
keyword: use wvfs.rmtree to remove kwdemo directory
Pierre-Yves David - May 1, 2015, 11:56 p.m.
On 05/01/2015 04:21 PM, Christian Ebert wrote:
> # HG changeset patch
> # User Christian Ebert <blacktrash@gmx.net>
> # Date 1429364366 -7200
> #      Sat Apr 18 15:39:26 2015 +0200
> # Node ID 7d7445e0ea622532c20a8a8fa5a00e582fe96c38
> # Parent  2ee10789d66b9ca500a7bfb1b3e031e04cdc4fb7
> keyword: use wvfs.rmtree to remove kwdemo directory
>
> diff --git a/hgext/keyword.py b/hgext/keyword.py
> --- a/hgext/keyword.py
> +++ b/hgext/keyword.py
> @@ -457,9 +457,7 @@ def demo(ui, repo, *args, **opts):
>       repo.commit(text=msg)
>       ui.status(_('\n\tkeywords expanded\n'))
>       ui.write(repo.wread(fn))
> -    for root, dirs, files in os.walk(tmpdir):
> -        for f in files:
> -            util.unlinkpath(repo.vfs.reljoin(root, f))
> +    repo.wvfs.rmtree()

rmtree with no argument?
Christian Ebert - May 2, 2015, 7:34 a.m.
* Pierre-Yves David on Friday, May 01, 2015 at 16:56:28 -0700
> On 05/01/2015 04:21 PM, Christian Ebert wrote:
>> # HG changeset patch
>> # User Christian Ebert <blacktrash@gmx.net>
>> # Date 1429364366 -7200
>> #      Sat Apr 18 15:39:26 2015 +0200
>> # Node ID 7d7445e0ea622532c20a8a8fa5a00e582fe96c38
>> # Parent  2ee10789d66b9ca500a7bfb1b3e031e04cdc4fb7
>> keyword: use wvfs.rmtree to remove kwdemo directory
>> 
>> diff --git a/hgext/keyword.py b/hgext/keyword.py
>> --- a/hgext/keyword.py
>> +++ b/hgext/keyword.py
>> @@ -457,9 +457,7 @@ def demo(ui, repo, *args, **opts):
>>     repo.commit(text=msg)
>>     ui.status(_('\n\tkeywords expanded\n'))
>>     ui.write(repo.wread(fn))
>> -    for root, dirs, files in os.walk(tmpdir):
>> -        for f in files:
>> -            util.unlinkpath(repo.vfs.reljoin(root, f))
>> +    repo.wvfs.rmtree()
> 
> rmtree with no argument?

Yes, the entire self repo/tmpdir, like in subrepo.py.

At least that what happens, and it should.
Matt Mackall - May 2, 2015, 9:48 p.m.
On Sat, 2015-05-02 at 08:34 +0100, Christian Ebert wrote:
> * Pierre-Yves David on Friday, May 01, 2015 at 16:56:28 -0700
> > On 05/01/2015 04:21 PM, Christian Ebert wrote:
> >> # HG changeset patch
> >> # User Christian Ebert <blacktrash@gmx.net>
> >> # Date 1429364366 -7200
> >> #      Sat Apr 18 15:39:26 2015 +0200
> >> # Node ID 7d7445e0ea622532c20a8a8fa5a00e582fe96c38
> >> # Parent  2ee10789d66b9ca500a7bfb1b3e031e04cdc4fb7
> >> keyword: use wvfs.rmtree to remove kwdemo directory
> >> 
> >> diff --git a/hgext/keyword.py b/hgext/keyword.py
> >> --- a/hgext/keyword.py
> >> +++ b/hgext/keyword.py
> >> @@ -457,9 +457,7 @@ def demo(ui, repo, *args, **opts):
> >>     repo.commit(text=msg)
> >>     ui.status(_('\n\tkeywords expanded\n'))
> >>     ui.write(repo.wread(fn))
> >> -    for root, dirs, files in os.walk(tmpdir):
> >> -        for f in files:
> >> -            util.unlinkpath(repo.vfs.reljoin(root, f))
> >> +    repo.wvfs.rmtree()
> > 
> > rmtree with no argument?
> 
> Yes, the entire self repo/tmpdir, like in subrepo.py.
> 
> At least that what happens, and it should.

That's quite a scary API, now that you've drawn it to my attention.
Makes it a little too easy to accidentally delete repositories. We
probably shouldn't have a default arg there.. and we might even consider
not accepting empty strings.
Christian Ebert - May 3, 2015, 1:24 a.m.
* Matt Mackall on Saturday, May 02, 2015 at 16:48:31 -0500
> On Sat, 2015-05-02 at 08:34 +0100, Christian Ebert wrote:
>> * Pierre-Yves David on Friday, May 01, 2015 at 16:56:28 -0700
>>> On 05/01/2015 04:21 PM, Christian Ebert wrote:
>>>> # HG changeset patch
>>>> # User Christian Ebert <blacktrash@gmx.net>
>>>> # Date 1429364366 -7200
>>>> #      Sat Apr 18 15:39:26 2015 +0200
>>>> # Node ID 7d7445e0ea622532c20a8a8fa5a00e582fe96c38
>>>> # Parent  2ee10789d66b9ca500a7bfb1b3e031e04cdc4fb7
>>>> keyword: use wvfs.rmtree to remove kwdemo directory
>>>> 
>>>> diff --git a/hgext/keyword.py b/hgext/keyword.py
>>>> --- a/hgext/keyword.py
>>>> +++ b/hgext/keyword.py
>>>> @@ -457,9 +457,7 @@ def demo(ui, repo, *args, **opts):
>>>>    repo.commit(text=msg)
>>>>    ui.status(_('\n\tkeywords expanded\n'))
>>>>    ui.write(repo.wread(fn))
>>>> -    for root, dirs, files in os.walk(tmpdir):
>>>> -        for f in files:
>>>> -            util.unlinkpath(repo.vfs.reljoin(root, f))
>>>> +    repo.wvfs.rmtree()
>>> 
>>> rmtree with no argument?
>> 
>> Yes, the entire self repo/tmpdir, like in subrepo.py.
>> 
>> At least that what happens, and it should.
> 
> That's quite a scary API, now that you've drawn it to my attention.
> Makes it a little too easy to accidentally delete repositories. We
> probably shouldn't have a default arg there.. and we might even consider
> not accepting empty strings.

I can pass the temp dir as argument explicitly. That works as
well in this case. You want me to resend again like that?
Christian Ebert - May 3, 2015, 2:08 a.m.
* Christian Ebert on Sunday, May 03, 2015 at 02:24:59 +0100
> * Matt Mackall on Saturday, May 02, 2015 at 16:48:31 -0500
>> On Sat, 2015-05-02 at 08:34 +0100, Christian Ebert wrote:
>>> * Pierre-Yves David on Friday, May 01, 2015 at 16:56:28 -0700
>>>> On 05/01/2015 04:21 PM, Christian Ebert wrote:
>>>>> +    repo.wvfs.rmtree()
>>>> 
>>>> rmtree with no argument?
>>> 
>>> Yes, the entire self repo/tmpdir, like in subrepo.py.
>>> 
>>> At least that what happens, and it should.
>> 
>> That's quite a scary API, now that you've drawn it to my attention.
>> Makes it a little too easy to accidentally delete repositories. We
>> probably shouldn't have a default arg there.. and we might even consider
>> not accepting empty strings.
> 
> I can pass the temp dir as argument explicitly. That works as
> well in this case. You want me to resend again like that?

I suggest

repo.wvfs.rmtree(repo.root)

which should make it obvious that this is intentional.
Augie Fackler - May 5, 2015, 2:11 p.m.
On Sun, May 03, 2015 at 02:24:59AM +0100, Christian Ebert wrote:
> * Matt Mackall on Saturday, May 02, 2015 at 16:48:31 -0500
> > On Sat, 2015-05-02 at 08:34 +0100, Christian Ebert wrote:
> >> * Pierre-Yves David on Friday, May 01, 2015 at 16:56:28 -0700
> >>> On 05/01/2015 04:21 PM, Christian Ebert wrote:
> >>>> # HG changeset patch
> >>>> # User Christian Ebert <blacktrash@gmx.net>
> >>>> # Date 1429364366 -7200
> >>>> #      Sat Apr 18 15:39:26 2015 +0200
> >>>> # Node ID 7d7445e0ea622532c20a8a8fa5a00e582fe96c38
> >>>> # Parent  2ee10789d66b9ca500a7bfb1b3e031e04cdc4fb7
> >>>> keyword: use wvfs.rmtree to remove kwdemo directory
> >>>>
> >>>> diff --git a/hgext/keyword.py b/hgext/keyword.py
> >>>> --- a/hgext/keyword.py
> >>>> +++ b/hgext/keyword.py
> >>>> @@ -457,9 +457,7 @@ def demo(ui, repo, *args, **opts):
> >>>>    repo.commit(text=msg)
> >>>>    ui.status(_('\n\tkeywords expanded\n'))
> >>>>    ui.write(repo.wread(fn))
> >>>> -    for root, dirs, files in os.walk(tmpdir):
> >>>> -        for f in files:
> >>>> -            util.unlinkpath(repo.vfs.reljoin(root, f))
> >>>> +    repo.wvfs.rmtree()
> >>>
> >>> rmtree with no argument?
> >>
> >> Yes, the entire self repo/tmpdir, like in subrepo.py.
> >>
> >> At least that what happens, and it should.
> >
> > That's quite a scary API, now that you've drawn it to my attention.
> > Makes it a little too easy to accidentally delete repositories. We
> > probably shouldn't have a default arg there.. and we might even consider
> > not accepting empty strings.
>
> I can pass the temp dir as argument explicitly. That works as
> well in this case. You want me to resend again like that?

Yes please

>
> --
> Auftreten Tarzan und Martha -
> ich hatte Sankt Pauli untersch├Ątzt.
>
> _MICHAEL WEBER: MARTHA_ --->> http://www.blacktrash.org/baustellen/#martha
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Christian Ebert - May 5, 2015, 6:23 p.m.
* Augie Fackler on Tuesday, May 05, 2015 at 10:11:59 -0400
> On Sun, May 03, 2015 at 02:24:59AM +0100, Christian Ebert wrote:
>> * Matt Mackall on Saturday, May 02, 2015 at 16:48:31 -0500
>>> That's quite a scary API, now that you've drawn it to my attention.
>>> Makes it a little too easy to accidentally delete repositories. We
>>> probably shouldn't have a default arg there.. and we might even consider
>>> not accepting empty strings.
>> 
>> I can pass the temp dir as argument explicitly. That works as
>> well in this case. You want me to resend again like that?
> 
> Yes please

It's already in, after I had sent a V2:
http://selenic.com/hg/rev/4ecbd88cde9a

thx

Patch

diff --git a/hgext/keyword.py b/hgext/keyword.py
--- a/hgext/keyword.py
+++ b/hgext/keyword.py
@@ -457,9 +457,7 @@  def demo(ui, repo, *args, **opts):
     repo.commit(text=msg)
     ui.status(_('\n\tkeywords expanded\n'))
     ui.write(repo.wread(fn))
-    for root, dirs, files in os.walk(tmpdir):
-        for f in files:
-            util.unlinkpath(repo.vfs.reljoin(root, f))
+    repo.wvfs.rmtree()
 
 @command('kwexpand',
     commands.walkopts,