* bug#17549: 24.4.50; regression: url-insert-file-contents @ 2014-05-22 11:11 Leo Liu 2014-05-29 23:24 ` Leo Liu 0 siblings, 1 reply; 12+ messages in thread From: Leo Liu @ 2014-05-22 11:11 UTC (permalink / raw) To: 17549; +Cc: Juanma Barranquero 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#17549: 24.4.50; regression: url-insert-file-contents 2014-05-22 11:11 bug#17549: 24.4.50; regression: url-insert-file-contents Leo Liu @ 2014-05-29 23:24 ` Leo Liu 2014-05-30 2:09 ` Juanma Barranquero 0 siblings, 1 reply; 12+ messages in thread From: Leo Liu @ 2014-05-29 23:24 UTC (permalink / raw) To: 17549; +Cc: Juanma Barranquero 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#17549: 24.4.50; regression: url-insert-file-contents 2014-05-29 23:24 ` Leo Liu @ 2014-05-30 2:09 ` Juanma Barranquero 2014-06-24 3:24 ` Leo Liu 0 siblings, 1 reply; 12+ messages in thread From: Juanma Barranquero @ 2014-05-30 2:09 UTC (permalink / raw) To: Leo Liu; +Cc: 17549 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#17549: 24.4.50; regression: url-insert-file-contents 2014-05-30 2:09 ` Juanma Barranquero @ 2014-06-24 3:24 ` Leo Liu 2014-06-24 12:49 ` Stefan Monnier 0 siblings, 1 reply; 12+ messages in thread From: Leo Liu @ 2014-06-24 3:24 UTC (permalink / raw) To: Juanma Barranquero; +Cc: 17549 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#17549: 24.4.50; regression: url-insert-file-contents 2014-06-24 3:24 ` Leo Liu @ 2014-06-24 12:49 ` Stefan Monnier 2014-06-24 12:58 ` Juanma Barranquero 2014-06-24 13:38 ` Leo Liu 0 siblings, 2 replies; 12+ messages in thread From: Stefan Monnier @ 2014-06-24 12:49 UTC (permalink / raw) To: Leo Liu; +Cc: Juanma Barranquero, 17549 >> 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#17549: 24.4.50; regression: url-insert-file-contents 2014-06-24 12:49 ` Stefan Monnier @ 2014-06-24 12:58 ` Juanma Barranquero 2014-06-24 13:38 ` Leo Liu 1 sibling, 0 replies; 12+ messages in thread From: Juanma Barranquero @ 2014-06-24 12:58 UTC (permalink / raw) To: Stefan Monnier; +Cc: 17549, Leo Liu [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#17549: 24.4.50; regression: url-insert-file-contents 2014-06-24 12:49 ` Stefan Monnier 2014-06-24 12:58 ` Juanma Barranquero @ 2014-06-24 13:38 ` Leo Liu 2014-06-24 14:01 ` Juanma Barranquero 1 sibling, 1 reply; 12+ messages in thread From: Leo Liu @ 2014-06-24 13:38 UTC (permalink / raw) To: Stefan Monnier; +Cc: Juanma Barranquero, 17549 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#17549: 24.4.50; regression: url-insert-file-contents 2014-06-24 13:38 ` Leo Liu @ 2014-06-24 14:01 ` Juanma Barranquero 2014-06-25 0:58 ` Leo Liu 0 siblings, 1 reply; 12+ messages in thread From: Juanma Barranquero @ 2014-06-24 14:01 UTC (permalink / raw) To: Leo Liu; +Cc: 17549 [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#17549: 24.4.50; regression: url-insert-file-contents 2014-06-24 14:01 ` Juanma Barranquero @ 2014-06-25 0:58 ` Leo Liu 2014-06-26 0:19 ` Leo Liu 0 siblings, 1 reply; 12+ messages in thread From: Leo Liu @ 2014-06-25 0:58 UTC (permalink / raw) To: Juanma Barranquero; +Cc: 17549 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#17549: 24.4.50; regression: url-insert-file-contents 2014-06-25 0:58 ` Leo Liu @ 2014-06-26 0:19 ` Leo Liu 2014-06-26 2:38 ` Stefan Monnier 0 siblings, 1 reply; 12+ messages in thread From: Leo Liu @ 2014-06-26 0:19 UTC (permalink / raw) To: Stefan Monnier; +Cc: 17549 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#17549: 24.4.50; regression: url-insert-file-contents 2014-06-26 0:19 ` Leo Liu @ 2014-06-26 2:38 ` Stefan Monnier 2014-06-26 4:03 ` Leo Liu 0 siblings, 1 reply; 12+ messages in thread From: Stefan Monnier @ 2014-06-26 2:38 UTC (permalink / raw) To: Leo Liu; +Cc: 17549 > +(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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#17549: 24.4.50; regression: url-insert-file-contents 2014-06-26 2:38 ` Stefan Monnier @ 2014-06-26 4:03 ` Leo Liu 0 siblings, 0 replies; 12+ messages in thread From: Leo Liu @ 2014-06-26 4:03 UTC (permalink / raw) To: Stefan Monnier; +Cc: 17549 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-06-26 4:03 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-05-22 11:11 bug#17549: 24.4.50; regression: url-insert-file-contents Leo Liu 2014-05-29 23:24 ` Leo Liu 2014-05-30 2:09 ` Juanma Barranquero 2014-06-24 3:24 ` Leo Liu 2014-06-24 12:49 ` Stefan Monnier 2014-06-24 12:58 ` Juanma Barranquero 2014-06-24 13:38 ` Leo Liu 2014-06-24 14:01 ` Juanma Barranquero 2014-06-25 0:58 ` Leo Liu 2014-06-26 0:19 ` Leo Liu 2014-06-26 2:38 ` Stefan Monnier 2014-06-26 4:03 ` Leo Liu
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).