[PATCH v2] Output unmodified Content-Type header value for JSON format.
On Thu, 12 Jan 2012 23:16:29 -0400, David Bremner wrote: > On Thu, 12 Jan 2012 18:08:08 +0100, Pieter Praet wrote: > > > > - The previous point is a bit of a counterargument to this, but in > > > > I couls separate it. I made is a single patch to avoid having a > > > revision with broken emacs UI (and tests). > > > > > > I'd like to propose to always apply patch series on a *topic* branch > > which would then be merged back into 'master', thus avoiding this issue > > altogether whilst making it more obvious which patches belong together > > (eg. for easier cross-referencing with the ML). > > Hi Pieter; > > Sorry, I must have lost the thread somewhere. What application are you > thinking about, and what issue do you think that topic branches avoid? > No specific application; In the case of patch series, it may occur that an individual commit leaves the code in a broken state. One might argue that devs need to ensure that the project builds cleanly at every commit, but this can be prohibitively time-consuming or even simply impossible due to the need to keep changes atomic and area-specific (eg. changes to the binary and the Emacs UI should be separate commits irrespective of their relation). So it would make sense to consistently apply (certain, or perhaps all?) patch series on an ad-hoc branch which would then be merged back into 'master', instead of being directly applied on 'master'. Aside from preventing broken builds (as was Dmitry's reason to consolidate a series touching on multiple distinct "spheres" of the code into a single patch), series of related commits would also be clearly delineated/clustered, facilitating source archaeology. (and all without making the commit log exude the odor of entropy incarnate) > d > Peace -- Pieter
[PATCH v2] Output unmodified Content-Type header value for JSON format.
On Thu, 12 Jan 2012 18:08:08 +0100, Pieter Praet wrote: > > > - The previous point is a bit of a counterargument to this, but in > > I couls separate it. I made is a single patch to avoid having a > > revision with broken emacs UI (and tests). > > > I'd like to propose to always apply patch series on a *topic* branch > which would then be merged back into 'master', thus avoiding this issue > altogether whilst making it more obvious which patches belong together > (eg. for easier cross-referencing with the ML). Hi Pieter; Sorry, I must have lost the thread somewhere. What application are you thinking about, and what issue do you think that topic branches avoid? d
[PATCH v2] Output unmodified Content-Type header value for JSON format.
On Sun, 20 Nov 2011 22:35:42 +0400, Dmitry Kurochkin wrote: > On Sat, 19 Nov 2011 08:59:29 -0400, David Bremner > wrote: > > On Sat, 19 Nov 2011 08:18:41 +0400, Dmitry Kurochkin > gmail.com> wrote: > > > Before the change, notmuch used g_mime_content_type_to_string(3) > > > function to output Content-Type header value. Turns out it outputs > > > only "type/subtype" part and ignores all parameters. Also, if there > > > is no Content-Type header, default "text/plain" value is used. > > > > Hi Dmitry; > > > > I haven't analyzed the substance of your patch yet, but I did have a > > couple thoughts while reading your mail. > > > > - It seems that every time we change the json format, we have a round of > > suffering because people are unable to detect a mismatch between their > > emacs code and the cli. Not that this is your problem necessarily, but > > it would be nice if someone (TM), would come up with some version info > > for the json output, and a patch to check it on the emacs side. > > > > IMO this is a good idea. > > > - The previous point is a bit of a counterargument to this, but in > > general, I think I prefer patches that modify the core seperate from > > those that do emacs (or python, or ...) stuff. > > > > I couls separate it. I made is a single patch to avoid having a > revision with broken emacs UI (and tests). > I'd like to propose to always apply patch series on a *topic* branch which would then be merged back into 'master', thus avoiding this issue altogether whilst making it more obvious which patches belong together (eg. for easier cross-referencing with the ML). > Regards, > Dmitry > > > - I understand you want to make your patches reviewable without applying > > by including lots of context, but at a certain point it has actually > > the opposite effect for me. I just don't read 900+ line emails ;). Of > > course, I can still apply the patch and look at it, so it's really up > > to you. > > > > d > ___ > notmuch mailing list > notmuch at notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch Peace -- Pieter
Re: [PATCH v2] Output unmodified Content-Type header value for JSON format.
On Thu, 12 Jan 2012 18:08:08 +0100, Pieter Praet pie...@praet.org wrote: - The previous point is a bit of a counterargument to this, but in I couls separate it. I made is a single patch to avoid having a revision with broken emacs UI (and tests). I'd like to propose to always apply patch series on a *topic* branch which would then be merged back into 'master', thus avoiding this issue altogether whilst making it more obvious which patches belong together (eg. for easier cross-referencing with the ML). Hi Pieter; Sorry, I must have lost the thread somewhere. What application are you thinking about, and what issue do you think that topic branches avoid? d ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH v2] Output unmodified Content-Type header value for JSON format.
On Sat, 19 Nov 2011 08:59:29 -0400, David Bremner wrote: > On Sat, 19 Nov 2011 08:18:41 +0400, Dmitry Kurochkin gmail.com> wrote: > > Before the change, notmuch used g_mime_content_type_to_string(3) > > function to output Content-Type header value. Turns out it outputs > > only "type/subtype" part and ignores all parameters. Also, if there > > is no Content-Type header, default "text/plain" value is used. > > Hi Dmitry; > > I haven't analyzed the substance of your patch yet, but I did have a > couple thoughts while reading your mail. > > - It seems that every time we change the json format, we have a round of > suffering because people are unable to detect a mismatch between their > emacs code and the cli. Not that this is your problem necessarily, but > it would be nice if someone (TM), would come up with some version info > for the json output, and a patch to check it on the emacs side. > IMO this is a good idea. > - The previous point is a bit of a counterargument to this, but in > general, I think I prefer patches that modify the core seperate from > those that do emacs (or python, or ...) stuff. > I couls separate it. I made is a single patch to avoid having a revision with broken emacs UI (and tests). Regards, Dmitry > - I understand you want to make your patches reviewable without applying > by including lots of context, but at a certain point it has actually > the opposite effect for me. I just don't read 900+ line emails ;). Of > course, I can still apply the patch and look at it, so it's really up > to you. > > d
Re: [PATCH v2] Output unmodified Content-Type header value for JSON format.
On Sat, 19 Nov 2011 08:59:29 -0400, David Bremner da...@tethera.net wrote: On Sat, 19 Nov 2011 08:18:41 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: Before the change, notmuch used g_mime_content_type_to_string(3) function to output Content-Type header value. Turns out it outputs only type/subtype part and ignores all parameters. Also, if there is no Content-Type header, default text/plain value is used. Hi Dmitry; I haven't analyzed the substance of your patch yet, but I did have a couple thoughts while reading your mail. - It seems that every time we change the json format, we have a round of suffering because people are unable to detect a mismatch between their emacs code and the cli. Not that this is your problem necessarily, but it would be nice if someone (TM), would come up with some version info for the json output, and a patch to check it on the emacs side. IMO this is a good idea. - The previous point is a bit of a counterargument to this, but in general, I think I prefer patches that modify the core seperate from those that do emacs (or python, or ...) stuff. I couls separate it. I made is a single patch to avoid having a revision with broken emacs UI (and tests). Regards, Dmitry - I understand you want to make your patches reviewable without applying by including lots of context, but at a certain point it has actually the opposite effect for me. I just don't read 900+ line emails ;). Of course, I can still apply the patch and look at it, so it's really up to you. d ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH v2] Output unmodified Content-Type header value for JSON format.
Before the change, notmuch used g_mime_content_type_to_string(3) function to output Content-Type header value. Turns out it outputs only "type/subtype" part and ignores all parameters. Also, if there is no Content-Type header, default "text/plain" value is used. JSON is supposed to be a "low-level" structured format and should not add missing values or throw away information. The patch changes notmuch show to use unmodified Content-Type value for JSON format. Also, no default value is added if the header is missing. Corresponding changes to Emacs UI are made to handle full Content-Type header values. The header is parsed using MIME `mail-header-parse-content-type' function. All message part rendering functions have access to full Content-Type value. In particular, this is important for `notmuch-show-mm-display-part-inline' which uses `mm-display-part' to display parts that notmuch-show does not handle. Expected results for the tests are updated accordingly. --- Changes in v2 since v1: * Use "text/plain; charset=us-ascii" for default Content-Type value in `notmuch-show-get-content-type' function as defined in RFC 2045. emacs/notmuch-show.el | 29 +++-- notmuch-show.c| 14 -- test/crypto | 23 --- test/json |6 +++--- test/maildir-sync |1 - test/multipart| 36 ++-- 6 files changed, 60 insertions(+), 49 deletions(-) diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el index d5c95d8..cb801ae 100644 --- a/emacs/notmuch-show.el +++ b/emacs/notmuch-show.el @@ -261,120 +261,120 @@ message at DEPTH in the current thread." (if (and header-value (not (string-equal "" header-value))) (notmuch-show-insert-header header header-value notmuch-message-headers) (save-excursion (save-restriction (narrow-to-region start (point-max)) (run-hooks 'notmuch-show-markup-headers-hook) (define-button-type 'notmuch-show-part-button-type 'action 'notmuch-show-part-button-action 'follow-link t 'face 'message-mml) (defun notmuch-show-insert-part-header (nth content-type declared-type name comment) (let ((button)) (setq button (insert-button (concat "[ " (if name (concat name ": ") "") - declared-type - (if (not (string-equal declared-type content-type)) - (concat " (as " content-type ")") + (car declared-type) + (if (not (string-equal (car declared-type) (car content-type))) + (concat " (as " (car content-type) ")") "") (or comment "") " ]") :type 'notmuch-show-part-button-type :notmuch-part nth :notmuch-filename name)) (insert "\n") ;; return button button)) ;; Functions handling particular MIME parts. (defun notmuch-show-save-part (message-id nth filename) (let ((process-crypto notmuch-show-process-crypto)) (with-temp-buffer (setq notmuch-show-process-crypto process-crypto) ;; Always acquires the part via `notmuch part', even if it is ;; available in the JSON output. (insert (notmuch-show-get-bodypart-internal message-id nth)) (let ((file (read-file-name "Filename to save as: " (or mailcap-download-directory "~/") nil nil filename))) ;; Don't re-compress .gz & al. Arguably we should make ;; `file-name-handler-alist' nil, but that would chop ;; ange-ftp, which is reasonable to use here. (mm-write-region (point-min) (point-max) file nil nil nil 'no-conversion t) (defun notmuch-show-mm-display-part-inline (msg part content-type content) "Use the mm-decode/mm-view functions to display a part in the current buffer, if possible." (let ((display-buffer (current-buffer))) (with-temp-buffer (insert content) - (let ((handle (mm-make-handle (current-buffer) (list content-type + (let ((handle (mm-make-handle (current-buffer) content-type))) (set-buffer display-buffer) (if (and (mm-inlinable-p handle) (mm-inlined-p handle)) (progn (mm-display-part handle) t) nil) (defvar notmuch-show-multipart/alternative-discouraged '( ;; Avoid HTML parts. "text/html" ;; multipart/related usually contain a text/html part and some associated graphics. "multipart/related" )) (defun notmuch-show-multipart/*-to-list (part) - (mapcar '(lambda (inner-part) (plist-get inner-part :content-type)) + (mapcar '(lambda (inner-part) (car (notmuch-show-get-content-type inner-part))) (plist-get part :content)))
Re: [PATCH v2] Output unmodified Content-Type header value for JSON format.
On Sat, 19 Nov 2011 08:18:41 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: Before the change, notmuch used g_mime_content_type_to_string(3) function to output Content-Type header value. Turns out it outputs only type/subtype part and ignores all parameters. Also, if there is no Content-Type header, default text/plain value is used. Hi Dmitry; I haven't analyzed the substance of your patch yet, but I did have a couple thoughts while reading your mail. - It seems that every time we change the json format, we have a round of suffering because people are unable to detect a mismatch between their emacs code and the cli. Not that this is your problem necessarily, but it would be nice if someone (TM), would come up with some version info for the json output, and a patch to check it on the emacs side. - The previous point is a bit of a counterargument to this, but in general, I think I prefer patches that modify the core seperate from those that do emacs (or python, or ...) stuff. - I understand you want to make your patches reviewable without applying by including lots of context, but at a certain point it has actually the opposite effect for me. I just don't read 900+ line emails ;). Of course, I can still apply the patch and look at it, so it's really up to you. d ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
