bug#34338: 26.1; delete-file return codes and failures

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#34338: 26.1; delete-file return codes and failures

Boruch Baum
Subject: 26.1; delete-file return codes and failures

Currently:

A1) When delete-file successfully deletes a file, the function returns
    nil, not t.

A2) When the file does not exist, the function returns the same
    value - nil.

A3) When the file has been chmod'ed -w, the function performs the
    operation, with no regard to any option to emulate or not emulate 'rm
    -f', and also returns the identical value - nil.

A4) When either the file's containing folder is chmod'ed -x, or the file
    is chattr'ed +i, the function crashes.


I'd like to suggest:

  delete-file FILE &optional NOERROR FORCE

B1) return t on success

B2) raise an error when (not NOERROR) and:

  B2.1) file doesn't exist

  B2.2) (and (chmod -w) (not FORCE))

  B2.3) another form of permission denial is encountered

B3) return nil when NOERROR and:

  B2.1) file doesn't exist

  B2.2)  another form of permission denial is encountered


C) maybe log the exact error or reason for nil to *Messages*.



In GNU Emacs 26.1 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.2)
 of 2018-12-26, modified by Debian built on x86-ubc-01
System Description: Devuan GNU/Linux 2.0.0 (ascii)

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



Reply | Threaded
Open this post in threaded view
|

bug#34338: 26.1; delete-file return codes and failures

Michael Albinus
Boruch Baum <[hidden email]> writes:

Hi Boruch,

> I'd like to suggest:
>
>   delete-file FILE &optional NOERROR FORCE

At the very least, keep existing arguments. So it would be

delete-file FILE &optional THRASH NOERROR FORCE

Your other proposals look OK to me. It might be worth to say, that FORCE
could fail for some Tramp backends (but I haven't checked in detail yet).

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

bug#34338: 26.1; delete-file return codes and failures

Boruch Baum
On 2019-02-06 09:24, Michael Albinus wrote:
> Boruch Baum <[hidden email]> writes:
> > I'd like to suggest:
> >
> >   delete-file FILE &optional NOERROR FORCE
>
> At the very least, keep existing arguments. So it would be
>
> delete-file FILE &optional THRASH NOERROR FORCE

Off-topic, but potentially worthy of consideration: For a second, I
didn't realize that 'thrash' might be a typo, and I got excited, because
it made it seem that there existed a hidden feature that I would love
for delete-file to have: some sort of 'thrashing' the file before
un-linking / de-allocating, aka. a secure delete similar to the linux
core-util 'shred'. As background, I noticed this behavior of
'delete-file' when proposing a feature for emacs-w3m to securely scrub a
user's internet browsing history. There[1], I'm currently just calling
'shred' or some other external program of the user's choice.

> Your other proposals look OK to me. It might be worth to say, that FORCE
> could fail for some Tramp backends (but I haven't checked in detail yet).
>
> Best regards, Michael.

Always happy to try to be of help.

[1] https://github.com/emacs-w3m/emacs-w3m/pull/2

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



Reply | Threaded
Open this post in threaded view
|

bug#34338: 26.1; delete-file return codes and failures

Eli Zaretskii
In reply to this post by Boruch Baum
> Date: Tue, 5 Feb 2019 16:47:37 -0500
> From: Boruch Baum <[hidden email]>
>
> Currently:
>
> A1) When delete-file successfully deletes a file, the function returns
>     nil, not t.
>
> A2) When the file does not exist, the function returns the same
>     value - nil.
>
> A3) When the file has been chmod'ed -w, the function performs the
>     operation, with no regard to any option to emulate or not emulate 'rm
>     -f', and also returns the identical value - nil.
>
> A4) When either the file's containing folder is chmod'ed -x, or the file
>     is chattr'ed +i, the function crashes.

The function's return value is not documented, which is an indication
that it is "not useful".  You will see in the code that it always
either returns nil or signals an error.

> I'd like to suggest:
>
>   delete-file FILE &optional NOERROR FORCE

As Michael points out, there's one optional argument already, and we
need to preserve it.

> B2) raise an error when (not NOERROR) and:
>
>   B2.1) file doesn't exist
>
>   B2.2) (and (chmod -w) (not FORCE))
>
>   B2.3) another form of permission denial is encountered

!ERROR and either of the following, or all of them?

In any case, you propose a backward-incompatible change in behavior,
so it won't fly.  We could perhaps do it the other way around: add a
new optional argument ERROR-OUT, which, when non-nil, will cause the
function to signal an error when B2.1 or B2.2 happen (I believe B2.3
already causes an error).  And similarly with FORCE.

IOW, since omitting optional arguments is the same as passing nil for
them, we cannot add new arguments that change behavior if they are
nil, certainly not in a function that is not only command, but also an
important primitive used all over the place in Emacs.

> C) maybe log the exact error or reason for nil to *Messages*.

Not sure what you mean by "exact error or reason", I believe we
already log the reason.

Finally, did you think how to allow users set or reset these new knobs
when invoking delete-file interactively?  I think these options are
mainly for interactive use, and in that case we should have some
convenient way of setting them interactively.

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#34338: 26.1; delete-file return codes and failures

Boruch Baum
On 2019-02-06 18:19, Eli Zaretskii wrote:
> > Date: Tue, 5 Feb 2019 16:47:37 -0500
> > From: Boruch Baum <[hidden email]>
>
> As Michael points out, there's one optional argument already, and we
> need to preserve it.

Yup.

>
> > B2) raise an error when (not NOERROR) and:
> >
> >   B2.1) file doesn't exist
> >
> >   B2.2) (and (chmod -w) (not FORCE))
> >
> >   B2.3) another form of permission denial is encountered
>
> !ERROR and either of the following, or all of them?

Either.

> In any case, you propose a backward-incompatible change in behavior,
> so it won't fly. We could perhaps do it the other way around: add a
> new optional argument ERROR-OUT, which, when non-nil, will cause the
> function to signal an error when B2.1 or B2.2 happen (I believe B2.3
> already causes an error). And similarly with FORCE.

> IOW
> ... (snip) ...

The part that would transform a prior condition of 'crash' to some
return value is a kind of backward-incompatibility that I think most
people would appreciate. The issue would be if there exists code in
emacs that traps on the current crashes (eg. using 'condition-case'). I
like the idea of requiring non-nil to force an ERROR-OUT, but that would
inconsistent with the more common emacs use of NOERROR as an arg.

For a proposed FORCE arg, backward-incompatibility is a positive
feature, a bug-fix, in that emacs is not currently respecting a user's
environment choice to place a level of protection from deleting the
target file. A user made a conscious positive act at some point to
put a level of protection on a file, that in the usual context, 'rm',
would require further interaction in order to proceed, and emacs is
currently not abiding by that.

> > C) maybe log the exact error or reason for nil to *Messages*.
>
> Not sure what you mean by "exact error or reason", I believe we
> already log the reason.

For me, in response chmod -x $parent_dir, the error message is:

  eval: Removing old name: Permission denied, /home/boruch/foo/bar

And the response to chattr +i bar

  eval: Removing old name: Operation not permitted, /home/boruch/foo/bar

So the messages are unique, but not clear.

> Finally, did you think how to allow users set or reset these new knobs
> when invoking delete-file interactively?

No! Um ....

> I think these options are mainly for interactive use, and in that case
> we should have some convenient way of setting them interactively.

So, currently the prefix arg is already being used for TRASH ...

Interactively, having a 'knob' for NOERROR (or its reverse, per above)
doesn't seem to me necessary ... what value does it add for an
interactive invocation? ...

Interactively, for FORCE, there's no 'lossage-advantage' (borrowed term
based upon `C-h l') to requiring a user to add keystrokes up-front prior
to delete-file, rather than requiring the user to y-or-n if the file has
protection. So, that doesn't seem necessary, unless you want to add a
feature to allow delete-file to operate on a regex or sequence of
filenames instead of a single file.

That leaves my off-topic comment to Michael about a THRASH (not TRASH,
but maybe better SHRED) arg. My input on that would be to have such a
feature as a separate function (eg. shred-file), and not an arg to
delete-file.

חודש טוב ואדר כפול שמח.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



Reply | Threaded
Open this post in threaded view
|

bug#34338: 26.1; delete-file return codes and failures

Eli Zaretskii
> Date: Wed, 6 Feb 2019 16:02:11 -0500
> From: Boruch Baum <[hidden email]>
> Cc: [hidden email]
>
> > > B2) raise an error when (not NOERROR) and:
> > >
> > >   B2.1) file doesn't exist
> > >
> > >   B2.2) (and (chmod -w) (not FORCE))
> > >
> > >   B2.3) another form of permission denial is encountered
> >
> > !ERROR and either of the following, or all of them?
>
> Either.
>
> > In any case, you propose a backward-incompatible change in behavior,
> > so it won't fly. We could perhaps do it the other way around: add a
> > new optional argument ERROR-OUT, which, when non-nil, will cause the
> > function to signal an error when B2.1 or B2.2 happen (I believe B2.3
> > already causes an error). And similarly with FORCE.
>
> > IOW
> > ... (snip) ...
>
> The part that would transform a prior condition of 'crash' to some
> return value is a kind of backward-incompatibility that I think most
> people would appreciate.

I'm more worried about the opposite: signaling an error where we
currently silently do nothing.

> For a proposed FORCE arg, backward-incompatibility is a positive
> feature, a bug-fix

Sorry, it's too late to fix such "bugs" in veteran interfaces.  We
must do that in backward-compatible way.

> > > C) maybe log the exact error or reason for nil to *Messages*.
> >
> > Not sure what you mean by "exact error or reason", I believe we
> > already log the reason.
>
> For me, in response chmod -x $parent_dir, the error message is:
>
>   eval: Removing old name: Permission denied, /home/boruch/foo/bar
>
> And the response to chattr +i bar
>
>   eval: Removing old name: Operation not permitted, /home/boruch/foo/bar
>
> So the messages are unique, but not clear.

The description comes from the error code returned by a C library
function.  Doing more than that would mean additional checks, which
will be expensive and probably non-portable.  I don't see the benefit.
I mean, why isn't "Operation not permitted" enough, it tells you that
your user is not permitted to do that, which is clear enough IMO.



Reply | Threaded
Open this post in threaded view
|

bug#34338: 26.1; delete-file return codes and failures

Boruch Baum
On 2019-02-07 05:36, Eli Zaretskii wrote:

> > Date: Wed, 6 Feb 2019 16:02:11 -0500
> > From: Boruch Baum <[hidden email]>
> >
> > So the messages are unique, but not clear.
>
> The description comes from the error code returned by a C library
> function.  Doing more than that would mean additional checks, which
> will be expensive and probably non-portable.  I don't see the benefit.
> I mean, why isn't "Operation not permitted" enough, it tells you that
> your user is not permitted to do that, which is clear enough IMO.

Reasonable.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0