headers
Mikael Magnusson | 16 Aug 01:45
Gravatar

rfc patch, abort rm instead of only removing the * from the cmdline

It surprised me that this isn't already what happens, what do others think?
Imagine for example the typo
% rm -rf * /tmp
Before, even if you said no to the prompt, it will remove all your
files in /tmp.

--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2515,13 +2515,13 @@ execcmd(Estate state, int input, int output,
int how, int last1)
 	    next = nextnode(node);
 	    if (s[0] == Star && !s[1]) {
 		if (!checkrmall(pwd))
-		    uremnode(args, node);
+		    return;
 	    } else if (l > 2 && s[l - 2] == '/' && s[l - 1] == Star) {
 		char t = s[l - 2];

 		s[l - 2] = 0;
 		if (!checkrmall(s))
-		    uremnode(args, node);
+		    return;
 		s[l - 2] = t;
 	    }
 	}

--

-- 
Mikael Magnusson
(Continue reading)

Permalink | Reply |
headers
Richard Hartmann | 16 Aug 17:58

Re: rfc patch, abort rm instead of only removing the * from the cmdline

On Sat, Aug 16, 2008 at 01:49, Mikael Magnusson <mikachu <at> gmail.com> wrote:

> % rm -rf * /tmp
> Before, even if you said no to the prompt, it will remove all your
> files in /tmp.

I am not quite sure what typo you want to catch? Maybe

  rm -rf */tmp

but I am not sure.

> -                   uremnode(args, node);
> +                   return;

Silently aborting on a valid syntax is always bad. As long as you
are in there, why not add a warning, informing the user of what
you are doing? Also, all new code should have comments, imo :)

Richard
Permalink | Reply |
headers
Mikael Magnusson | 16 Aug 18:02
Gravatar

Re: rfc patch, abort rm instead of only removing the * from the cmdline

2008/8/16 Richard Hartmann <richih.mailinglist <at> gmail.com>:
> On Sat, Aug 16, 2008 at 01:49, Mikael Magnusson <mikachu <at> gmail.com> wrote:
>
>> % rm -rf * /tmp
>> Before, even if you said no to the prompt, it will remove all your
>> files in /tmp.
>
> I am not quite sure what typo you want to catch? Maybe
>
>  rm -rf */tmp
>
> but I am not sure.
>
>
>> -                   uremnode(args, node);
>> +                   return;
>
> Silently aborting on a valid syntax is always bad. As long as you
> are in there, why not add a warning, informing the user of what
> you are doing? Also, all new code should have comments, imo :)

Maybe it was unclear from the context I gave, but I'm talking about this prompt:
% rm *
zsh: sure you want to delete all the files in /tmp/a [yn]?

Answering no to this prompt, I would expect the command to not be run,
but it is, with the * removed.

--

-- 
Mikael Magnusson
(Continue reading)

Permalink | Reply |
headers
Richard Hartmann | 16 Aug 18:14

Re: rfc patch, abort rm instead of only removing the * from the cmdline

On Sat, Aug 16, 2008 at 18:02, Mikael Magnusson <mikachu <at> gmail.com> wrote:

> Answering no to this prompt, I would expect the command to not be run,
> but it is, with the * removed.

Agree 100%! This must be changed asap!

As Peter is on holidays, can Bart chip in on this?

Richard
Permalink | Reply |
headers
Stephane Chazelas | 16 Aug 22:52

Re: rfc patch, abort rm instead of only removing the * from the cmdline

On Sat, Aug 16, 2008 at 06:02:13PM +0200, Mikael Magnusson wrote:
[...]
> Maybe it was unclear from the context I gave, but I'm talking about this prompt:
> % rm *
> zsh: sure you want to delete all the files in /tmp/a [yn]?
> 
> Answering no to this prompt, I would expect the command to not be run,
> but it is, with the * removed.
[...] 

With what version of zsh? I can't reproduce it here with the
current one.

% rm *
zsh: sure you want to delete all the files in /tmp/a [yn]? n
% rm
rm: missing operand
Try `rm --help' for more information.
%

Am I missing something?

--

-- 
Stéphane
Permalink | Reply |
headers
Mikael Magnusson | 16 Aug 22:58
Gravatar

Re: rfc patch, abort rm instead of only removing the * from the cmdline

2008/8/16 Stephane Chazelas <Stephane_Chazelas <at> yahoo.fr>:
> On Sat, Aug 16, 2008 at 06:02:13PM +0200, Mikael Magnusson wrote:
> [...]
>> Maybe it was unclear from the context I gave, but I'm talking about this prompt:
>> % rm *
>> zsh: sure you want to delete all the files in /tmp/a [yn]?
>>
>> Answering no to this prompt, I would expect the command to not be run,
>> but it is, with the * removed.
> [...]
>
> With what version of zsh? I can't reproduce it here with the
> current one.
>
> % rm *
> zsh: sure you want to delete all the files in /tmp/a [yn]? n
> % rm
> rm: missing operand
> Try `rm --help' for more information.
> %
>
> Am I missing something?

It looks like it does abort the command if the * is the _only_ argument...
I couldn't check easily as I already have the patch applied :).

% rm * --help
zsh: sure you want to delete all the files in /home/mikachu/a [yn]? n
Usage: rm [OPTION]... FILE...
Remove (unlink) the FILE(s).
(Continue reading)

Permalink | Reply |
headers
Richard Hartmann | 17 Aug 00:30

Re: rfc patch, abort rm instead of only removing the * from the cmdline

On Sat, Aug 16, 2008 at 22:58, Mikael Magnusson <mikachu <at> gmail.com> wrote:

> I couldn't check easily as I already have the patch applied :).

I can confirm for 4.3.6-6 on Debian unstable.

Richard
Permalink | Reply |
headers
Stephane Chazelas | 17 Aug 08:57

Re: rfc patch, abort rm instead of only removing the * from the cmdline

On Sat, Aug 16, 2008 at 10:58:19PM +0200, Mikael Magnusson wrote:
[...]
> It looks like it does abort the command if the * is the _only_ argument...
> I couldn't check easily as I already have the patch applied :).
> 
> % rm * --help
> zsh: sure you want to delete all the files in /home/mikachu/a [yn]? n
> Usage: rm [OPTION]... FILE...
> Remove (unlink) the FILE(s).
[...]

OK, I can reproduce it now. And I agree with you it wasn't quite
what I expected. But then, I think the fix should rather be
something like:

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.140
diff -p -u -r1.140 exec.c
--- Src/exec.c	13 Aug 2008 21:02:02 -0000	1.140
+++ Src/exec.c	17 Aug 2008 06:56:17 -0000
@@ -2547,19 +2547,24 @@ execcmd(Estate state, int input, int out

 	    next = nextnode(node);
 	    if (s[0] == Star && !s[1]) {
-		if (!checkrmall(pwd))
-		    uremnode(args, node);
+		if (!checkrmall(pwd)) {
+		    errflag = 1;
(Continue reading)

Permalink | Reply |
headers
Bart Schaefer | 17 Aug 22:42

Re: rfc patch, abort rm instead of only removing the * from the cmdline

On Aug 16,  1:49am, Mikael Magnusson wrote:
}
} It surprised me that this isn't already what happens, what do others think?
} Imagine for example the typo
} % rm -rf * /tmp
} Before, even if you said no to the prompt, it will remove all your
} files in /tmp.

On Aug 16,  6:14pm, Richard Hartmann wrote:
} 
} Agree 100%! This must be changed asap!
} 
} As Peter is on holidays, can Bart chip in on this?

Well ...

It having been this way for far more than a decade without anyone ever
complaining before, I'm not sure "asap!" really applies.

The existing behavior really is what Paul Falstad wanted the shell to
do, all those years ago.  Like several of the very oldest features,
it's rather woefully under-documented in the man pages.  If you want
to abort the whole command, you're supposed to interrupt it with ^C,
not answer "n" -- the prompt is really just to give you an extra moment
to think about what a stupid thing you might be about to do, not to
second-guess your entire command line; compare the RM_STAR_WAIT option
(which is a lot more recent).

That said, if it were going to be changed, Stephane's patch is better.
(Continue reading)

Permalink | Reply |
headers
Richard Hartmann | 18 Aug 12:04

Re: rfc patch, abort rm instead of only removing the * from the cmdline

On Sun, Aug 17, 2008 at 22:42, Bart Schaefer <schaefer <at> brasslantern.com> wrote:

> It having been this way for far more than a decade without anyone ever
> complaining before, I'm not sure "asap!" really applies.

Something to be in a certain state for a long time does mean it should
not be fixed. To qualify, I did mean 'with the next release', not 'release
emergency patches', though.
Also, I know several people who have had massive data loss due to
misunderstanding what a program wants to do, or even due to a bug,
and never bothered to file a bug. That is unfortunate, but a fact. So
the mere lack of feedback does not mean it did not happen. Neither
does it mean it did happen, of course. Yet, especially with such
counter-intuitve behaviour, are slow to file issues. Especially if they
are not sure what happened or notice a long time after the fact.

> The existing behavior really is what Paul Falstad wanted the shell to
> do, all those years ago.  Like several of the very oldest features,
> it's rather woefully under-documented in the man pages.  If you want
> to abort the whole command, you're supposed to interrupt it with ^C,
> not answer "n" -- the prompt is really just to give you an extra moment
> to think about what a stupid thing you might be about to do, not to
> second-guess your entire command line; compare the RM_STAR_WAIT option
> (which is a lot more recent).

I can't think of a situation where ZSH queries me if I want to do
something and answering no does anything other than stopping to
execute whatever it wanted to do. The CORRECT option is somewhat
special, but follows the same pattern.
Thus, not aborting the command completely is against most user's
(Continue reading)

Permalink | Reply |
headers
Peter Stephenson | 21 Aug 17:58
Favicon

Re: rfc patch, abort rm instead of only removing the * from the cmdline

On Sun, 17 Aug 2008 13:42:15 -0700
Bart Schaefer <schaefer <at> brasslantern.com> wrote:
> Ideally I suppose this prompt should work like the prompt for "correct",
> giving you "nyae" as choices so you can choose to abort or to edit your
> typo.  Then it would be more obvious that "n" was not going to entirely
> whack the command.

That would certainly make things more consistent; I'm not sure how easy
that is at this point in the code, however, which I think (without
checking) is quite late into execution.  Does anyone want to look?

--

-- 
Peter Stephenson <p.w.stephenson <at> ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/
Permalink | Reply |
headers
Bart Schaefer | 22 Aug 07:17

Re: rfc patch, abort rm instead of only removing the * from the cmdline

On Aug 21,  4:58pm, Peter Stephenson wrote:
} Subject: Re: rfc patch, abort rm instead of only removing the * from the c
}
} On Sun, 17 Aug 2008 13:42:15 -0700
} Bart Schaefer <schaefer <at> brasslantern.com> wrote:
} > Ideally I suppose this prompt should work like the prompt for "correct",
} > giving you "nyae" as choices so you can choose to abort or to edit
} 
} That would certainly make things more consistent; I'm not sure how easy
} that is at this point in the code, however, which I think (without
} checking) is quite late into execution.  Does anyone want to look?

It can't be that late, as it has to happen before globbing in order to
find the Star token.  However, correction aborts by making hend() return
false, and relative to that in init.c:loop(), checkrmall() is down in
execode() which is after hend() has finished and even after preexec has
been run.  So the [yna] part would be easy (use Stephane's patch exept
only on a response of "a"), but it would have to unwind (or re-instate)
a fair bit of state to get back to the same command line for "e".

(This thread should probably move to zsh-workers at this point.)
Permalink | Reply |

Gmane