bug#17549: 24.4.50; regression: url-insert-file-contents

bug#17549: 24.4.50; regression: url-insert-file-contents

[Leo Liu]

In using url-http-parse-response we have restricted
url-insert-file-contents to only HTTP. Previously it handles all URL
protocols. For example, if fed file:///url-to-whatever, it throws an
error:

  Debugger entered--Lisp error: (void-variable url-http-end-of-headers)

Leo

bug#17549: 24.4.50; regression: url-insert-file-contents

[Leo Liu]

Hi Juanma,

The regression is introduced by your commit 116865 in emacs-24 and this
causes a core function in my setup to fail. I have temporarily reverted
the change in my local copy. Could you take a look and fix it before the
next pretest?

Thanks,
Leo

bug#17549: 24.4.50; regression: url-insert-file-contents

[Juanma Barranquero]

On Fri, May 30, 2014 at 1:24 AM, Leo Liu <sdl.web@gmail.com> wrote:

> Could you take a look and fix it before the next pretest?

Yes, I'll look into it.

   J

bug#17549: 24.4.50; regression: url-insert-file-contents

[Leo Liu]

On 2014-05-30 04:09 +0200, Juanma Barranquero wrote:
> Yes, I'll look into it.
>
>    J

Ping?

Any objection to moving the http/s code back to package.el?

Leo

bug#17549: 24.4.50; regression: url-insert-file-contents

[Stefan Monnier]

>> Yes, I'll look into it.
> Ping?
> Any objection to moving the http/s code back to package.el?

Does this affect emacs-24 as well?  If so, for emacs-24, it's OK, but
for trunk we want something better.


        Stefan

bug#17549: 24.4.50; regression: url-insert-file-contents

[Juanma Barranquero]

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

AFAIK, we are not yet ready to release emacs-24, so it'd be better to find
a fix than revert.

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

bug#17549: 24.4.50; regression: url-insert-file-contents

[Leo Liu]

On 2014-06-24 08:49 -0400, Stefan Monnier wrote:
> Does this affect emacs-24 as well?  If so, for emacs-24, it's OK, but
> for trunk we want something better.
>
>
>         Stefan

Yes, this is a bug in emacs-24 as well.

On 2014-06-24 14:58 +0200, Juanma Barranquero wrote:
> AFAIK, we are not yet ready to release emacs-24, so it'd be better to find
> a fix than revert.

I am not against a thorough solution but I think leaving this broken for
months can be problematic. Secondly that code moved from package.el
might be a quick dirty hack anyway, a quick look at url-http seems to
suggest it is repeating some work already done by
url-retrieve-synchronously.

Leo

bug#17549: 24.4.50; regression: url-insert-file-contents

[Juanma Barranquero]

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

On Tue, Jun 24, 2014 at 3:38 PM, Leo Liu <sdl.web@gmail.com> wrote:

> I am not against a thorough solution but I think leaving this broken for
> months can be problematic.

Sorry. I keep trying to find time to fix this and a few other bugs, but I'm
very busy right now.

> Secondly that code moved from package.el
> might be a quick dirty hack anyway, a quick look at url-http seems to
> suggest it is repeating some work already done by
> url-retrieve-synchronously.

AFAIR, originally I wanted a small change, just throw an error in a
specific case, and Stefan pushed for a bigger (though, I suppose, cleaner)
fix; there was some back-and-forth and we finally settled on what's
installed. So perhaps you can go back, reread the bug thread and find
something in the proposed solutions that fixes the original problem and
does not cause the one you're reporting.

    J

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

bug#17549: 24.4.50; regression: url-insert-file-contents

[Leo Liu]

On 2014-06-24 16:01 +0200, Juanma Barranquero wrote:
> Sorry. I keep trying to find time to fix this and a few other bugs, but I'm
> very busy right now.

No worries ;)

> AFAIR, originally I wanted a small change, just throw an error in a
> specific case, and Stefan pushed for a bigger (though, I suppose, cleaner)
> fix; there was some back-and-forth and we finally settled on what's
> installed. So perhaps you can go back, reread the bug thread and find
> something in the proposed solutions that fixes the original problem and
> does not cause the one you're reporting.

OK, I read that thread quickly and now understood the decision. I
propose the following solution for now. The patch introduces a compiler
warning on url-http-codes. How best to suppress it?

=== modified file 'lisp/url/url-handlers.el'
--- lisp/url/url-handlers.el	2014-05-12 06:59:30 +0000
+++ lisp/url/url-handlers.el	2014-06-25 00:41:33 +0000
@@ -33,7 +33,6 @@
 (autoload 'url-expand-file-name "url-expand" "Convert url to a fully specified url, and canonicalize it.")
 (autoload 'mm-dissect-buffer "mm-decode" "Dissect the current buffer and return a list of MIME handles.")
 (autoload 'url-scheme-get-property "url-methods" "Get property of a URL SCHEME.")
-(autoload 'url-http-parse-response "url-http" "Parse just the response code.")
 
 ;; Always used after mm-dissect-buffer and defined in the same file.
 (declare-function mm-save-part-to-file "mm-decode" (handle file))
@@ -313,12 +312,12 @@
   (let ((buffer (url-retrieve-synchronously url)))
     (unless buffer (signal 'file-error (list url "No Data")))
     (with-current-buffer buffer
-      (let ((response (url-http-parse-response)))
-        (if (and (>= response 200) (< response 300))
-            (goto-char (point-min))
-          (let ((desc (buffer-substring-no-properties (1+ (point))
-                                                      (line-end-position))))
+      (when (bound-and-true-p url-http-response-status)
+        (unless (and (>= url-http-response-status 200)
+                     (< url-http-response-status 300))
+          (let ((desc (nth 2 (assq url-http-response-status url-http-codes))))
             (kill-buffer buffer)
+            ;; Signal file-error per http://debbugs.gnu.org/16733.
             (signal 'file-error (list url desc))))))
     (if visit (setq buffer-file-name url))
     (save-excursion
@@ -333,6 +332,7 @@
           ;; usual heuristic/rules that we apply to files.
           (decode-coding-inserted-region start (point) url visit beg end replace))
         (list url (car size-and-charset))))))
+
 (put 'insert-file-contents 'url-file-handlers 'url-insert-file-contents)
 
 (defun url-file-name-completion (url directory &optional predicate)

=== modified file 'lisp/url/url-http.el'
--- lisp/url/url-http.el	2014-03-29 00:55:44 +0000
+++ lisp/url/url-http.el	2014-06-25 00:23:24 +0000
@@ -48,7 +48,6 @@
 (defvar url-http-response-version)
 (defvar url-http-target-url)
 (defvar url-http-transfer-encoding)
-(defvar url-http-end-of-headers)
 (defvar url-show-status)
 
 (require 'url-gw)

bug#17549: 24.4.50; regression: url-insert-file-contents

[Leo Liu]

On 2014-06-25 08:58 +0800, Leo Liu wrote:
> The patch introduces a compiler warning on url-http-codes. How best to
> suppress it?

I'd be very happy if this bug can be fixed for the next release. Any
comments on the proposed patch? Thanks, Leo

=== modified file 'lisp/url/url-handlers.el'
--- lisp/url/url-handlers.el	2014-05-12 06:59:30 +0000
+++ lisp/url/url-handlers.el	2014-06-26 00:18:44 +0000
@@ -33,7 +33,6 @@
 (autoload 'url-expand-file-name "url-expand" "Convert url to a fully specified url, and canonicalize it.")
 (autoload 'mm-dissect-buffer "mm-decode" "Dissect the current buffer and return a list of MIME handles.")
 (autoload 'url-scheme-get-property "url-methods" "Get property of a URL SCHEME.")
-(autoload 'url-http-parse-response "url-http" "Parse just the response code.")
 
 ;; Always used after mm-dissect-buffer and defined in the same file.
 (declare-function mm-save-part-to-file "mm-decode" (handle file))
@@ -308,17 +307,19 @@
       (insert data))
     (list (length data) charset)))
 
+(defconst url-http-codes)
+
 ;;;###autoload
 (defun url-insert-file-contents (url &optional visit beg end replace)
   (let ((buffer (url-retrieve-synchronously url)))
     (unless buffer (signal 'file-error (list url "No Data")))
     (with-current-buffer buffer
-      (let ((response (url-http-parse-response)))
-        (if (and (>= response 200) (< response 300))
-            (goto-char (point-min))
-          (let ((desc (buffer-substring-no-properties (1+ (point))
-                                                      (line-end-position))))
+      (when (bound-and-true-p url-http-response-status)
+        (unless (and (>= url-http-response-status 200)
+                     (< url-http-response-status 300))
+          (let ((desc (nth 2 (assq url-http-response-status url-http-codes))))
             (kill-buffer buffer)
+            ;; Signal file-error per http://debbugs.gnu.org/16733.
             (signal 'file-error (list url desc))))))
     (if visit (setq buffer-file-name url))
     (save-excursion
@@ -333,6 +334,7 @@
           ;; usual heuristic/rules that we apply to files.
           (decode-coding-inserted-region start (point) url visit beg end replace))
         (list url (car size-and-charset))))))
+
 (put 'insert-file-contents 'url-file-handlers 'url-insert-file-contents)
 
 (defun url-file-name-completion (url directory &optional predicate)

=== modified file 'lisp/url/url-http.el'
--- lisp/url/url-http.el	2014-03-29 00:55:44 +0000
+++ lisp/url/url-http.el	2014-06-25 00:23:24 +0000
@@ -48,7 +48,6 @@
 (defvar url-http-response-version)
 (defvar url-http-target-url)
 (defvar url-http-transfer-encoding)
-(defvar url-http-end-of-headers)
 (defvar url-show-status)
 
 (require 'url-gw)

bug#17549: 24.4.50; regression: url-insert-file-contents

[Stefan Monnier]

> +(defconst url-http-codes)

This should use `defvar'.

> -      (let ((response (url-http-parse-response)))
> -        (if (and (>= response 200) (< response 300))
> -            (goto-char (point-min))
> -          (let ((desc (buffer-substring-no-properties (1+ (point))
> -                                                      (line-end-position))))
> +      (when (bound-and-true-p url-http-response-status)
> +        (unless (and (>= url-http-response-status 200)
> +                     (< url-http-response-status 300))
> +          (let ((desc (nth 2 (assq url-http-response-status url-http-codes))))

IIUC the above just adds a "(when (bound-and-true-p
url-http-response-status)" wrapper around the existing code, right?
If so, it looks like a safe enough fix to install it in emacs-24.

This still doesn't look like The Right Way to do things in URL.
I think The Right Way would be for the url-http code to set some
backend-agnostic properties which url-handlers.el can then use.


        Stefan

bug#17549: 24.4.50; regression: url-insert-file-contents

[Leo Liu]

Fixed in 24.4

On 2014-06-25 22:38 -0400, Stefan Monnier wrote:
> IIUC the above just adds a "(when (bound-and-true-p
> url-http-response-status)" wrapper around the existing code, right?
> If so, it looks like a safe enough fix to install it in emacs-24.

Yes, it should be safe enough. So bound-and-true-p make sure it is
http/s. Secondly we don't re-do url-http-parse-response and rely on its
point end at before the status-text.

> This still doesn't look like The Right Way to do things in URL.
> I think The Right Way would be for the url-http code to set some
> backend-agnostic properties which url-handlers.el can then use.

I agree. This bit of code is http specific and should be in the backend
instead. I'll add a note for now. BTW, it might not be easy to invent
similar semantics for other protocols though.

Thanks,
Leo