bug#28797: 26.0.90; Improve printing of error on catching file-error in dired

bug#28797: 26.0.90; Improve printing of error on catching file-error in dired

[Kaushal Modi]

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

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

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

bug#28797: 26.0.90; Improve printing of error on catching file-error in dired

[Noam Postavsky]

On Thu, Oct 12, 2017 at 10:56 AM, Kaushal Modi <kaushal.modi@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

bug#28797: 26.0.90; Improve printing of error on catching file-error in dired

[Noam Postavsky]

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

severity 28797 minor
tags 28797 + patch
quit

Kaushal Modi <kaushal.modi@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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 1329 bytes --]

From a150d99f8e278e9b90240e4b0c460ca974b32aeb Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@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

bug#28797: 26.0.90; Improve printing of error on catching file-error in dired

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Thu, 12 Oct 2017 23:16:28 -0400
> Cc: 28797@debbugs.gnu.org, tino.calancha@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.

bug#28797: 26.0.90; Improve printing of error on catching file-error in dired

[Noam Postavsky]

Eli Zaretskii <eliz@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.

bug#28797: 26.0.90; Improve printing of error on catching file-error in dired

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Cc: 28797@debbugs.gnu.org,  tino.calancha@gmail.com,  kaushal.modi@gmail.com
> Date: Fri, 13 Oct 2017 09:15:01 -0400
> 
> Eli Zaretskii <eliz@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.

bug#28797: 26.0.90; Improve printing of error on catching file-error in dired

[Noam Postavsky]

Eli Zaretskii <eliz@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

bug#28797: 26.0.90; Improve printing of error on catching file-error in dired

[Tino Calancha]

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@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---

bug#28797: 26.0.90; Improve printing of error on catching file-error in dired

[Noam Postavsky]

Tino Calancha <tino.calancha@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.

bug#28797: 26.0.90; Improve printing of error on catching file-error in dired

[Noam Postavsky]

tags 28797 fixed
close 28797 26.1
quit

Eli Zaretskii <eliz@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