GNU bug report logs - #16733
messed up unicode chars in package description

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 16733 in the body.
You can then email your comments to 16733 AT debbugs.gnu.org in the normal way.

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

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

From: Glenn Morris <rgm <at> gnu.org>
To: submit <at> debbugs.gnu.org
Subject: messed up unicode chars in package description
Date: Wed, 12 Feb 2014 20:47:36 -0500

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

Package: emacs
Version: 24.3

(Also in current trunk.)

emacs -Q
M-x list-packages RET

Select "ascii-art-to-unicode"

Scroll down the description past the part that says "M-x aa2u RET".
The buffer is completely messed up at this point, showing raw
characters instead of the unicode box that you see if you visit the raw
source file for the package. See attached image.

[emacs.png (image/png, attachment)]

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 19 Mar 2014 02:34:58 +0100

On Thu, Feb 13, 2014 at 2:47 AM, Glenn Morris <rgm <at> gnu.org> wrote:

> Scroll down the description past the part that says "M-x aa2u RET".
> The buffer is completely messed up at this point, showing raw
> characters instead of the unicode box that you see if you visit the raw
> source file for the package. See attached image.

This seems enough.

=== modified file 'lisp/emacs-lisp/package.el'
--- lisp/emacs-lisp/package.el  2014-03-14 20:55:40 +0000
+++ lisp/emacs-lisp/package.el  2014-03-19 01:31:28 +0000
@@ -1528,7 +1528,8 @@
                        (let ((version-control 'never)
                              (require-final-newline t))
                          (save-buffer))
-                       (setq readme-string (buffer-string))
+                       (setq readme-string (decode-coding-string
+                                            (buffer-string) 'prefer-utf-8 t))
                        t))
                 (error nil))
               (insert readme-string))

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

From: Glenn Morris <rgm <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 19 Mar 2014 02:26:59 -0400

Juanma Barranquero wrote:

> On Thu, Feb 13, 2014 at 2:47 AM, Glenn Morris <rgm <at> gnu.org> wrote:
>
>> Scroll down the description past the part that says "M-x aa2u RET".
>> The buffer is completely messed up at this point, showing raw
>> characters instead of the unicode box that you see if you visit the raw
>> source file for the package. See attached image.
>
> This seems enough.

OK. (Presumably it could still go wrong if it ever finds a non-ascii,
non-utf-8 package, but those are probably very rare.)

> === modified file 'lisp/emacs-lisp/package.el'
> --- lisp/emacs-lisp/package.el  2014-03-14 20:55:40 +0000
> +++ lisp/emacs-lisp/package.el  2014-03-19 01:31:28 +0000
> @@ -1528,7 +1528,8 @@
>                         (let ((version-control 'never)
>                               (require-final-newline t))
>                           (save-buffer))
> -                       (setq readme-string (buffer-string))
> +                       (setq readme-string (decode-coding-string
> +                                            (buffer-string) 'prefer-utf-8 t))
>                         t))
>                  (error nil))
>                (insert readme-string))

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 19 Mar 2014 11:19:19 +0100

On Wed, Mar 19, 2014 at 7:26 AM, Glenn Morris <rgm <at> gnu.org> wrote:

> (Presumably it could still go wrong if it ever finds a non-ascii,
> non-utf-8 package

True. What about this one?

=== modified file 'lisp/emacs-lisp/package.el'
--- lisp/emacs-lisp/package.el  2014-03-14 20:55:40 +0000
+++ lisp/emacs-lisp/package.el  2014-03-19 10:17:45 +0000
@@ -1528,7 +1528,9 @@
                        (let ((version-control 'never)
                              (require-final-newline t))
                          (save-buffer))
-                       (setq readme-string (buffer-string))
+                       (let* ((text (buffer-string))
+                              (coding (detect-coding-string text t)))
+                         (setq readme-string (decode-coding-string
text coding t)))
                        t))
                 (error nil))
               (insert readme-string))

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

From: Glenn Morris <rgm <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 19 Mar 2014 11:53:08 -0400

If it works, sounds fine.
(I wonder why doesn't Emacs simply DTRT though.)

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 19 Mar 2014 17:08:47 +0100

On Wed, Mar 19, 2014 at 4:53 PM, Glenn Morris <rgm <at> gnu.org> wrote:

> If it works, sounds fine.

Yes, it does.

> (I wonder why doesn't Emacs simply DTRT though.)

Apparently, url-retrieve-synchronously returns a unibyte buffer.

(multibyte-string-p (with-current-buffer
                        (url-retrieve-synchronously "http://es.gnu.org")
                      (buffer-string)))

=> nil

and yet the homepage of GNU España contains non-ASCII chars and its
metadata says it is encoded in utf-8:

C:\> lwp-request -m HEAD http://es.gnu.org
200 OK
[...]
Content-Language: es
Content-Type: text/html; charset=UTF-8
[...]

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

From: Glenn Morris <rgm <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 19 Mar 2014 12:16:42 -0400

Juanma Barranquero wrote:

> Apparently, url-retrieve-synchronously returns a unibyte buffer.
>
> (multibyte-string-p (with-current-buffer
>                         (url-retrieve-synchronously "http://es.gnu.org")
>                       (buffer-string)))
>
> => nil
>
> and yet the homepage of GNU España contains non-ASCII chars and its

GNU what?! ;)

> metadata says it is encoded in utf-8:
>
> C:\> lwp-request -m HEAD http://es.gnu.org
> 200 OK
> [...]
> Content-Language: es
> Content-Type: text/html; charset=UTF-8

I wonder if url.el has/should have a facility for fetching urls and
automatically decoding them according to the charset they claim to be
in?

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 16733-done <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 19 Mar 2014 17:24:18 +0100

On Wed, Mar 19, 2014 at 5:16 PM, Glenn Morris <rgm <at> gnu.org> wrote:

> GNU what?! ;)

Oh, you know, the webpage of these barbarian free-software advocates
who speak the world's second language in number of native speakers
(after Mandarin) ;-)

> I wonder if url.el has/should have a facility for fetching urls and
> automatically decoding them according to the charset they claim to be
> in?

Perhaps it has, but url/*.el has 32 files; I'm not going to delve into
it right now ;-)  If it has the facility and a better fix than mine is
possible, fine (my fix isn't complex, removing it is trivial). And if
not, certainly adding that feature is a post-freeze endeavour.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: rgm <at> gnu.org, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 19 Mar 2014 19:08:25 +0200

> From: Juanma Barranquero <lekktu <at> gmail.com>
> Date: Wed, 19 Mar 2014 17:08:47 +0100
> Cc: 16733 <at> debbugs.gnu.org
> 
> > (I wonder why doesn't Emacs simply DTRT though.)
> 
> Apparently, url-retrieve-synchronously returns a unibyte buffer.

Right, so we need to decode the buffer before displaying it.

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Glenn Morris <rgm <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 19 Mar 2014 18:09:23 +0100

On Wed, Mar 19, 2014 at 6:08 PM, Eli Zaretskii <eliz <at> gnu.org> wrote:

> Right, so we need to decode the buffer before displaying it.

That's what my fix does.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: lekktu <at> gmail.com, 16733 <at> debbugs.gnu.org, rgm <at> gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 19 Mar 2014 19:11:36 +0200

> From: Juanma Barranquero <lekktu <at> gmail.com>
> Date: Wed, 19 Mar 2014 17:24:18 +0100
> Cc: 16733-done <at> debbugs.gnu.org
> 
> > I wonder if url.el has/should have a facility for fetching urls and
> > automatically decoding them according to the charset they claim to be
> > in?
> 
> Perhaps it has, but url/*.el has 32 files; I'm not going to delve into
> it right now ;-)

I think you want url-insert from url-handlers.el.

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Glenn Morris <rgm <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 19 Mar 2014 18:43:46 +0100

On Wed, Mar 19, 2014 at 6:11 PM, Eli Zaretskii <eliz <at> gnu.org> wrote:

> I think you want url-insert from url-handlers.el.

Thanks.

So the following patch is a better fix. I'm OK with installing it,
though it is a bit more intrusive that the previous one.

   J


2014-03-19  Juanma Barranquero  <lekktu <at> gmail.com>

        * emacs-lisp/package.el: Fix bug#16733.
        (url-handlers): Require.
        (package--with-work-buffer): When LOCATION is a URL, use url-insert to
        properly decode the buffer.  Suggested by Eli Zaretskii <eliz <at> gnu.org>.


=== modified file 'lisp/emacs-lisp/package.el'
--- lisp/emacs-lisp/package.el  2014-03-19 16:14:26 +0000
+++ lisp/emacs-lisp/package.el  2014-03-19 17:35:44 +0000
@@ -166,6 +166,7 @@
 (eval-when-compile (require 'cl-lib))

 (require 'tabulated-list)
+(require 'url-handlers)

 (defgroup package nil
   "Manager for Emacs Lisp packages."
@@ -770,15 +771,13 @@
 and evaluates BODY while that buffer is current.  This work
 buffer is killed afterwards.  Return the last value in BODY."
   (declare (indent 2) (debug t))
-  `(let* ((http (string-match "\\`https?:" ,location))
-         (buffer
-          (if http
-              (url-retrieve-synchronously (concat ,location ,file))
-            (generate-new-buffer "*package work buffer*"))))
+  `(let ((buffer (generate-new-buffer "*package work buffer*")))
      (prog1
         (with-current-buffer buffer
-          (if http
-              (progn (package-handle-response)
+          (if (string-match-p "\\`https?:" ,location)
+              (progn (url-insert (url-retrieve-synchronously
+                                  (concat ,location ,file)))
+                     (package-handle-response)
                      (re-search-forward "^$" nil 'move)
                      (forward-char)
                      (delete-region (point-min) (point)))
@@ -1531,8 +1530,7 @@
                        (setq readme-string (buffer-string))
                        t))
                 (error nil))
-              (let ((coding (detect-coding-string readme-string t)))
-                (insert (decode-coding-string readme-string coding t))))
+              (insert readme-string))
              ((file-readable-p readme)
               (insert-file-contents readme)
               (goto-char (point-max))))))))

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

From: Glenn Morris <rgm <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 19 Mar 2014 14:05:29 -0400

Juanma Barranquero wrote:

> +(require 'url-handlers)

I'd favour autoloading url-insert rather than requiring.
(I see url-insert-file-contents has an autoload cookie, so maybe that is
supposed to be the relevant entry point.)

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 19 Mar 2014 19:33:26 +0100

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

On Mar 19, 2014 7:05 PM, "Glenn Morris" <rgm <at> gnu.org> wrote:

> I'd favour autoloading url-insert rather than requiring.
> (I see url-insert-file-contents has an autoload cookie, so maybe that is
> supposed to be the relevant entry point.)

I'll look into it (and also fix my patch so it kills the URL buffer).

      J

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

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

From: Stefan <monnier <at> iro.umontreal.ca>
To: Glenn Morris <rgm <at> gnu.org>
Cc: Juanma Barranquero <lekktu <at> gmail.com>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 19 Mar 2014 15:00:07 -0400

> I wonder if url.el has/should have a facility for fetching urls and
> automatically decoding them according to the charset they claim to
> be in?

IIRC there's something to that effect already somewhere in url/*.el, but
maybe it's not in url-retrieve-synchronously, or it fails in this case
for some reason.


        Stefan

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

From: Glenn Morris <rgm <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 19 Mar 2014 15:42:27 -0400

Glenn Morris wrote:

>> and yet the homepage of GNU España contains non-ASCII chars and its
>
> GNU what?! ;)
[...]
>> C:\> lwp-request -m HEAD http://es.gnu.org

I'm sorry! I did not read that properly.
I thought you'd made a freudian typo for "GNU elpa", but now I see you
were talking about a different URL. No offence intended.

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 19 Mar 2014 21:44:47 +0100

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

On Mar 19, 2014 8:42 PM, "Glenn Morris" <rgm <at> gnu.org> wrote:

> I'm sorry! I did not read that properly.
> I thought you'd made a freudian typo for "GNU elpa", but now I see you
> were talking about a different URL. No offence intended.

Don't worry, no offense at all. I thought you were joking.

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

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Thu, 20 Mar 2014 06:12:24 +0100

On Wed, Mar 19, 2014 at 7:05 PM, Glenn Morris <rgm <at> gnu.org> wrote:

> I'd favour autoloading url-insert rather than requiring.
> (I see url-insert-file-contents has an autoload cookie, so maybe that is
> supposed to be the relevant entry point.)

All in all, trying to use url-insert(-file-contents) has been less
than straightforward.

- url-insert-file-contents can be used, and in fact it simplifies the
code in package--with-work-buffer quite a bit. A nicety is that, if
the HTTP response headers to not set an explicit coding, it guesses.
The catch is that it does not detect missing URLs, i.e., if you point
it to http://www.gnu.org/does-not-exist it will happily, and
successfully, return the 404 Page Not Found error page. (That, BTW,
makes package-handle-response irrelevant, because it is no longer used
anywhere).

- url-insert works on the buffer returned by
url-retrieve-synchronously, so package-handle-response can be used to
detect errors, i.e. response codes not in [200, 300). The problem is
that if the response header does not include a coding, url-insert
doesn't know what to do. Ironically, that's exactly the case with
Glenn's original report:

C:\> lwp-request -m HEAD
http://elpa.gnu.org/packages/ascii-art-to-unicode-readme.txt
200 OK
[...]
Content-Length: 1255
Content-Type: text/plain
[...]

So, at this point, I see the following alternatives:

1) Leave it as it is now, with
detect-coding-string/decode-coding-string (or perhaps
decode-coding-(inserted-)region).

2) Use url-insert-file and accept that it will return the wrong
contents for missing URLs; that could be acceptable, depending on how
consistent is the data returned by ELPA and other repositories (this
is my preferred fix, BTW).

3) Copy all or some of the logic of url-insert-file and create a
package-* version which checks for errors with package-handle-response
(Ugly duplication of code).

4) Fix url-insert-file so it can (optionally) check for errors (after
the freeze).



The patch for 3), as an example:


=== modified file 'lisp/emacs-lisp/package.el'
--- lisp/emacs-lisp/package.el  2014-03-19 16:14:26 +0000
+++ lisp/emacs-lisp/package.el  2014-03-20 05:09:52 +0000
@@ -770,38 +770,16 @@
 and evaluates BODY while that buffer is current.  This work
 buffer is killed afterwards.  Return the last value in BODY."
   (declare (indent 2) (debug t))
-  `(let* ((http (string-match "\\`https?:" ,location))
-         (buffer
-          (if http
-              (url-retrieve-synchronously (concat ,location ,file))
-            (generate-new-buffer "*package work buffer*"))))
-     (prog1
-        (with-current-buffer buffer
-          (if http
-              (progn (package-handle-response)
-                     (re-search-forward "^$" nil 'move)
-                     (forward-char)
-                     (delete-region (point-min) (point)))
-            (unless (file-name-absolute-p ,location)
-              (error "Archive location %s is not an absolute file name"
-                     ,location))
-            (insert-file-contents (expand-file-name ,file ,location)))
-          ,@body)
-       (kill-buffer buffer))))
-
-(defun package-handle-response ()
-  "Handle the response from a `url-retrieve-synchronously' call.
-Parse the HTTP response and throw if an error occurred.
-The url package seems to require extra processing for this.
-This should be called in a `save-excursion', in the download buffer.
-It will move point to somewhere in the headers."
-  ;; We assume HTTP here.
-  (require 'url-http)
-  (let ((response (url-http-parse-response)))
-    (when (or (< response 200) (>= response 300))
-      (error "Error downloading %s:%s"
-            (url-recreate-url url-http-target-url)
-            (buffer-substring-no-properties (point) (line-end-position))))))
+  `(with-temp-buffer
+     (if (string-match-p "\\`https?:" ,location)
+        (progn
+          (url-insert-file-contents (concat ,location ,file))
+          (goto-char (point-min)))
+       (unless (file-name-absolute-p ,location)
+        (error "Archive location %s is not an absolute file name"
+               ,location))
+       (insert-file-contents (expand-file-name ,file ,location)))
+     ,@body))

 (defun package--archive-file-exists-p (location file)
   (let ((http (string-match "\\`https?:" location)))
@@ -1272,7 +1250,7 @@
                     (car archive)))))
       ;; Read the retrieved buffer to make sure it is valid (e.g. it
       ;; may fetch a URL redirect page).
-      (when (listp (read buffer))
+      (when (listp (read (current-buffer)))
        (make-directory dir t)
        (setq buffer-file-name (expand-file-name file dir))
        (let ((version-control 'never)
@@ -1531,8 +1509,7 @@
                        (setq readme-string (buffer-string))
                        t))
                 (error nil))
-              (let ((coding (detect-coding-string readme-string t)))
-                (insert (decode-coding-string readme-string coding t))))
+              (insert readme-string))
              ((file-readable-p readme)
               (insert-file-contents readme)
               (goto-char (point-max))))))))

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

From: Glenn Morris <rgm <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Thu, 20 Mar 2014 12:02:25 -0400

Juanma Barranquero wrote:

> that if the response header does not include a coding, url-insert
> doesn't know what to do. Ironically, that's exactly the case with
> Glenn's original report:
>
> C:\> lwp-request -m HEAD
> http://elpa.gnu.org/packages/ascii-art-to-unicode-readme.txt
> 200 OK
> [...]
> Content-Length: 1255
> Content-Type: text/plain
> [...]

IMO this could be a bug in whatever script generates elpa webpages.
Seems like it should specify a charset, based on coding:, if present.

> So, at this point, I see the following alternatives:
>
> 1) Leave it as it is now, with
> detect-coding-string/decode-coding-string (or perhaps
> decode-coding-(inserted-)region).

Seems fine to me for 24.4. It's a minor issue.
Maybe reopen this as a reminder to revisit it afterwards (or new report).

(BTW, I'm surprised if url.el does not have a way to handle 404s.)

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: rgm <at> gnu.org, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Thu, 20 Mar 2014 18:21:45 +0200

> From: Juanma Barranquero <lekktu <at> gmail.com>
> Date: Thu, 20 Mar 2014 06:12:24 +0100
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
> 
> 1) Leave it as it is now, with
> detect-coding-string/decode-coding-string (or perhaps
> decode-coding-(inserted-)region).

This is suboptimal when the URL already tells its charset.  In that
case, you should use the information; you should fall back to
detect-coding-string only if that information is not available.

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Thu, 20 Mar 2014 17:30:46 +0100

On Thu, Mar 20, 2014 at 5:02 PM, Glenn Morris <rgm <at> gnu.org> wrote:
> Juanma Barranquero wrote:

> IMO this could be a bug in whatever script generates elpa webpages.
> Seems like it should specify a charset, based on coding:, if present.

Or a misconfiguration in the Apache server.

> Seems fine to me for 24.4. It's a minor issue.
> Maybe reopen this as a reminder to revisit it afterwards (or new report).

I'm OK with that.

> (BTW, I'm surprised if url.el does not have a way to handle 404s.)

Perhaps I haven't been clear enough. url has ways to deal with it;
url-retrieve(-synchronously) return the status code in the buffer
along other HTTP responses. It's url-insert-file-contents which
ignores it.

The way they are implemented, you can use url-insert, check the status
in the resulting buffer, and lose the additional coding checks that
url-insert-file-contents does, or you can use
url-insert-file-contents, and lose the status check (because
url-insert-file-contents does not give back the buffer it gets from
url-retrieve-synchronously).

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Glenn Morris <rgm <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Thu, 20 Mar 2014 18:26:15 +0100

On Thu, Mar 20, 2014 at 5:21 PM, Eli Zaretskii <eliz <at> gnu.org> wrote:

> This is suboptimal when the URL already tells its charset.  In that
> case, you should use the information; you should fall back to
> detect-coding-string only if that information is not available.

That would mean to manually parse the headers and/or duplicate part of
what url-insert / url-insert-file-contents do. In that case, I'd
prefer one of the other fixes.

   J

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Glenn Morris <rgm <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 02:11:51 +0100

A possible fix for trunk, not the release branch.

Basically, the idea is to move most functionality from
url-insert-file-contents to a new url-insert-file-contents-internal,
which has an additional arg, a check function (possibly nil). That
function is called with the same parameters that
url-insert-file-contents plus the url buffer (with response codes,
etc.) returned by url-retrieve-synchronously. The original u-i-f-c
turns into a wrapper for u-i-f-c-internal, and
package--with-work-buffer can call u-i-f-c-internal and leverage
package-handle-response to check for errors.

(If we can assume that package-handle-response isn't used outside
package.el, a further simplification is possible.)

   J


=== modified file 'lisp/emacs-lisp/package.el'
--- lisp/emacs-lisp/package.el  2014-03-21 06:06:52 +0000
+++ lisp/emacs-lisp/package.el  2014-03-22 00:48:26 +0000
@@ -770,24 +770,20 @@
 and evaluates BODY while that buffer is current.  This work
 buffer is killed afterwards.  Return the last value in BODY."
   (declare (indent 2) (debug t))
-  `(let* ((http (string-match "\\`https?:" ,location))
-         (buffer
-          (if http
-              (url-retrieve-synchronously (concat ,location ,file))
-            (generate-new-buffer "*package work buffer*"))))
-     (prog1
-        (with-current-buffer buffer
-          (if http
-              (progn (package-handle-response)
-                     (re-search-forward "^$" nil 'move)
-                     (forward-char)
-                     (delete-region (point-min) (point)))
-            (unless (file-name-absolute-p ,location)
-              (error "Archive location %s is not an absolute file name"
-                     ,location))
-            (insert-file-contents (expand-file-name ,file ,location)))
-          ,@body)
-       (kill-buffer buffer))))
+  `(with-temp-buffer
+     (if (string-match-p "\\`https?:" ,location)
+        (progn
+          (require 'url-handlers)
+          (url-insert-file-contents-internal
+           (lambda (buffer &rest _)
+             (with-current-buffer buffer (package-handle-response)))
+           (concat ,location ,file))
+          (goto-char (point-min)))
+       (unless (file-name-absolute-p ,location)
+        (error "Archive location %s is not an absolute file name"
+               ,location))
+       (insert-file-contents (expand-file-name ,file ,location)))
+     ,@body))

 (defun package-handle-response ()
   "Handle the response from a `url-retrieve-synchronously' call.
@@ -1272,7 +1268,7 @@
                     (car archive)))))
       ;; Read the retrieved buffer to make sure it is valid (e.g. it
       ;; may fetch a URL redirect page).
-      (when (listp (read buffer))
+      (when (listp (read (current-buffer)))
        (make-directory dir t)
        (setq buffer-file-name (expand-file-name file dir))
        (let ((version-control 'never)
@@ -1531,8 +1527,7 @@
                        (setq readme-string (buffer-string))
                        t))
                 (error nil))
-              (let ((coding (detect-coding-string readme-string t)))
-                (insert (decode-coding-string readme-string coding t))))
+              (insert readme-string))
              ((file-readable-p readme)
               (insert-file-contents readme)
               (goto-char (point-max))))))))

=== modified file 'lisp/url/url-handlers.el'
--- lisp/url/url-handlers.el    2014-01-01 07:43:34 +0000
+++ lisp/url/url-handlers.el    2014-03-22 00:53:21 +0000
@@ -290,11 +290,12 @@
       (insert data))
     (list (length data) charset)))

-;;;###autoload
-(defun url-insert-file-contents (url &optional visit beg end replace)
+(defun url-insert-file-contents-internal (check url &optional visit
beg end replace)
   (let ((buffer (url-retrieve-synchronously url)))
-    (if (not buffer)
-       (error "Opening input file: No such file or directory, %s" url))
+    (when check
+      (unwind-protect
+         (funcall check buffer url visit beg end replace)
+       (when buffer (kill-buffer))))
     (if visit (setq buffer-file-name url))
     (save-excursion
       (let* ((start (point))
@@ -308,6 +309,14 @@
           ;; 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))))))
+
+;;;###autoload
+(defun url-insert-file-contents (url &optional visit beg end replace)
+  (url-insert-file-contents-internal
+   (lambda (buffer url &rest _ignore)
+     (unless buffer
+       (error "Opening input file: No such file or directory, %s" url)))
+   url visit beg end replace))
 (put 'insert-file-contents 'url-file-handlers 'url-insert-file-contents)

 (defun url-file-name-completion (url directory &optional predicate)

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Glenn Morris <rgm <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 02:18:09 +0100

On Sat, Mar 22, 2014 at 2:11 AM, Juanma Barranquero <lekktu <at> gmail.com> wrote:

> +    (when check
> +      (unwind-protect
> +         (funcall check buffer url visit beg end replace)
> +       (when buffer (kill-buffer))))

An obvious braino for

 (when check
   (condition-case err
       (funcall check buffer url visit beg end replace)
     (error
      (when buffer (kill-buffer))
      (signal (car err) (cdr err)))))

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Glenn Morris <rgm <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 02:33:21 +0100

This is the same patch, but adding additional parameters to
package-handle-response to pass the buffer to check.


=== modified file 'lisp/emacs-lisp/package.el'
--- lisp/emacs-lisp/package.el  2014-03-21 06:06:52 +0000
+++ lisp/emacs-lisp/package.el  2014-03-22 01:24:09 +0000
@@ -770,38 +770,35 @@
 and evaluates BODY while that buffer is current.  This work
 buffer is killed afterwards.  Return the last value in BODY."
   (declare (indent 2) (debug t))
-  `(let* ((http (string-match "\\`https?:" ,location))
-         (buffer
-          (if http
-              (url-retrieve-synchronously (concat ,location ,file))
-            (generate-new-buffer "*package work buffer*"))))
-     (prog1
-        (with-current-buffer buffer
-          (if http
-              (progn (package-handle-response)
-                     (re-search-forward "^$" nil 'move)
-                     (forward-char)
-                     (delete-region (point-min) (point)))
-            (unless (file-name-absolute-p ,location)
-              (error "Archive location %s is not an absolute file name"
-                     ,location))
-            (insert-file-contents (expand-file-name ,file ,location)))
-          ,@body)
-       (kill-buffer buffer))))
+  `(with-temp-buffer
+     (if (string-match-p "\\`https?:" ,location)
+        (progn
+          (require 'url-handlers)
+          (url-insert-file-contents-internal #'package-handle-response
+                                             (concat ,location ,file))
+          (goto-char (point-min)))
+       (unless (file-name-absolute-p ,location)
+        (error "Archive location %s is not an absolute file name"
+               ,location))
+       (insert-file-contents (expand-file-name ,file ,location)))
+     ,@body))

-(defun package-handle-response ()
+(defun package-handle-response (&optional buffer &rest _ignore)
   "Handle the response from a `url-retrieve-synchronously' call.
 Parse the HTTP response and throw if an error occurred.
+Parsing happens in BUFFER, or the current buffer if nil.
 The url package seems to require extra processing for this.
 This should be called in a `save-excursion', in the download buffer.
 It will move point to somewhere in the headers."
   ;; We assume HTTP here.
   (require 'url-http)
-  (let ((response (url-http-parse-response)))
-    (when (or (< response 200) (>= response 300))
-      (error "Error downloading %s:%s"
-            (url-recreate-url url-http-target-url)
-            (buffer-substring-no-properties (point) (line-end-position))))))
+  (with-current-buffer (or buffer (current-buffer))
+    (let ((response (url-http-parse-response)))
+      (when (or (< response 200) (>= response 300))
+       (error "Error downloading %s:%s"
+              (url-recreate-url url-http-target-url)
+              (buffer-substring-no-properties (point)
+                                              (line-end-position)))))))

 (defun package--archive-file-exists-p (location file)
   (let ((http (string-match "\\`https?:" location)))
@@ -1272,7 +1269,7 @@
                     (car archive)))))
       ;; Read the retrieved buffer to make sure it is valid (e.g. it
       ;; may fetch a URL redirect page).
-      (when (listp (read buffer))
+      (when (listp (read (current-buffer)))
        (make-directory dir t)
        (setq buffer-file-name (expand-file-name file dir))
        (let ((version-control 'never)
@@ -1531,8 +1528,7 @@
                        (setq readme-string (buffer-string))
                        t))
                 (error nil))
-              (let ((coding (detect-coding-string readme-string t)))
-                (insert (decode-coding-string readme-string coding t))))
+              (insert readme-string))
              ((file-readable-p readme)
               (insert-file-contents readme)
               (goto-char (point-max))))))))

=== modified file 'lisp/url/url-handlers.el'
--- lisp/url/url-handlers.el    2014-01-01 07:43:34 +0000
+++ lisp/url/url-handlers.el    2014-03-22 01:16:06 +0000
@@ -290,11 +290,14 @@
       (insert data))
     (list (length data) charset)))

-;;;###autoload
-(defun url-insert-file-contents (url &optional visit beg end replace)
+(defun url-insert-file-contents-internal (check url &optional visit
beg end replace)
   (let ((buffer (url-retrieve-synchronously url)))
-    (if (not buffer)
-       (error "Opening input file: No such file or directory, %s" url))
+    (when check
+      (condition-case err
+         (funcall check buffer url visit beg end replace)
+       (error
+        (when buffer (kill-buffer))
+        (signal (car err) (cdr err)))))
     (if visit (setq buffer-file-name url))
     (save-excursion
       (let* ((start (point))
@@ -308,6 +311,14 @@
           ;; 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))))))
+
+;;;###autoload
+(defun url-insert-file-contents (url &optional visit beg end replace)
+  (url-insert-file-contents-internal
+   (lambda (buffer url &rest _ignore)
+     (unless buffer
+       (error "Opening input file: No such file or directory, %s" url)))
+   url visit beg end replace))
 (put 'insert-file-contents 'url-file-handlers 'url-insert-file-contents)

 (defun url-file-name-completion (url directory &optional predicate)

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Fri, 21 Mar 2014 22:54:23 -0400

> --- lisp/url/url-handlers.el    2014-01-01 07:43:34 +0000
> +++ lisp/url/url-handlers.el    2014-03-22 01:16:06 +0000
> @@ -290,11 +290,14 @@
>        (insert data))
>      (list (length data) charset)))

> -;;;###autoload
> -(defun url-insert-file-contents (url &optional visit beg end replace)
> +(defun url-insert-file-contents-internal (check url &optional visit
> beg end replace)
>    (let ((buffer (url-retrieve-synchronously url)))
> -    (if (not buffer)
> -       (error "Opening input file: No such file or directory, %s" url))
> +    (when check
> +      (condition-case err
> +         (funcall check buffer url visit beg end replace)
> +       (error
> +        (when buffer (kill-buffer))
> +        (signal (car err) (cdr err)))))
>      (if visit (setq buffer-file-name url))
>      (save-excursion
>        (let* ((start (point))

Let's not call it "-internal" since it should be "always" preferred over
the other one.  Also, I see no need to pass "url visit beg end replace"
to "check" (if that function needs it, the caller can provide it).

Also, how 'bout only calling that `check' function in case of a problem?


        Stefan

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 04:27:10 +0100

On Sat, Mar 22, 2014 at 3:54 AM, Stefan Monnier
<monnier <at> iro.umontreal.ca> wrote:

> Let's not call it "-internal" since it should be "always" preferred over
> the other one.

Then please suggest a name; I'm out of ideas. And a docstring (for
url-insert-file-contents, I mean; the one for url-whatever-its-name is
just the same, plus CHECK).

Also, if the new function is to be "preferred", I'll add an autoload for it.

> Also, I see no need to pass "url visit beg end replace"
> to "check" (if that function needs it, the caller can provide it).

OK.

> Also, how 'bout only calling that `check' function in case of a problem?

That check function is the one that should decide that there is a
problem... The idea being that different callers of
url-whatever-its-name will want to check different things. For
example, the current url-insert-file-contents does not worry about
404, etc., only that the buffer wasn't created (which can happen if
you pass it a mailto: URI, for example). We don't want to change
url-insert-file-contents behavior, do we?

   J

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: rgm <at> gnu.org, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 09:25:42 +0200

> From: Juanma Barranquero <lekktu <at> gmail.com>
> Date: Sat, 22 Mar 2014 02:11:51 +0100
> Cc: Glenn Morris <rgm <at> gnu.org>, 16733 <at> debbugs.gnu.org
> 
> A possible fix for trunk, not the release branch.

Why not for the release branch?  It's a simple fix for a glaring bug.

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Glenn Morris <rgm <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 13:20:33 +0100

On Sat, Mar 22, 2014 at 8:25 AM, Eli Zaretskii <eliz <at> gnu.org> wrote:

> Why not for the release branch?  It's a simple fix for a glaring bug.

Stefan's definition of simple is what counts in this case. If he's OK
with it, great. I agree with you and would prefer to commit it to the
release branch, of course.

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

From: Stefan <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Juanma Barranquero <lekktu <at> gmail.com>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 10:31:39 -0400

>> A possible fix for trunk, not the release branch.
> Why not for the release branch?  It's a simple fix for a glaring bug.

Doesn't look that simple to me.  And IIUC we already have a(nother) fix
for the glaring bug installed in emacs-24.


        Stefan

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

From: Stefan <monnier <at> iro.umontreal.ca>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 10:33:51 -0400

> Then please suggest a name; I'm out of ideas.  And a docstring (for
> url-insert-file-contents, I mean; the one for url-whatever-its-name is
> just the same, plus CHECK).
> Also, if the new function is to be "preferred", I'll add an autoload for it.

Let's settle the rest first.

> We don't want to change url-insert-file-contents behavior, do we?

I don't know, but ignoring 404 errors seems like a bug to me.


        Stefan

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Stefan <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 15:39:28 +0100

On Sat, Mar 22, 2014 at 3:31 PM, Stefan <monnier <at> iro.umontreal.ca> wrote:

> And IIUC we already have a(nother) fix
> for the glaring bug installed in emacs-24.

Yes. The current fix disregards HTTP headers and uses the highest
priority coding system that can decode the received text. It is not
impossible to imagine cases where that would fail, but given that most
ELPA entries are likely to be ASCII or UTF-8, I'm not really worried.

    J

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Stefan <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 15:43:52 +0100

On Sat, Mar 22, 2014 at 3:33 PM, Stefan <monnier <at> iro.umontreal.ca> wrote:

> I don't know, but ignoring 404 errors seems like a bug to me.

url-insert-file-contents acts like visiting the URL. If you go to
http://www.gnu.org/nonexistent, the fact that the page does not exist
is an error from your POV. But your web browser receives the error
page just fine and displays it like any other page (the fact that HTTP
returned 404 is metadata for the action).

If the caller wants a different behavior, they should be using other
lower level url functions, not url-insert-file-contents.

   J

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: monnier <at> iro.umontreal.ca, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 17:02:46 +0200

> From: Juanma Barranquero <lekktu <at> gmail.com>
> Date: Sat, 22 Mar 2014 15:39:28 +0100
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
> 
> On Sat, Mar 22, 2014 at 3:31 PM, Stefan <monnier <at> iro.umontreal.ca> wrote:
> 
> > And IIUC we already have a(nother) fix
> > for the glaring bug installed in emacs-24.
> 
> Yes. The current fix disregards HTTP headers and uses the highest
> priority coding system that can decode the received text. It is not
> impossible to imagine cases where that would fail

The correct fix is not much more complex, so I wonder why do we need
to settle for less than that, when the pretest not even started yet.
There's no danger to destabilize anything here.

> but given that most ELPA entries are likely to be ASCII or UTF-8,
> I'm not really worried.

Indeed, but that goes both ways: the correct fix will not be able to
make too much damage, even if it misses something.

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 16:07:05 +0100

On Sat, Mar 22, 2014 at 4:02 PM, Eli Zaretskii <eliz <at> gnu.org> wrote:

> The correct fix is not much more complex, so I wonder why do we need
> to settle for less than that, when the pretest not even started yet.
> There's no danger to destabilize anything here.

I'm not settling for anything, I wrote what I think is a better fix.
But it's not my decision to say whether it is complex enough or not,
or whether it goes to the trunk or the branch. I've already agreed
with you that, if it were my decision, I would apply the better fix to
the release branch. But at the same time, I'm not worried about the
hacky fix in the branch, so I'm not willing to make a stand on this
issue.

    J

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: monnier <at> iro.umontreal.ca, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 17:09:23 +0200

> From: Juanma Barranquero <lekktu <at> gmail.com>
> Date: Sat, 22 Mar 2014 16:07:05 +0100
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 16733 <at> debbugs.gnu.org
> 
> On Sat, Mar 22, 2014 at 4:02 PM, Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
> > The correct fix is not much more complex, so I wonder why do we need
> > to settle for less than that, when the pretest not even started yet.
> > There's no danger to destabilize anything here.
> 
> I'm not settling for anything

Relax, my comment wasn't aimed at you.

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 16:11:04 +0100

On Sat, Mar 22, 2014 at 4:09 PM, Eli Zaretskii <eliz <at> gnu.org> wrote:

> Relax, my comment wasn't aimed at you.

Well, it was a direct reply to a message by me.

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

From: Stefan <monnier <at> iro.umontreal.ca>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 11:55:27 -0400

>> I don't know, but ignoring 404 errors seems like a bug to me.
> url-insert-file-contents acts like visiting the URL. If you go to
> http://www.gnu.org/nonexistent, the fact that the page does not exist
> is an error from your POV. But your web browser receives the error
> page just fine and displays it like any other page (the fact that HTTP
> returned 404 is metadata for the action).

I think for url-insert-file-contents, the intention is to pretend URLs
make up a virtual file-system (that's the point of url-handlers.el).
From this point of view http://www.gnu.org/nonexistent is a file that
doesn't exist.  So url-insert-file-contents should signal an error just
like insert-file-contents would.

> If the caller wants a different behavior, they should be using other
> lower level url functions, not url-insert-file-contents.

I think it's the other way around: if the caller really wants to see the
particular error page that www.gnu.org decided to return, then he should
use a lower level URL function.

Similarly, url-insert-file-contents should (and does, IIUC) follow
redirections so you get to the the destination of the redirection rather
than the redirection page itself.


        Stefan

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Stefan <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 17:46:45 +0100

On Sat, Mar 22, 2014 at 4:55 PM, Stefan <monnier <at> iro.umontreal.ca> wrote:

> I think for url-insert-file-contents, the intention is to pretend URLs
> make up a virtual file-system (that's the point of url-handlers.el).
> From this point of view http://www.gnu.org/nonexistent is a file that
> doesn't exist.  So url-insert-file-contents should signal an error just
> like insert-file-contents would.

The semantics of a missing file in the filesystem and a missing page
in a web site are different; filesystems do not usually have a
"default error file" to return when the search failed; web sites, do,
and in many cases, that page *is* informative; it can contain helpful
links, etc. Try accessing

  http://en.wikipedia.org/wiki/Stefan_Monnier

It returns a 404, but still, the error page created on the fly is
quite useful (with a simple click you can search all mentions of
"Stefan Monnier" in the Wikipedia, for example).

> Similarly, url-insert-file-contents should (and does, IIUC) follow
> redirections so you get to the the destination of the redirection rather
> than the redirection page itself.

Because the semantics of a redirection imply following the
redirection. There's nothing in the semantics of 404 that say that you
should ignore the result page, or not. Is up to the application /
user. 404 is not telling you that you got nothing, only that you
didn't get what you expected.

Anyway, I can see how this discussion could lead us nowhere. We
disagree in two points, so let's recapitulate:

- Currently, url-insert-file-contents does ignore 404 results and most
other response codes. I don't think it's a good idea to change its
long-standing behavior, but if you insist, it's your call. That's only
a matter of changing the function passed as CHECK to
url-whatever-its-name.

- You suggest that url-w-i-n should only call CHECK "in case of a
problem". I do not like this, because I think CHECK is *the* function
tasked with telling whether there's a problem or not. How will you
define what is a problem (what if I really want to get back these 404
pages?) and how to pass back that information to CHECK?

     J

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

From: Stefan <monnier <at> IRO.UMontreal.CA>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 14:53:31 -0400

>> I think for url-insert-file-contents, the intention is to pretend URLs
>> make up a virtual file-system (that's the point of url-handlers.el).
>> From this point of view http://www.gnu.org/nonexistent is a file that
>> doesn't exist.  So url-insert-file-contents should signal an error just
>> like insert-file-contents would.
> The semantics of a missing file in the filesystem and a missing page
> in a web site are different; filesystems do not usually have a
> "default error file" to return when the search failed; web sites, do,
> and in many cases, that page *is* informative; it can contain helpful
> links, etc. Try accessing

I know.  But we're talking about a file-system, not about the web.
So we're talking about accessing those not in a browser but from any
random piece of code which may do god-knows-what.

>   http://en.wikipedia.org/wiki/Stefan_Monnier
> It returns a 404, but still, the error page created on the fly is
> quite useful (with a simple click you can search all mentions of
> "Stefan Monnier" in the Wikipedia, for example).

That's useful in a browser where the error can be rendered and read by
a user.  But it's inconsistent for example with url-http-file-exists-p.

When your code fetches http://foo.bar/baz.el, it probably won't expect
to get some HTML content instead.

> - Currently, url-insert-file-contents does ignore 404 results and most
> other response codes. I don't think it's a good idea to change its
> long-standing behavior, but if you insist, it's your call.

I think it should be changed, yes.

> - You suggest that url-w-i-n should only call CHECK "in case of a
> problem". I do not like this, because I think CHECK is *the* function
> tasked with telling whether there's a problem or not. How will you
> define what is a problem (what if I really want to get back these 404
> pages?) and how to pass back that information to CHECK?

After url-insert-file-contents is changed, I don't care much about the
rest because package.el will not need to call something else than
url-insert-file-contents so we may not even need url-w-i-n.


        Stefan

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Stefan <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sat, 22 Mar 2014 20:05:07 +0100

On Sat, Mar 22, 2014 at 7:53 PM, Stefan <monnier <at> iro.umontreal.ca> wrote:

> After url-insert-file-contents is changed, I don't care much about the
> rest because package.el will not need to call something else than
> url-insert-file-contents so we may not even need url-w-i-n.

You'll have to describe the semantics you expect from url-i-f-c in
greater detail, because at this moment I don't know how you define
error, how do you intend to check it, and what you want to do if an
error is detected.

    J

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

From: Stefan <monnier <at> IRO.UMontreal.CA>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Sun, 23 Mar 2014 18:24:22 -0400

>> After url-insert-file-contents is changed, I don't care much about the
>> rest because package.el will not need to call something else than
>> url-insert-file-contents so we may not even need url-w-i-n.
> You'll have to describe the semantics you expect from url-i-f-c in
> greater detail, because at this moment I don't know how you define
> error,

I'd follow the behavior of url-http-file-exists-p, i.e. consider any
result outside the range [200,300[ to be an error.

> how do you intend to check it,

Not sure about the details, here, because I'm not familiar enough with
the URL package.

> and what you want to do if an error is detected.

Same as insert-file-contents, i.e. signal a `file-error'.


        Stefan

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Stefan <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Tue, 25 Mar 2014 04:04:10 +0100

2014-03-25  Juanma Barranquero  <lekktu <at> gmail.com>

        * url-handlers.el (url-http-parse-response): Add autoload.
        (url-insert-file-contents): Signal file-error in case of HTTP error.

       * emacs-lisp/package.el (url-http-parse-response)
        (url-http-end-of-headers, url-recreate-url, url-http-target-url):
        Remove unused declarations.
        (package-handle-response): Remove.
        (package--with-work-buffer): Use url-insert-file-contents and simplify.
        (package--download-one-archive): Use current-buffer instead of
        dynamic binding of `buffer'.
        (describe-package-1): Do not decode readme-string.


=== modified file 'lisp/url/url-handlers.el'
--- lisp/url/url-handlers.el    2014-01-01 07:43:34 +0000
+++ lisp/url/url-handlers.el    2014-03-25 02:44:43 +0000
@@ -33,6 +33,7 @@
 (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))
@@ -293,21 +294,27 @@
 ;;;###autoload
 (defun url-insert-file-contents (url &optional visit beg end replace)
   (let ((buffer (url-retrieve-synchronously url)))
-    (if (not buffer)
-       (error "Opening input file: No such file or directory, %s" url))
-    (if visit (setq buffer-file-name url))
-    (save-excursion
-      (let* ((start (point))
-             (size-and-charset (url-insert buffer beg end)))
-        (kill-buffer buffer)
-        (when replace
-          (delete-region (point-min) start)
-          (delete-region (point) (point-max)))
-        (unless (cadr size-and-charset)
-          ;; If the headers don't specify any particular charset, use the
-          ;; 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))))))
+    (unwind-protect
+       (progn
+         (when (or (not buffer)
+                   (with-current-buffer buffer
+                     (let ((response (url-http-parse-response)))
+                       (goto-char (point-min))
+                       (or (< response 200) (>= response 300)))))
+           (signal 'file-error `("Opening input URL" ,url)))
+         (if visit (setq buffer-file-name url))
+         (save-excursion
+           (let* ((start (point))
+                  (size-and-charset (url-insert buffer beg end)))
+             (when replace
+               (delete-region (point-min) start)
+               (delete-region (point) (point-max)))
+             (unless (cadr size-and-charset)
+               ;; If the headers don't specify any particular charset, use the
+               ;; 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)))))
+      (when buffer (kill-buffer buffer)))))
 (put 'insert-file-contents 'url-file-handlers 'url-insert-file-contents)

 (defun url-file-name-completion (url directory &optional predicate)

=== modified file 'lisp/emacs-lisp/package.el'
--- lisp/emacs-lisp/package.el  2014-03-22 08:43:30 +0000
+++ lisp/emacs-lisp/package.el  2014-03-25 02:46:22 +0000
@@ -205,13 +205,9 @@

 (defvar Info-directory-list)
 (declare-function info-initialize "info" ())
-(declare-function url-http-parse-response "url-http" ())
 (declare-function url-http-file-exists-p "url-http" (url))
 (declare-function lm-header "lisp-mnt" (header))
 (declare-function lm-commentary "lisp-mnt" (&optional file))
-(defvar url-http-end-of-headers)
-(declare-function url-recreate-url "url-parse" (urlobj))
-(defvar url-http-target-url)

 (defcustom package-archives '(("gnu" . "http://elpa.gnu.org/packages/"))
   "An alist of archives from which to fetch.
@@ -770,38 +766,14 @@
 and evaluates BODY while that buffer is current.  This work
 buffer is killed afterwards.  Return the last value in BODY."
   (declare (indent 2) (debug t))
-  `(let* ((http (string-match "\\`https?:" ,location))
-         (buffer
-          (if http
-              (url-retrieve-synchronously (concat ,location ,file))
-            (generate-new-buffer "*package work buffer*"))))
-     (prog1
-        (with-current-buffer buffer
-          (if http
-              (progn (package-handle-response)
-                     (re-search-forward "^$" nil 'move)
-                     (forward-char)
-                     (delete-region (point-min) (point)))
-            (unless (file-name-absolute-p ,location)
-              (error "Archive location %s is not an absolute file name"
-                     ,location))
-            (insert-file-contents (expand-file-name ,file ,location)))
-          ,@body)
-       (kill-buffer buffer))))
-
-(defun package-handle-response ()
-  "Handle the response from a `url-retrieve-synchronously' call.
-Parse the HTTP response and throw if an error occurred.
-The url package seems to require extra processing for this.
-This should be called in a `save-excursion', in the download buffer.
-It will move point to somewhere in the headers."
-  ;; We assume HTTP here.
-  (require 'url-http)
-  (let ((response (url-http-parse-response)))
-    (when (or (< response 200) (>= response 300))
-      (error "Error downloading %s:%s"
-            (url-recreate-url url-http-target-url)
-            (buffer-substring-no-properties (point) (line-end-position))))))
+  `(with-temp-buffer
+     (if (string-match-p "\\`https?:" ,location)
+        (url-insert-file-contents (concat ,location ,file))
+       (unless (file-name-absolute-p ,location)
+        (error "Archive location %s is not an absolute file name"
+               ,location))
+       (insert-file-contents (expand-file-name ,file ,location)))
+     ,@body))

 (defun package--archive-file-exists-p (location file)
   (let ((http (string-match "\\`https?:" location)))
@@ -1272,7 +1244,7 @@
                     (car archive)))))
       ;; Read the retrieved buffer to make sure it is valid (e.g. it
       ;; may fetch a URL redirect page).
-      (when (listp (read buffer))
+      (when (listp (read (current-buffer)))
        (make-directory dir t)
        (setq buffer-file-name (expand-file-name file dir))
        (let ((version-control 'never)
@@ -1531,8 +1503,7 @@
                        (setq readme-string (buffer-string))
                        t))
                 (error nil))
-              (let ((coding (detect-coding-string readme-string t)))
-                (insert (decode-coding-string readme-string coding t))))
+              (insert readme-string))
              ((file-readable-p readme)
               (insert-file-contents readme)
               (goto-char (point-max))))))))

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

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Mon, 24 Mar 2014 23:35:07 -0400

> +           (signal 'file-error `("Opening input URL" ,url)))

The old package.el code included (buffer-substring-no-properties (point)
(line-end-position)) in the message.  Would it make sense to put it here
as well?

Otherwise, it looks good, and I think we can even put it on emacs-24.


        Stefan

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

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Tue, 25 Mar 2014 10:54:29 +0100

On Tue, Mar 25, 2014 at 4:35 AM, Stefan Monnier
<monnier <at> iro.umontreal.ca> wrote:

> The old package.el code included (buffer-substring-no-properties (point)
> (line-end-position)) in the message.  Would it make sense to put it here
> as well?

It's the HTTP error description. As the "no buffer" case doesn't have
such a description, I've modified url-insert-file-contents so in tha
case it returns "No Data", to match "Not Found" and other HTTP errors.

That's the resulting url-i-f-c:

(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))))
            (kill-buffer buffer)
            (signal 'file-error (list url desc))))))
    (if visit (setq buffer-file-name url))
    (save-excursion
      (let* ((start (point))
             (size-and-charset (url-insert buffer beg end)))
        (kill-buffer buffer)
        (when replace
          (delete-region (point-min) start)
          (delete-region (point) (point-max)))
        (unless (cadr size-and-charset)
          ;; If the headers don't specify any particular charset, use the
          ;; 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))))))

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

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733 <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Tue, 25 Mar 2014 09:21:25 -0400

> That's the resulting url-i-f-c:

Looks good, thanks,


        Stefan

Message #147 received at 16733-done <at> debbugs.gnu.org (full text, mbox):

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 16733-done <at> debbugs.gnu.org
Subject: Re: bug#16733: messed up unicode chars in package description
Date: Wed, 26 Mar 2014 16:24:53 +0100

Version: 24.4

Committed to the release branch.

Previous Next

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