On Sun,  8 Jan 2012 00:52:42 -0700, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> +(defun w3m-region (start end)) ;; From `w3m.el'.
> +(defun notmuch-mua-quote-part (part)
> +  (with-temp-buffer
> +    (insert part)
> +    (message-mode)
> +    (fill-region (point-min) (point-max))
> +    (goto-char (point-min))
> +    (perform-replace "^" "> " nil t nil)
> +    (set-buffer-modified-p nil)
> +    (buffer-substring (point-min) (point-max))))

Couldn't all of this be done directly in the reply buffer?

> +(defun notmuch-mua-parse-html-part (part)
> +  (with-temp-buffer
> +    (insert part)
> +    (w3m-region (point-min) (point-max))
> +    (set-buffer-modified-p nil)
> +    (buffer-substring (point-min) (point-max))))

Blank lines between functions are the house style (as much as there is
such a thing).

Using w3m means that you should `require' it. What happens when a user
doesn't have it? (Either the elisp or the command.)

>  (defun notmuch-mua-reply (query-string &optional sender)
> -  (let (headers
> +  (let (reply
> +	original
> +	headers
>  	body
> -	(args '("reply")))
> +	(args '("reply" "--format=json")))

Initialised variables are generally shown before uninitialised. I know
that you didn't do this 'wrong' and that it's an unrelated change, but
it should be fixed. (To the people who say that should be a separate
patch I say 'meh' - life is too short.)

>      (if notmuch-show-process-crypto
>  	(setq args (append args '("--decrypt"))))
>      (setq args (append args (list query-string)))
> -    ;; This make assumptions about the output of `notmuch reply', but
> -    ;; really only that the headers come first followed by a blank
> -    ;; line and then the body.
> +    ;; Get the reply object as JSON, and parse it into an elisp object.
>      (with-temp-buffer
>        (apply 'call-process (append (list notmuch-command nil (list t t) nil) args))
>        (goto-char (point-min))
> -      (if (re-search-forward "^$" nil t)
> -	  (save-excursion
> -	    (save-restriction
> -	      (narrow-to-region (point-min) (point))
> -	      (goto-char (point-min))
> -	      (setq headers (mail-header-extract)))))
> -      (forward-line 1)
> -      (setq body (buffer-substring (point) (point-max))))
> +      (setq reply (aref (json-read) 0)))
> +
> +    ;; Get the list of headers
> +    (setq headers (cdr (assq 'headers (assq 'reply reply))))
> +    ;; Construct the body of the reply.
> +    (setq original (cdr (assq 'original reply)))

The scope of (at least) `headers' and `original' could be tightened by
moving them down to the following `let' (and converting it to `let*').

> +
> +    ;; Start with the prelude, based on the headers of the original message.
> +    (let ((original-headers (cdr (assq 'headers original))))
> +      (setq body (format "On %s, %s wrote:\n"
> +			 (cdr (assq 'date original-headers))
> +			 (cdr (assq 'from original-headers)))))
> +
> +    ;; Extract the body parts and construct a reasonable quoted body.
> +    (let* ((body-parts (cdr (assq 'body original)))
> +	   (find-parts (lambda (type) (delq nil (mapcar (lambda (part)
> +							  (if (string= (cdr (assq 'content-type part)) type)
> +							      (cdr (assq 'content part))))
> +							body-parts))))

`find-parts' might be useful in other places. Maybe add it to notmuch-lib.el?

> +	   (plain-parts (apply find-parts '("text/plain")))
> +	   (html-parts (apply find-parts '("text/html"))))
> +      
> +      (if (not (null plain-parts))
> +	  (mapc (lambda (part) (setq body (concat body (notmuch-mua-quote-part part)))) plain-parts)
> +	(mapc (lambda (part) (setq body (concat body (notmuch-mua-quote-part (notmuch-mua-parse-html-part part))))) html-parts)))

If you have an 'else' clause, why test '(if (not ..' ?

> +    (setq body (concat body "\n"))
> +	

If it already ends with a carriage return, why do this?