fix: make 'rm -rf foo/' be a bit harder #14749

Shane-XB-Qian · 2024-05-11T08:21:39Z

// if not back out #14742
// then make it be a bit harder to make mistake to rm all
//
// also seems recover its original logic of 'all' argument

// if not back out vim#14742 // then make it be a bit harder to make mistake to rm all // // also seems recover its original logic of 'all' argument Signed-off-by: shane.xb.qian <shane.qian@foxmail.com>

chrisbra · 2024-05-11T09:20:15Z

runtime/autoload/netrw.vim

@@ -11454,24 +11454,29 @@ fun! s:NetrwLocalRmFile(path,fname,all)

  else
   " attempt to remove directory
+   let rmfile= substitute(rmfile,'[\/]$','','e')


I would leave the directory indicator. That is actually useful.

it was there before this PR and #14742
i guess it was remained there for judging if it was a dir or not, i just kept it.

No, this was not touched by #14742

yes, so i didnot plan to touch it either.
// please notice that if/else

chrisbra · 2024-05-11T09:21:37Z

runtime/autoload/netrw.vim

   if !all
    echohl Statement
    call inputsave()
-    let ok= input("Confirm *recursive* deletion of directory<".rmfile."> ","[{y(es)},n(o),a(ll),q(uit)] ")
+    " make it be a bit harder to make mistake by inputting upper case
+    let ok= input("Confirm *recursive* deletion of directory<".rmfile."> ","[{Y(ES)},n(o),A(LL),q(uit)] ")


Now you are unexpectedly changing the key-bindings. This will break peoples habits. I don't think this is appreciated by long-time users. (besides, it actually looks funny, that some keys need to be upper-cased and others not).

well, i am not against just back out the #14742, then kept the appreciated long-time users' habits, or you had better idea not to so easy or high possibility to make mistake to rm all?

What mistake, there is still a confirmation dialog

lower case is easy to mis-type, like a 'hit-enter' happened.

it was designed to: if 'all=1' then no 'confirm', else req a confirm, and confirm to only delete('rm', 'd') not 'rf'.

chrisbra · 2024-05-11T10:03:16Z

Okay, let's step back a bit for now. What problem are you trying to solve?

I usually don't use netrw, but I just tried: D on a directory. There is always a confirmation dialog, Confirm *recursive* deletion of directory</tmp/c/> [{y(es)},n(o),a(ll),q(uit)], I need to confirm using y or a to delete. I think this is fine. Just pressing Enter, does not delete anything.

Conversely, when I go back to commit d3952e8 (so before #14742) was merged, it errored out for a non-empty directory, even if I explicitly confirmed to delete. Which I do not find particular helpful.

Shane-XB-Qian · 2024-05-11T10:19:06Z

there 2 rm operation in netrw actually, one is 'local', another is 'remote', there some cfg about 'rm' for 'remote', but 'local' was designed to only delete('rmfile', 'd') not 'rf',
it knew it maybe a bit inconvenience, but please Keep In Mind:
lots or most official vim users would use vim (vs xxxvim) in production, (a.k.a 'what vim is'), such 'rm all' operation is dangerous which like last time the performance bug #14650 , which would make 'real or classic vim users' afraid to use vim. (or use 'nano'? :sweet_smile:)

clason · 2024-05-11T10:21:18Z

Here you go again with the gatekeeping...

Shane-XB-Qian · 2024-05-11T10:34:11Z

Conversely, when I go back to commit d3952e8 (so before #14742) was merged, it errored out for a non-empty directory, even if I explicitly confirmed to delete. Which I do not find particular helpful.

i think it meant to make 'rm all' be harder, so if you'd like to keep #14742 , then wish you accept this enhance (and fix) too.

chrisbra · 2024-05-11T10:42:06Z

there 2 rm operation in netrw actually, one is 'local', another is 'remote', there some cfg about 'rm' for 'remote', but 'local' was designed to only delete('rmfile', 'd') not 'rf',

Correct. But #14742 only changed the NetrwLocalRmFile function, which as the name indicated, is only used for local browsing. I tested with using :e scp://localhost//tmp/ and then trying to delete anything. #14742 did not change anything here. In fact, I was not able to delete anything on the remote side (recursively). So this has nothing to do with local vs. remote.

Sorry, I think the new behaviour makes a bit more sense to me.

Shane-XB-Qian · 2024-05-11T10:43:39Z

Here you go again with the gatekeeping...

funny boy

Shane-XB-Qian · 2024-05-11T10:47:13Z

there 2 rm operation in netrw actually, one is 'local', another is 'remote', there some cfg about 'rm' for 'remote', but 'local' was designed to only delete('rmfile', 'd') not 'rf',

Correct. But #14742 only changed the NetrwLocalRmFile function, which as the name indicated, is only used for local browsing. I tested with using :e scp://localhost//tmp/ and then trying to delete anything. #14742 did not change anything here. In fact, I was not able to delete anything on the remote side (recursively). So this has nothing to do with local vs. remote.

default for 'remote' is 'rmdir' which would not rm non-empty dir.

#14742 did not change anything here.

it changed:

lower case is easy to mis-type, like a 'hit-enter' happened.
it was designed to: if 'all=1' then no 'confirm', else req a confirm, and confirm to only delete('rmfile', 'd') not 'rf'.

chrisbra · 2024-05-11T11:17:41Z

lower case is easy to mis-type, like a 'hit-enter' happened.

This was not changed. It was like that before already.

it was designed to: if 'all=1' then no 'confirm', else req a confirm, and confirm to only delete('rmfile', 'd') not 'rf'.

This seems to happen when either:

moving from local directory to remote directory. (in which case it makes sense I would say)
When deleteing several marked files/directories in which asking for confirmation several times is probably redundant and the changed behaviour also makes sense I would think.

Shane-XB-Qian · 2024-05-11T11:33:11Z

lower case is easy to mis-type, like a 'hit-enter' happened.

This was not changed. It was like that before already.

i meant after #14742 when you mis-typed 'D' then lower case is easier to make mistake which compared to upper case you had to press 'shift' first, a.k.a 'SAFE'.

it was designed to: if 'all=1' then no 'confirm', else req a confirm, and confirm to only delete('rmfile', 'd') not 'rf'.

This seems to happen when either:

1. moving from [local directory to remote directory](https://github.com/vim/vim/blob/58448e09be497a8abb595ae309b6edfbc8e0e05a/runtime/autoload/netrw.vim#L7855-L7863). (in which case it makes sense I would say)

2. When [deleteing several marked files/directories](https://github.com/vim/vim/blob/58448e09be497a8abb595ae309b6edfbc8e0e05a/runtime/autoload/netrw.vim#L11362-L11395) in which asking for confirmation several times is probably redundant and the changed behaviour also makes sense I would think.

yes, seems those 2 places for now, but please note:

" s:NetrwLocalRmFile: remove file fname given the path {{{2
"                     Give confirmation prompt unless all==1
fun! s:NetrwLocalRmFile(path,fname,all)

when all == 1, it was designed to 'no confirm', in the future some places may call this fun too, supposed it is Not ok to rm all directly.

Shane-XB-Qian · 2024-05-11T11:38:44Z

runtime/autoload/netrw.vim

-   endif
-   let rmfile= substitute(rmfile,'[\/]$','','e')
-
-   if all || ok =~# 'y\%[es]' || ok == ""


also please note: i removed this ok == "" though not sure when it would happen (or mostly it would not happen), but it scared me if i just pressed 'enter' somehow.

chrisbra · 2024-05-11T11:40:23Z

i meant after #14742 when you mis-typed 'D' then lower case is easier to make mistake which compared to upper case you had to press 'shift' first, a.k.a 'SAFE'.

You already have to type capital D, which makes deleting already a conscious action (I would have expected d, like in ncdu, but this doesn't delete). So that is not an issue then.

in the future some places may call this fun too, supposed it is Not ok to rm all directly.

May be, or may be not. We don't have a netrw maintainer and I don't expect this to change soon. I think this needs to be taken care of, when new functions start calling it. But that is not something to worry about currently.

Okay, let's close it then. I think we have verified that the current new version is fine and there is no issue. Sorry, I am not convinced that this change is a good one.

Shane-XB-Qian · 2024-05-11T12:13:08Z

i meant after #14742 when you mis-typed 'D' then lower case is easier to make mistake which compared to upper case you had to press 'shift' first, a.k.a 'SAFE'.

You already have to type capital D, which makes deleting already a conscious action (I would have expected d, like in ncdu, but this doesn't delete). So that is not an issue then.

ncdu is a gui-like diaglog for deletion, is it same?
which compared to vim is a cli input confirmation in the bottom, same like inputting any others keys, which somehow mis-typed in case.

in the future some places may call this fun too, supposed it is Not ok to rm all directly.

May be, or may be not. We don't have a netrw maintainer and I don't expect this to change soon.

um, i supposed we should not wait a knew problem to happen, there perhaps no regret to recover files once rm all.
if you were not sure, i supposed you still can try to ask question/advice from @cecamp
or if you were not sure, then why merged #14742 so easy, ahahahah...

I think this needs to be taken care of, when new functions start calling it. But that is not something to worry about currently.

Okay, let's close it then. I think we have verified that the current new version is fine and there is no issue. Sorry, I am not convinced that this change is a good one.

anyway, if you were not like this PR, i can maintain this local hack to myself, no problem.

chrisbra · 2024-05-11T12:34:29Z

ncdu is a gui-like diaglog for deletion, is it same?

It's a tui application. Like Vim is a tui application.

we should not wait a knew problem to happen,

Let me repeat: There is currently not a known problem.

Shane-XB-Qian · 2024-05-11T12:44:00Z

ncdu is a gui-like diaglog for deletion, is it same?

It's a tui application. Like Vim is a tui application.

it is gui-like diaglog, not a cli inputing yes/no/all/quit in vim, so to ncdu, users knew what he is doing, and mostly impossible to feedkey.

we should not wait a knew problem to happen,

Let me repeat: There is currently not a known problem.

i think i had repeated many times:

inputting by lower case to rm -rf is high possibility making mistake.
" Give confirmation prompt unless all==1
fun! s:NetrwLocalRmFile(path,fname,all)
when all == 1, it was designed to 'no confirm', in the future some places may call this fun too, supposed it is Not ok to rm all directly.

benknoble · 2024-05-11T12:46:24Z

There is some research that making deletes harder is not always the right idea. I appreciate that the confirmation makes it harder for my cat to accidentally delete files, but extra roadblocks (given the requirement to shift-d already) are probably not necessary.

Clarifying the interface of a public function (even one not intended for public use) may be worthwhile.

chrisbra · 2024-05-11T14:00:19Z

it is gui-like diaglog, not a cli inputing yes/no/all/quit in vim, so to ncdu, users knew what he is doing, and mostly impossible to feedkey.

I'd still call it TUI and it requires to confirm :)

inputting by lower case to rm -rf is high possibility making mistake.
2. " Give confirmation prompt unless all==1
fun! s:NetrwLocalRmFile(path,fname,all)
when all == 1, it was designed to 'no confirm', in the future some places may call this fun too, supposed it is Not ok to rm all directly.

I repeat one more time:

That is not a problem per se and just changing key-bindings for this will probably annoy users.
You just confirmed, that this is okay currently. So no problem currently.

Shane-XB-Qian · 2024-05-11T14:18:09Z

it is gui-like diaglog, not a cli inputing yes/no/all/quit in vim, so to ncdu, users knew what he is doing, and mostly impossible to feedkey.

I'd still call it TUI and it requires to confirm :)

gui-like, -like, or ok, if you like, it is popup window in tui.. um.. the point was not the like, vs/but how it interactive.

inputting by lower case to rm -rf is high possibility making mistake.
2. " Give confirmation prompt unless all==1
fun! s:NetrwLocalRmFile(path,fname,all)
when all == 1, it was designed to 'no confirm', in the future some places may call this fun too, supposed it is Not ok to rm all directly.

I repeat one more time:
1. That is not a problem per se and just changing key-bindings for this will probably annoy users.

it was not having to change but the #14742 break the game (yes, this is a repeat too).
or changing it designed then required some way to save it, that change (to 'rf') also was an annoying or scare to users.

2. You just [confirmed, that this is okay currently](https://github.com/vim/vim/pull/14749#issuecomment-2105687353). So no problem currently.

i cofirmed there now seems 2 functions called s:NetrwLocalRmFile, but knew it was dangerous if there would be more and if all == 1.

chrisbra · 2024-05-11T14:25:04Z

let's just agree to disagree here and let's not waste more time here. :)

Thanks all.

Shane-XB-Qian · 2024-05-11T14:29:12Z

ok, fine, to pass-by vim users, if you scared the mis-typed 'D' rm -rf all your files, then try to patch this PR by yourself.

fix: make 'rm -rf foo/' be a bit harder

b4ba1ab

// if not back out vim#14742 // then make it be a bit harder to make mistake to rm all // // also seems recover its original logic of 'all' argument Signed-off-by: shane.xb.qian <shane.qian@foxmail.com>

github-actions bot added the plugin-netrw label May 11, 2024

chrisbra reviewed May 11, 2024

View reviewed changes

Shane-XB-Qian commented May 11, 2024

View reviewed changes

chrisbra closed this May 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make 'rm -rf foo/' be a bit harder #14749

fix: make 'rm -rf foo/' be a bit harder #14749

Shane-XB-Qian commented May 11, 2024

chrisbra May 11, 2024

Shane-XB-Qian May 11, 2024

chrisbra May 11, 2024

Shane-XB-Qian May 11, 2024

chrisbra May 11, 2024

Shane-XB-Qian May 11, 2024

chrisbra May 11, 2024

Shane-XB-Qian May 11, 2024

chrisbra commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

clason commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

chrisbra commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

chrisbra commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

Shane-XB-Qian May 11, 2024

chrisbra commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

chrisbra commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

benknoble commented May 11, 2024

chrisbra commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

chrisbra commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

fix: make 'rm -rf foo/' be a bit harder #14749

fix: make 'rm -rf foo/' be a bit harder #14749

Conversation

Shane-XB-Qian commented May 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbra commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

clason commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

chrisbra commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

chrisbra commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

Choose a reason for hiding this comment

chrisbra commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

chrisbra commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

benknoble commented May 11, 2024

chrisbra commented May 11, 2024

Shane-XB-Qian commented May 11, 2024

chrisbra commented May 11, 2024

Shane-XB-Qian commented May 11, 2024