Re: [PATCH] Avoid crash in `org-file-contents' in case of network failure

[Ihor Radchenko]

Bastien Guerry  writes:

>> Bastien, could you please check FSF records?
>
> I confirm Damien's FSF record is in order.

Thanks!
Added to contributor list: https://git.sr.ht/~bzg/worg/commit/ada245e4

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: [PATCH] Avoid crash in `org-file-contents' in case of network failure

[Bastien Guerry]

Ihor Radchenko  writes:

> Damien Cassou  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

Re: [PATCH] Avoid crash in `org-file-contents' in case of network failure

[Ihor Radchenko]

Damien Cassou  writes:

> Subject: [PATCH] org.el: Avoid crash in `org-file-contents' in case of network
>  failure

Applied, onto bugfix.
Thanks for your contribution!
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=f9aeba5dd

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: [PATCH] Avoid crash in `org-file-contents' in case of network failure

[Damien Cassou]

Ihor Radchenko  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
>From c116e27a84972f9e12a869f2b4983bb84ee50b15 Mon Sep 17 00:00:00 2001
From: Damien Cassou 
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  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

Re: [PATCH] Avoid crash in `org-file-contents' in case of network failure

[Ihor Radchenko]

Damien Cassou  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 .
Support Org development at ,
or support my work at 

Re: [PATCH] Avoid crash in `org-file-contents' in case of network failure

[Damien Cassou]

Hi Ihor,

thank you for your review.

Ihor Radchenko  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

Re: [PATCH] Avoid crash in `org-file-contents' in case of network failure

[Ihor Radchenko]

Damien Cassou  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 .
Support Org development at ,
or support my work at 

[PATCH] Avoid crash in `org-file-contents' in case of network failure

[Damien Cassou]

* 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
>From b13b58711405afd1a065c371251ec0ada35d86dc Mon Sep 17 00:00:00 2001
From: Damien Cassou 
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  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