GNU bug report logs - #28797
26.0.90; Improve printing of error on catching file-error in dired

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 28797 in the body.
You can then email your comments to 28797 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Kaushal Modi <kaushal.modi <at> gmail.com>
To: "bug-gnu-emacs <at> gnu.org" <bug-gnu-emacs <at> gnu.org>,
 Drew Adams <drew.adams <at> oracle.com>, 
 Noam Postavsky <npostavs <at> gmail.com>, Tino Calancha <tino.calancha <at> gmail.com>
Subject: 26.0.90; Improve printing of error on catching file-error in dired
Date: Thu, 12 Oct 2017 14:56:08 +0000

[Message part 1 (text/plain, inline)]

Hello,

This is a spin-off bug report from
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28792#29

This bug report is to do with:

1. Improving the format of error messages generated by catching error
signals from C, so that the errors look almost like readable English
instead of a dump of a list.
2. Those errors should generate a backtrace.. right now, a user needs to
update the debug-on-message variable to force back traces for such messages
which is, first: more cumbersome than doing M-x toggle-debug-on-error and
then recreating the error, and second: not a common knowledge.

Excerpt from the above referenced debbugs thread for continuity:

=====
OK, but it seems non-standard compared to error messages from Elisp land.

Shouldn't

(file-error Non-regular file Is a directory /home/kmodi/.emacs.d/foo)

look like:

file-error: Non-regular file: /home/kmodi/.emacs.d/foo is a directory

- Why those parentheses?
- Why are "N" and "I" capitalized in-between that "sentence".. error
messages are usually sentences without ending in period, right?
- Above instead looks like a list printed with 3 elements.

Also, the error is not-informative.. the user is trying to trash foo/ and
knows that foo/ is a directory.. so how would the below help?

(file-error Non-regular file Is a directory /home/kmodi/.emacs.d/foo)
=====


-- 

Kaushal Modi

[Message part 2 (text/html, inline)]

Message #8 received at 28797 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Kaushal Modi <kaushal.modi <at> gmail.com>
Cc: 28797 <at> debbugs.gnu.org, Drew Adams <drew.adams <at> oracle.com>,
 Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#28797: 26.0.90; Improve printing of error on catching
 file-error in dired
Date: Thu, 12 Oct 2017 11:08:51 -0400

On Thu, Oct 12, 2017 at 10:56 AM, Kaushal Modi <kaushal.modi <at> gmail.com> wrote:

> 2. Those errors should generate a backtrace.. right now, a user needs to
> update the debug-on-message variable to force back traces for such messages
> which is, first: more cumbersome than doing M-x toggle-debug-on-error and
> then recreating the error, and second: not a common knowledge.

Another possibility is setting debug-on-signal, see (elisp) Handling Errors.

PS. please use X-Debbugs-CC to Cc bugs to people other than the bug
list, that we get the version of the mail with the bug number, see
https://debbugs.gnu.org/Reporting.html

Message #11 received at 28797 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: Kaushal Modi <kaushal.modi <at> gmail.com>
Cc: 28797 <at> debbugs.gnu.org, drew.adams <at> oracle.com, tino.calancha <at> gmail.com
Subject: Re: bug#28797: 26.0.90;
 Improve printing of error on catching file-error in dired
Date: Thu, 12 Oct 2017 23:16:28 -0400

[Message part 1 (text/plain, inline)]

severity 28797 minor
tags 28797 + patch
quit

Kaushal Modi <kaushal.modi <at> gmail.com> writes:

> 1. Improving the format of error messages generated by catching error
> signals from C, so that the errors look almost like readable English
> instead of a dump of a list.
> 2. Those errors should generate a backtrace.. right now, a user needs
> to update the debug-on-message variable to force back traces for such
> messages which is, first: more cumbersome than doing M-x
> toggle-debug-on-error and then recreating the error, and second: not
> a common knowledge.

Here's a patch, uses error-message-string for #1,
condition-case-unless-debug for #2.

[v1-0001-Improve-dired-deletion-error-handling-Bug-28797.patch (text/x-diff, inline)]

From a150d99f8e278e9b90240e4b0c460ca974b32aeb Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs <at> gmail.com>
Date: Thu, 12 Oct 2017 23:12:00 -0400
Subject: [PATCH v1] Improve dired deletion error handling (Bug#28797)

* lisp/dired.el (dired-internal-do-deletions): Use
condition-case-unless-debug.  Use `error-message-string' to produce a
human readable error message.
---
 lisp/dired.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/dired.el b/lisp/dired.el
index 9e09d349f7..b24fea703a 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -3132,7 +3132,7 @@ dired-internal-do-deletions
 	    (while l
 	      (goto-char (cdr (car l)))
 	      (let ((inhibit-read-only t))
-		(condition-case err
+		(condition-case-unless-debug err
 		    (let ((fn (car (car l))))
 		      (dired-delete-file fn dired-recursive-deletes trash)
 		      ;; if we get here, removing worked
@@ -3143,7 +3143,7 @@ dired-internal-do-deletions
 		       #'dired-delete-entry fn))
                   (quit (throw '--delete-cancel (message "OK, canceled")))
 		  (error ;; catch errors from failed deletions
-		   (dired-log "%s\n" err)
+		   (dired-log "%s: %s\n" (car err) (error-message-string err))
 		   (setq failures (cons (car (car l)) failures)))))
 	      (setq l (cdr l)))
 	    (if (not failures)
-- 
2.11.0

Message #18 received at 28797 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Noam Postavsky <npostavs <at> users.sourceforge.net>
Cc: 28797 <at> debbugs.gnu.org, tino.calancha <at> gmail.com, kaushal.modi <at> gmail.com
Subject: Re: bug#28797: 26.0.90;
 Improve printing of error on catching file-error in dired
Date: Fri, 13 Oct 2017 11:58:52 +0300

> From: Noam Postavsky <npostavs <at> users.sourceforge.net>
> Date: Thu, 12 Oct 2017 23:16:28 -0400
> Cc: 28797 <at> debbugs.gnu.org, tino.calancha <at> gmail.com
> 
> > 1. Improving the format of error messages generated by catching error
> > signals from C, so that the errors look almost like readable English
> > instead of a dump of a list.
> > 2. Those errors should generate a backtrace.. right now, a user needs
> > to update the debug-on-message variable to force back traces for such
> > messages which is, first: more cumbersome than doing M-x
> > toggle-debug-on-error and then recreating the error, and second: not
> > a common knowledge.
> 
> Here's a patch, uses error-message-string for #1,

This doesn't handle all of the parts of the complaint, does it?

> condition-case-unless-debug for #2.

This is quite a radical change in very old behavior, so if it is
deemed a good idea, it should go to master.

Thanks.

Message #21 received at 28797 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28797 <at> debbugs.gnu.org, kaushal.modi <at> gmail.com, tino.calancha <at> gmail.com
Subject: Re: bug#28797: 26.0.90;
 Improve printing of error on catching file-error in dired
Date: Fri, 13 Oct 2017 09:15:01 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Here's a patch, uses error-message-string for #1,
>
> This doesn't handle all of the parts of the complaint, does it?

The resulting error will look like

    file-error: Non-regular file: Is a directory, /home/kmodi/.emacs.d/foo

It's not quite the perfect sentence envisioned on the OP, but it gets
all the information across, with punctuation separating the parts.

    Shouldn't

    (file-error Non-regular file Is a directory /home/kmodi/.emacs.d/foo)

    look like:

    file-error: Non-regular file: /home/kmodi/.emacs.d/foo is a directory

    - Why those parentheses?
    - Why are "N" and "I" capitalized in-between that "sentence".. error
    messages are usually sentences without ending in period, right?
    - Above instead looks like a list printed with 3 elements.

For this part:

    Also, the error is not-informative.. the user is trying to trash foo/
    and knows that foo/ is a directory.. so how would the below help?

I guess what would help is printing "there is a bug with Emacs renaming
a directory across filesystems", but that seems a bit out of reach for a
simple error formatting function...

>> condition-case-unless-debug for #2.
>
> This is quite a radical change in very old behavior, so if it is
> deemed a good idea, it should go to master.

Hmm, it doesn't seem that radical to me, but I don't have a problem
putting it only to master, or even not doing that at all and just saying
the user should use debug-on-signal.

Message #24 received at 28797 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Noam Postavsky <npostavs <at> users.sourceforge.net>
Cc: 28797 <at> debbugs.gnu.org, kaushal.modi <at> gmail.com, tino.calancha <at> gmail.com
Subject: Re: bug#28797: 26.0.90;
 Improve printing of error on catching file-error in dired
Date: Fri, 13 Oct 2017 16:43:33 +0300

> From: Noam Postavsky <npostavs <at> users.sourceforge.net>
> Cc: 28797 <at> debbugs.gnu.org,  tino.calancha <at> gmail.com,  kaushal.modi <at> gmail.com
> Date: Fri, 13 Oct 2017 09:15:01 -0400
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> Here's a patch, uses error-message-string for #1,
> >
> > This doesn't handle all of the parts of the complaint, does it?
> 
> The resulting error will look like
> 
>     file-error: Non-regular file: Is a directory, /home/kmodi/.emacs.d/foo
> 
> It's not quite the perfect sentence envisioned on the OP, but it gets
> all the information across, with punctuation separating the parts.

OK, close enough for me.

> >> condition-case-unless-debug for #2.
> >
> > This is quite a radical change in very old behavior, so if it is
> > deemed a good idea, it should go to master.
> 
> Hmm, it doesn't seem that radical to me, but I don't have a problem
> putting it only to master, or even not doing that at all and just saying
> the user should use debug-on-signal.

It's radical because we never produce backtrace in similar cases
anywhere else in Emacs.

Let's wait to hear what others think about this part.

Thanks.

Message #27 received at 28797 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28797 <at> debbugs.gnu.org, tino.calancha <at> gmail.com, kaushal.modi <at> gmail.com
Subject: Re: bug#28797: 26.0.90;
 Improve printing of error on catching file-error in dired
Date: Sun, 22 Oct 2017 12:28:11 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:

>> >> condition-case-unless-debug for #2.
>> >
>> > This is quite a radical change in very old behavior, so if it is
>> > deemed a good idea, it should go to master.
>> 
>> Hmm, it doesn't seem that radical to me, but I don't have a problem
>> putting it only to master, or even not doing that at all and just saying
>> the user should use debug-on-signal.
>
> It's radical because we never produce backtrace in similar cases
> anywhere else in Emacs.

I'm not sure which cases you consider "similar".

> Let's wait to hear what others think about this part.

Not seeing any opinions, so here is a fact: due to bug#11218, changing
this breaks dired-test-bug27940 [1].  Fixing #11218 would involve some
major changes to ert internals.

[1]: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27940#65

Message #30 received at 28797 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Noam Postavsky <npostavs <at> users.sourceforge.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Tino Calancha <tino.calancha <at> gmail.com>,
 28797 <at> debbugs.gnu.org, kaushal.modi <at> gmail.com
Subject: Re: bug#28797: 26.0.90; Improve printing of error on catching
 file-error in dired
Date: Wed, 25 Oct 2017 00:41:23 +0900 (JST)

On Sun, 22 Oct 2017, Noam Postavsky wrote:

>> Let's wait to hear what others think about this part.
>
> Not seeing any opinions, so here is a fact: due to bug#11218, changing
> this breaks dired-test-bug27940 [1].  Fixing #11218 would involve some
> major changes to ert internals.
>
> [1]: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27940#65

>The "no" case of dired-test-bug27940 is failing now.  I guess if
>RECURSIVE is set to nil, we should not try to delete non-empty
>directories, or maybe just catch the error if it happens?
I think that might be OK.  Following add that change on top
of your original patch:

--8<-----------------------------cut here---------------start------------->8---
commit 36c924fca0b4cde3a320b10d40e9453e55170a0f
Author: Tino Calancha <tino.calancha <at> gmail.com>
Date:   Wed Oct 25 00:38:56 2017 +0900

    Improve dired deletion error handling (Bug#28797)

    * lisp/dired (dired-delete-file): If the dir is non-empty and
    RECURSIVE is nil then return 'skip and don't try to delete
    the dir (Bug#28797).
    * lisp/dired.el (dired-internal-do-deletions): Use
    condition-case-unless-debug.  Use `error-message-string' to produce a
    human readable error message.
    Don't call dired-fun-in-all-buffers if `dired-delete-file' returns 'skip.

diff --git a/lisp/dired.el b/lisp/dired.el
index 1ec3ac4f99..74a37da992 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -3062,7 +3062,10 @@ dired-delete-file
                      ('"no" (setq recursive nil))
                      ('"quit" (keyboard-quit)))))
              (setq recursive nil)) ; Empty dir or recursive is nil.
-           (delete-directory file recursive trash))))
+           ;; Don't delete non-empty dirs when recursive is nil.
+           (if (and (not empty-dir-p) (not recursive))
+             'skip
+             (delete-directory file recursive trash)))))

 (defun dired-do-flagged-delete (&optional nomessage)
   "In Dired, delete the files flagged for deletion.
@@ -3134,18 +3137,19 @@ dired-internal-do-deletions
 	    (while l
 	      (goto-char (cdr (car l)))
 	      (let ((inhibit-read-only t))
-		(condition-case err
+		(condition-case-unless-debug err
 		    (let ((fn (car (car l))))
-		      (dired-delete-file fn dired-recursive-deletes trash)
-		      ;; if we get here, removing worked
-		      (setq succ (1+ succ))
-		      (progress-reporter-update progress-reporter succ)
-		      (dired-fun-in-all-buffers
-		       (file-name-directory fn) (file-name-nondirectory fn)
-		       #'dired-delete-entry fn))
+		      (if (eq 'skip (dired-delete-file fn dired-recursive-deletes trash))
+                          nil
+		        ;; if we get here, removing worked
+		        (setq succ (1+ succ))
+		        (progress-reporter-update progress-reporter succ)
+		        (dired-fun-in-all-buffers
+		         (file-name-directory fn) (file-name-nondirectory fn)
+		         #'dired-delete-entry fn)))
                   (quit (throw '--delete-cancel (message "OK, canceled")))
 		  (error ;; catch errors from failed deletions
-		   (dired-log "%s\n" err)
+		   (dired-log "%s: %s\n" (car err) (error-message-string err))
 		   (setq failures (cons (car (car l)) failures)))))
 	      (setq l (cdr l)))
 	    (if (not failures)
--8<-----------------------------cut here---------------end--------------->8---

Message #33 received at 28797 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 28797 <at> debbugs.gnu.org, kaushal.modi <at> gmail.com
Subject: Re: bug#28797: 26.0.90;
 Improve printing of error on catching file-error in dired
Date: Tue, 24 Oct 2017 19:33:30 -0400

Tino Calancha <tino.calancha <at> gmail.com> writes:

> On Sun, 22 Oct 2017, Noam Postavsky wrote:
>

>>The "no" case of dired-test-bug27940 is failing now.  I guess if
>>RECURSIVE is set to nil, we should not try to delete non-empty
>>directories, or maybe just catch the error if it happens?

Just to clarify, I wrote this before I had figured the Bug#11218 issue.
We *are* currently catching the error, but due to Bug#11218 catching
with condition-case-unless-debug (as opposed to condition-case) doesn't
work inside ert tests.

>      * lisp/dired (dired-delete-file): If the dir is non-empty and
>      RECURSIVE is nil then return 'skip and don't try to delete
>      the dir (Bug#28797).

I think it's okay to do this if we think it would be better for users to
avoid triggering the error messages altogether.  But it should be a
separate decision from whether we want condition-case or
condition-case-unless-debug.

Message #36 received at 28797 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28797 <at> debbugs.gnu.org, tino.calancha <at> gmail.com, kaushal.modi <at> gmail.com
Subject: Re: bug#28797: 26.0.90;
 Improve printing of error on catching file-error in dired
Date: Sun, 05 Nov 2017 12:20:02 -0500

tags 28797 fixed
close 28797 26.1
quit

Eli Zaretskii <eliz <at> gnu.org> writes:

>> The resulting error will look like
>> 
>>     file-error: Non-regular file: Is a directory, /home/kmodi/.emacs.d/foo
>> 
>> It's not quite the perfect sentence envisioned on the OP, but it gets
>> all the information across, with punctuation separating the parts.
>
> OK, close enough for me.
>
>> >> condition-case-unless-debug for #2.
>> >
>> > This is quite a radical change in very old behavior, so if it is
>> > deemed a good idea, it should go to master.
>> 
>> Hmm, it doesn't seem that radical to me, but I don't have a problem
>> putting it only to master, or even not doing that at all and just saying
>> the user should use debug-on-signal.
>
> It's radical because we never produce backtrace in similar cases
> anywhere else in Emacs.
>
> Let's wait to hear what others think about this part.

I've pushed just the error-message-string part to emacs-26.  Setting
debug-on-signal is the documented way of getting a backtrace in this
sort of case, so I think it's good enough to just leave as is.

I think Tino's patch is probably a good idea, but it's not related to
this bug, so I'm closing it now.

[1: efd0371c23]: 2017-11-05 11:38:38 -0500
  Improve dired deletion error handling (Bug#28797)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=efd0371c23c5dd04d73980b42d7cf64bbceccb9a

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.