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

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

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

[Michael Albinus]

Boruch Baum <boruch_baum@gmx.com> 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.

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

[Boruch Baum]

On 2019-02-06 09:24, Michael Albinus wrote:
> Boruch Baum <boruch_baum@gmx.com> 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

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

[Eli Zaretskii]

> Date: Tue, 5 Feb 2019 16:47:37 -0500
> From: Boruch Baum <boruch_baum@gmx.com>
> 
> 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.

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 <boruch_baum@gmx.com>
>
> 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

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

[Eli Zaretskii]

> Date: Wed, 6 Feb 2019 16:02:11 -0500
> From: Boruch Baum <boruch_baum@gmx.com>
> Cc: 34338@debbugs.gnu.org
> 
> > > 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.

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 <boruch_baum@gmx.com>
> >
> > 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

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

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

Boruch Baum <boruch_baum@gmx.com> writes:

> On 2019-02-07 05:36, Eli Zaretskii wrote:
>> > Date: Wed, 6 Feb 2019 16:02:11 -0500
>> > From: Boruch Baum <boruch_baum@gmx.com>
>> >
>> > 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.

It seems to me from the discussion like we don't want to make the proposed
change, since it has an unclear benefit and is backwards
incompatible.

Is there is something I'm missing here?  Otherwise, I think we can close
this as wontfix.

Best regards,
Stefan Kangas

[-- Attachment #2: Type: text/html, Size: 1161 bytes --]

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

[Stefan Kangas]

tags 34338 + wontfix
close 34338
quit

Stefan Kangas <stefan@marxist.se> writes:

> It seems to me from the discussion like we don't want to make the
> proposed change, since it has an unclear benefit and is backwards
> incompatible.
>
> Is there is something I'm missing here?  Otherwise, I think we can
> close this as wontfix.

No one has objected within 3 weeks, so I'm closing this as wontfix.

Best regards,
Stefan Kangas

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

[Boruch Baum]

How is it backwards incompatable? If the prior behavior was undefined,
no one would have been using it for anything. From their perspective, a
defined is just another form of undefined behavior, if you get my drift.

On 2019-10-30 22:07, Stefan Kangas wrote:
> tags 34338 + wontfix
> close 34338
> quit
>
> Stefan Kangas <stefan@marxist.se> writes:
>
> > It seems to me from the discussion like we don't want to make the
> > proposed change, since it has an unclear benefit and is backwards
> > incompatible.
> >
> > Is there is something I'm missing here?  Otherwise, I think we can
> > close this as wontfix.
>
> No one has objected within 3 weeks, so I'm closing this as wontfix.
>
> Best regards,
> Stefan Kangas

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

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

[Stefan Kangas]

Boruch Baum <boruch_baum@gmx.com> writes:

> How is it backwards incompatable? If the prior behavior was undefined,
> no one would have been using it for anything. From their perspective, a
> defined is just another form of undefined behavior, if you get my drift.

Having re-read the entire discussion, I think that we should perhaps
clarify what we're talking about.  Your proposal had three different
parts to it AFAICT:

Boruch Baum <boruch_baum@gmx.com> writes:

> B1) return t on success

Eli said that: "The function's return value is not documented, which is
an indication that it is 'not useful'."

Do you have a use-case in mind here?  Can't the caller just check using
'file-exists-p' if it matters instead?

> 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

Eli replied: "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."

I'm not against this part, but again I'm not sure what is the use-case
here.  And what about having the caller use 'file-exists-p' instead?

> B3) return nil when NOERROR and:
> 
>   B2.1) file doesn't exist
> 
>   B2.2)  another form of permission denial is encountered

I guess the inverted version of the above makes sense (return nil unless
ERROR-OUT), iff we add B1 and B2.

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

This part was agreed to be left out already, and I agree.

Best regards,
Stefan Kangas

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

[Boruch Baum]

On 2019-10-30 23:52, Stefan Kangas wrote:
> Boruch Baum <boruch_baum@gmx.com> writes:
>
> Do you have a use-case in mind here?  Can't the caller just check using
> 'file-exists-p' if it matters instead?

It _should_ *always* matter. Did you delete the file or not? If it didn't
matter, why waste your time attempting the deletion?

Having delete-file return the value is: a) consistent with what I think
is the expectation of most CS folks; b) the most efficient, since it
already knows the exact point of any failure, and basically just passes
the return value it gets from the underlying OS.

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

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

[Eli Zaretskii]

> Date: Wed, 30 Oct 2019 18:22:00 -0400
> From: Boruch Baum <boruch_baum@gmx.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, 34338@debbugs.gnu.org
> 
> How is it backwards incompatable? If the prior behavior was undefined,
> no one would have been using it for anything. From their perspective, a
> defined is just another form of undefined behavior, if you get my drift.

My "backward-incompatible" response was only related to your
suggestion to signal an error when a file doesn't exist, and/or modify
the function's signature in incompatible ways.  I didn't object to
making this function return a meaningful value, if the argument list
remains compatible, i.e. if existing code doesn't need to change.

However, if we make the function return meaningful values, we must
make sure it always does so, and in consistent manner.  For example,
the file handlers should be documented to return the same values (and
existing handlers should be modified to actually do so), moving the
file to trash, whether via a system-dependent primitive or application
code in files.el, should return the same value for the same
conditions, etc.

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

[Eli Zaretskii]

> Date: Wed, 30 Oct 2019 21:33:29 -0400
> From: Boruch Baum <boruch_baum@gmx.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, 34338@debbugs.gnu.org
> 
> > Do you have a use-case in mind here?  Can't the caller just check using
> > 'file-exists-p' if it matters instead?
> 
> It _should_ *always* matter. Did you delete the file or not? If it didn't
> matter, why waste your time attempting the deletion?

The current implementation basically is a moral equivalent of "rm -f".
It also is biased towards interactive usage, where the return value is
not very important.

Does it answer your question?

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

[Boruch Baum]

On 2019-10-31 16:32, Eli Zaretskii wrote:
> The current implementation basically is a moral equivalent of "rm -f".
> It also is biased towards interactive usage, where the return value is
> not very important.
>
> Does it answer your question?

Not satisfactorily.

Applying the notion of '_moral_ equivalence' to a programming command
option strikes me as alien, but as an exercise, the only _moral_
standard that I've come up with for "rm -f" is an immoral standard of
inconsiderateness. What I mean is that someone at some time put some
sort of block on deleting a file, and you're saying that emacs as its
*default* action is to inconsiderately try to do its best to ignore any
such block. As an issue in _morality_, it sounds quite inconsiderate and
thus presumably undesirable if one's decision-basis is some form of
morality.

Independent of morality, it's also inconsistent with the default
behavior of comparable programming contexts, and thus violates POLA
(principle of least astonishment). For example, compare the default
action of the written command delete-file with your own expressed basis
... the shell command rm, run interactively from the command line.

A better comparison basis would be other forms of lisp. Rabbi Google
tells me that both common-lisp and scheme signal an error on failure.
The only other lisp I have handy to try is another GNU lisp, guile
2.0.13, which also does not signal an error on a file marked 'chmod -w'.
Outside of lisp, python behaves similarly. So that's several POLA points
in emacs' favor.

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

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

[Eli Zaretskii]

> Date: Fri, 1 Nov 2019 05:34:23 -0400
> From: Boruch Baum <boruch_baum@gmx.com>
> Cc: stefan@marxist.se, 34338@debbugs.gnu.org
> 
> On 2019-10-31 16:32, Eli Zaretskii wrote:
> > The current implementation basically is a moral equivalent of "rm -f".
> > It also is biased towards interactive usage, where the return value is
> > not very important.
> >
> > Does it answer your question?
> 
> Not satisfactorily.

Actually, from what you wrote, it follows that it did answer your
question, you just disagree that delete-file should have been coded to
work as it does.

Please note that I didn't try to justify its behavior, only to explain
its rationale, in response to your question "If it didn't matter, why
waste your time attempting the deletion?"  IOW, there certainly _are_
valid use cases where "it doesn't matter", and the fact that "rm -f"
exists is an evidence.

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

[Stefan Kangas]

tags 34338 - wontfix
reopen 34338
thanks

Boruch Baum <boruch_baum@gmx.com> writes:

> On 2019-10-30 23:52, Stefan Kangas wrote:
>> Boruch Baum <boruch_baum@gmx.com> writes:
>>
>> Do you have a use-case in mind here?  Can't the caller just check using
>> 'file-exists-p' if it matters instead?
>
> It _should_ *always* matter. Did you delete the file or not? If it didn't
> matter, why waste your time attempting the deletion?

I think Eli answered this point well, and I still think a use-case
would have been helpful here.  If I was trying to convince people to
spend time on this, I would try to provide a code example where the
suggested change would simplify the code and/or make maintenance
easier.

> Having delete-file return the value is: a) consistent with what I think
> is the expectation of most CS folks;

Not sure if I'm like most CS folks, but that wasn't my expectation.
IME, this works differently in different languages.

> b) the most efficient, since it already knows the exact point of any
> failure, and basically just passes the return value it gets from the
> underlying OS.

That's a good point.

In any case, I closed the bug in the belief that there was nothing
more to do here given the lack of response.  I'm not against the
change as such.

I've therefore reopened the bug in the hope that someone would want to
take a crack at implementing this.  Boruch, perhaps you could consider
volunteering?

Best regards,
Stefan Kangas