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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 17549 in the body.
You can then email your comments to 17549 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.4.50; regression: url-insert-file-contents
Date: Thu, 22 May 2014 19:11:31 +0800

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

Message #8 received at 17549 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: 17549 <at> debbugs.gnu.org
Cc: Juanma Barranquero <lekktu <at> gmail.com>
Subject: Re: bug#17549: 24.4.50; regression: url-insert-file-contents
Date: Fri, 30 May 2014 07:24:40 +0800

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

Message #11 received at 17549 <at> debbugs.gnu.org (full text, mbox):

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 17549 <at> debbugs.gnu.org
Subject: Re: bug#17549: 24.4.50; regression: url-insert-file-contents
Date: Fri, 30 May 2014 04:09:44 +0200

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

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

Yes, I'll look into it.

   J

Message #14 received at 17549 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: 17549 <at> debbugs.gnu.org
Subject: Re: bug#17549: 24.4.50; regression: url-insert-file-contents
Date: Tue, 24 Jun 2014 11:24:03 +0800

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

Message #17 received at 17549 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: Juanma Barranquero <lekktu <at> gmail.com>, 17549 <at> debbugs.gnu.org
Subject: Re: bug#17549: 24.4.50; regression: url-insert-file-contents
Date: Tue, 24 Jun 2014 08:49:50 -0400

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

Message #20 received at 17549 <at> debbugs.gnu.org (full text, mbox):

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 17549 <at> debbugs.gnu.org, Leo Liu <sdl.web <at> gmail.com>
Subject: Re: bug#17549: 24.4.50; regression: url-insert-file-contents
Date: Tue, 24 Jun 2014 14:58:05 +0200

[Message part 1 (text/plain, inline)]

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

[Message part 2 (text/html, inline)]

Message #23 received at 17549 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Juanma Barranquero <lekktu <at> gmail.com>, 17549 <at> debbugs.gnu.org
Subject: Re: bug#17549: 24.4.50; regression: url-insert-file-contents
Date: Tue, 24 Jun 2014 21:38:04 +0800

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

Message #26 received at 17549 <at> debbugs.gnu.org (full text, mbox):

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 17549 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#17549: 24.4.50; regression: url-insert-file-contents
Date: Tue, 24 Jun 2014 16:01:35 +0200

[Message part 1 (text/plain, inline)]

On Tue, Jun 24, 2014 at 3:38 PM, Leo Liu <sdl.web <at> 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

[Message part 2 (text/html, inline)]

Message #29 received at 17549 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: 17549 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#17549: 24.4.50; regression: url-insert-file-contents
Date: Wed, 25 Jun 2014 08:58:49 +0800

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)

Message #32 received at 17549 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 17549 <at> debbugs.gnu.org
Subject: Re: bug#17549: 24.4.50; regression: url-insert-file-contents
Date: Thu, 26 Jun 2014 08:19:49 +0800

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)

Message #35 received at 17549 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 17549 <at> debbugs.gnu.org
Subject: Re: bug#17549: 24.4.50; regression: url-insert-file-contents
Date: Wed, 25 Jun 2014 22:38:37 -0400

> +(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

Message #38 received at 17549 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 17549 <at> debbugs.gnu.org
Subject: Re: bug#17549: 24.4.50; regression: url-insert-file-contents
Date: Thu, 26 Jun 2014 12:03:19 +0800

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.