* [PATCH] Avoid crash in `org-file-contents' in case of network failure
@ 2023-02-18 17:50 Damien Cassou
2023-02-22 9:30 ` Ihor Radchenko
0 siblings, 1 reply; 8+ messages in thread
From: Damien Cassou @ 2023-02-18 17:50 UTC (permalink / raw)
To: emacs-orgmode
[-- Attachment #1: Type: text/plain, Size: 287 bytes --]
* lisp/org.el (org-file-contents): Wrap the
`url-retrieve-synchronously' call into a `condition-case' block to
avoid throwing an error when NOERROR is non-nil.
--
Damien Cassou
"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Avoid-crash-in-org-file-contents-in-case-of-network-.patch --]
[-- Type: text/x-patch, Size: 2790 bytes --]
From b13b58711405afd1a065c371251ec0ada35d86dc Mon Sep 17 00:00:00 2001
From: Damien Cassou <damien@cassou.me>
Date: Sat, 18 Feb 2023 12:16:48 +0100
Subject: [PATCH] Avoid crash in `org-file-contents' in case of network failure
* lisp/org.el (org-file-contents): Wrap the
`url-retrieve-synchronously' call into a `condition-case' block to
avoid throwing an error when NOERROR is non-nil.
---
lisp/org.el | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/lisp/org.el b/lisp/org.el
index cc2c09e3a..e777f21f8 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -4561,21 +4561,25 @@ (defun org-file-contents (file &optional noerror nocache)
(cache)
(is-url
(if (org--should-fetch-remote-resource-p file)
- (with-current-buffer (url-retrieve-synchronously file)
- (goto-char (point-min))
- ;; Move point to after the url-retrieve header.
- (search-forward "\n\n" nil :move)
- ;; Search for the success code only in the url-retrieve header.
- (if (save-excursion
- (re-search-backward "HTTP.*\\s-+200\\s-OK" nil :noerror))
- ;; Update the cache `org--file-cache' and return contents.
- (puthash file
- (buffer-substring-no-properties (point) (point-max))
- org--file-cache)
- (funcall (if noerror #'message #'user-error)
- "Unable to fetch file from %S"
- file)
- nil))
+ (condition-case error
+ (with-current-buffer (url-retrieve-synchronously file)
+ (goto-char (point-min))
+ ;; Move point to after the url-retrieve header.
+ (search-forward "\n\n" nil :move)
+ ;; Search for the success code only in the url-retrieve header.
+ (if (save-excursion
+ (re-search-backward "HTTP.*\\s-+200\\s-OK" nil :noerror))
+ ;; Update the cache `org--file-cache' and return contents.
+ (puthash file
+ (buffer-substring-no-properties (point) (point-max))
+ org--file-cache)
+ (funcall (if noerror #'message #'user-error)
+ "Unable to fetch file from %S"
+ file)
+ nil))
+ (error (if noerror
+ (message "%s %S" (car error) (cdr error))
+ (signal (car error) (cdr error)))))
(funcall (if noerror #'message #'user-error)
"The remote resource %S is considered unsafe, and will not be downloaded."
file)))
--
2.38.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] Avoid crash in `org-file-contents' in case of network failure
2023-02-18 17:50 [PATCH] Avoid crash in `org-file-contents' in case of network failure Damien Cassou
@ 2023-02-22 9:30 ` Ihor Radchenko
2023-02-24 7:08 ` Damien Cassou
0 siblings, 1 reply; 8+ messages in thread
From: Ihor Radchenko @ 2023-02-22 9:30 UTC (permalink / raw)
To: Damien Cassou; +Cc: emacs-orgmode
Damien Cassou <damien@cassou.me> writes:
> * lisp/org.el (org-file-contents): Wrap the
> `url-retrieve-synchronously' call into a `condition-case' block to
> avoid throwing an error when NOERROR is non-nil.
Thanks for the patch!
Few comments.
> Subject: [PATCH] Avoid crash in `org-file-contents' in case of network failure
>
> * lisp/org.el (org-file-contents): Wrap the
> `url-retrieve-synchronously' call into a `condition-case' block to
> avoid throwing an error when NOERROR is non-nil.
According to our records in https://orgmode.org/worg/contributors.html,
you haven't signed FSF copyright agreement. May you add a TINYCHANGE
cookie to the commit message as required by
https://orgmode.org/worg/org-contribute.html#first-patch
> + (error (if noerror
> + (message "%s %S" (car error) (cdr error))
> + (signal (car error) (cdr error)))))
I think that it will be a good idea to indicate in the `message' where
the error is coming from. The error intercepted by this `condition-case'
form will likely be a network error, and it may be unclear to the user
which function is failing to make the connection.
--
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Avoid crash in `org-file-contents' in case of network failure
2023-02-22 9:30 ` Ihor Radchenko
@ 2023-02-24 7:08 ` Damien Cassou
2023-02-24 10:17 ` Ihor Radchenko
0 siblings, 1 reply; 8+ messages in thread
From: Damien Cassou @ 2023-02-24 7:08 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: emacs-orgmode
Hi Ihor,
thank you for your review.
Ihor Radchenko <yantar92@posteo.net> writes:
> According to our records in https://orgmode.org/worg/contributors.html,
> you haven't signed FSF copyright agreement. May you add a TINYCHANGE
> cookie to the commit message as required by
> https://orgmode.org/worg/org-contribute.html#first-patch
I've signed the FSF copyright agreement in the context of my
contributions to Emacs. Do I need to do any more paperwork?
I've added TINYCHANGE to the patch anyway.
>> + (error (if noerror
>> + (message "%s %S" (car error) (cdr error))
>> + (signal (car error) (cdr error)))))
>
> I think that it will be a good idea to indicate in the `message' where
> the error is coming from. The error intercepted by this `condition-case'
> form will likely be a network error, and it may be unclear to the user
> which function is failing to make the connection.
I tried to make that a little bit clearer:
(message "Org could't download \"%s\": %s %S" file (car error) (cdr
error))
Do you want me to explicitly refer to the current function name?
--
Damien Cassou
"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Avoid crash in `org-file-contents' in case of network failure
2023-02-24 7:08 ` Damien Cassou
@ 2023-02-24 10:17 ` Ihor Radchenko
2023-02-24 10:24 ` Damien Cassou
2023-02-28 9:22 ` Bastien Guerry
0 siblings, 2 replies; 8+ messages in thread
From: Ihor Radchenko @ 2023-02-24 10:17 UTC (permalink / raw)
To: Damien Cassou, Bastien; +Cc: emacs-orgmode
Damien Cassou <damien@cassou.me> writes:
> I've signed the FSF copyright agreement in the context of my
> contributions to Emacs. Do I need to do any more paperwork?
No, you don't.
Bastien, could you please check FSF records?
>> I think that it will be a good idea to indicate in the `message' where
>> the error is coming from. The error intercepted by this `condition-case'
>> form will likely be a network error, and it may be unclear to the user
>> which function is failing to make the connection.
>
> I tried to make that a little bit clearer:
>
> (message "Org could't download \"%s\": %s %S" file (car error) (cdr
> error))
>
> Do you want me to explicitly refer to the current function name?
Should be good enough.
I think you forgot to attach the updated patch :)
--
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Avoid crash in `org-file-contents' in case of network failure
2023-02-24 10:17 ` Ihor Radchenko
@ 2023-02-24 10:24 ` Damien Cassou
2023-02-26 11:44 ` Ihor Radchenko
2023-02-28 9:22 ` Bastien Guerry
1 sibling, 1 reply; 8+ messages in thread
From: Damien Cassou @ 2023-02-24 10:24 UTC (permalink / raw)
To: Ihor Radchenko, Bastien; +Cc: emacs-orgmode
[-- Attachment #1: Type: text/plain, Size: 247 bytes --]
Ihor Radchenko <yantar92@posteo.net> writes:
> I think you forgot to attach the updated patch :)
typical. I'm so sorry.
--
Damien Cassou
"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org.el-Avoid-crash-in-org-file-contents-in-case-of-n.patch --]
[-- Type: text/x-patch, Size: 2845 bytes --]
From c116e27a84972f9e12a869f2b4983bb84ee50b15 Mon Sep 17 00:00:00 2001
From: Damien Cassou <damien@cassou.me>
Date: Sat, 18 Feb 2023 12:16:48 +0100
Subject: [PATCH] org.el: Avoid crash in `org-file-contents' in case of network
failure
* lisp/org.el (org-file-contents): Wrap the
`url-retrieve-synchronously' call into a `condition-case' block to
avoid throwing an error when NOERROR is non-nil.
TINYCHANGE
---
lisp/org.el | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/lisp/org.el b/lisp/org.el
index b0a6d8ef3..a9fd76734 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -4561,21 +4561,25 @@ (defun org-file-contents (file &optional noerror nocache)
(cache)
(is-url
(if (org--should-fetch-remote-resource-p file)
- (with-current-buffer (url-retrieve-synchronously file)
- (goto-char (point-min))
- ;; Move point to after the url-retrieve header.
- (search-forward "\n\n" nil :move)
- ;; Search for the success code only in the url-retrieve header.
- (if (save-excursion
- (re-search-backward "HTTP.*\\s-+200\\s-OK" nil :noerror))
- ;; Update the cache `org--file-cache' and return contents.
- (puthash file
- (buffer-substring-no-properties (point) (point-max))
- org--file-cache)
- (funcall (if noerror #'message #'user-error)
- "Unable to fetch file from %S"
- file)
- nil))
+ (condition-case error
+ (with-current-buffer (url-retrieve-synchronously file)
+ (goto-char (point-min))
+ ;; Move point to after the url-retrieve header.
+ (search-forward "\n\n" nil :move)
+ ;; Search for the success code only in the url-retrieve header.
+ (if (save-excursion
+ (re-search-backward "HTTP.*\\s-+200\\s-OK" nil :noerror))
+ ;; Update the cache `org--file-cache' and return contents.
+ (puthash file
+ (buffer-substring-no-properties (point) (point-max))
+ org--file-cache)
+ (funcall (if noerror #'message #'user-error)
+ "Unable to fetch file from %S"
+ file)
+ nil))
+ (error (if noerror
+ (message "Org could't download \"%s\": %s %S" file (car error) (cdr error))
+ (signal (car error) (cdr error)))))
(funcall (if noerror #'message #'user-error)
"The remote resource %S is considered unsafe, and will not be downloaded."
file)))
--
2.38.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] Avoid crash in `org-file-contents' in case of network failure
2023-02-24 10:17 ` Ihor Radchenko
2023-02-24 10:24 ` Damien Cassou
@ 2023-02-28 9:22 ` Bastien Guerry
2023-03-01 10:04 ` Ihor Radchenko
1 sibling, 1 reply; 8+ messages in thread
From: Bastien Guerry @ 2023-02-28 9:22 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: Damien Cassou, emacs-orgmode
Ihor Radchenko <yantar92@posteo.net> writes:
> Damien Cassou <damien@cassou.me> writes:
>
>> I've signed the FSF copyright agreement in the context of my
>> contributions to Emacs. Do I need to do any more paperwork?
>
> No, you don't.
> Bastien, could you please check FSF records?
I confirm Damien's FSF record is in order.
--
Bastien Guerry
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-03-01 10:08 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-18 17:50 [PATCH] Avoid crash in `org-file-contents' in case of network failure Damien Cassou
2023-02-22 9:30 ` Ihor Radchenko
2023-02-24 7:08 ` Damien Cassou
2023-02-24 10:17 ` Ihor Radchenko
2023-02-24 10:24 ` Damien Cassou
2023-02-26 11:44 ` Ihor Radchenko
2023-02-28 9:22 ` Bastien Guerry
2023-03-01 10:04 ` Ihor Radchenko
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).