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

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

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

Kaushal Modi
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

Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Noam Postavsky-2
In reply to this post by Kaushal Modi
severity 28797 minor
tags 28797 + patch
quit

Kaushal Modi <[hidden email]> 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.


From a150d99f8e278e9b90240e4b0c460ca974b32aeb Mon Sep 17 00:00:00 2001
From: Noam Postavsky <[hidden email]>
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

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Noam Postavsky <[hidden email]>
> Date: Thu, 12 Oct 2017 23:16:28 -0400
> Cc: [hidden email], [hidden email]
>
> > 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.



Reply | Threaded
Open this post in threaded view
|

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

Noam Postavsky-2
Eli Zaretskii <[hidden email]> 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.




Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Noam Postavsky <[hidden email]>
> Cc: [hidden email],  [hidden email],  [hidden email]
> Date: Fri, 13 Oct 2017 09:15:01 -0400
>
> Eli Zaretskii <[hidden email]> 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.