v5 emacs postpone/resume patches

v5 emacs postpone/resume patches

[David Bremner]

This obsoletes id:20161106121501.11062-2-david@tethera.net

The actual net diff is the same, except for adding some tests.

The changes have been broken up into smaller pieces to make review
easier. 

[Patch v5 1/4] emacs: tree: remove binding for pressing button in message pane

[David Bremner]

From: Mark Walters <markwalters1009@gmail.com>

We want to use "e" for editting postponed messages in show, and in
tree view, so remove the binding for the function which does

     (In message pane) Activate BUTTON or button at point
---
 emacs/notmuch-tree.el | 1 -
 1 file changed, 1 deletion(-)

diff --git a/emacs/notmuch-tree.el b/emacs/notmuch-tree.el
index 62064ce..8398eb1 100644
--- a/emacs/notmuch-tree.el
+++ b/emacs/notmuch-tree.el
@@ -266,7 +266,6 @@ FUNC."
     (define-key map (kbd "M-TAB") (notmuch-tree-to-message-pane #'notmuch-show-previous-button))
     (define-key map (kbd "<backtab>")  (notmuch-tree-to-message-pane #'notmuch-show-previous-button))
     (define-key map (kbd "TAB") (notmuch-tree-to-message-pane #'notmuch-show-next-button))
-    (define-key map "e" (notmuch-tree-to-message-pane #'notmuch-tree-button-activate))
     (define-key map "$" (notmuch-tree-to-message-pane #'notmuch-show-toggle-process-crypto))
 
     ;; bindings from show (or elsewhere) but we close the message pane first.
-- 
2.10.2

[Patch v5 2/4] emacs: postpone a message

[David Bremner]

From: Mark Walters <markwalters1009@gmail.com>

This provides initial support for postponing in the emacs frontend;
resuming will follow in a later commit. On saving/postponing it uses
notmuch insert to put the message in the notmuch database

Current bindings are C-x C-s to save a draft, C-c C-p to postpone a
draft (save and exit compose buffer).

Previous drafts get tagged deleted on subsequent saves, or on the
message being sent.

Each draft gets its own message-id, and we use the namespace
draft-.... for draft message ids (so, at least for most people, drafts
are easily distinguisable).
---
 emacs/Makefile.local     |   3 +-
 emacs/notmuch-draft.el   | 162 +++++++++++++++++++++++++++++++++++++++++++++++
 emacs/notmuch-mua.el     |   4 ++
 test/T630-emacs-draft.sh |  42 ++++++++++++
 4 files changed, 210 insertions(+), 1 deletion(-)
 create mode 100644 emacs/notmuch-draft.el
 create mode 100755 test/T630-emacs-draft.sh

diff --git a/emacs/Makefile.local b/emacs/Makefile.local
index 2d6aedb..6896ff9 100644
--- a/emacs/Makefile.local
+++ b/emacs/Makefile.local
@@ -20,7 +20,8 @@ emacs_sources := \
 	$(dir)/notmuch-print.el \
 	$(dir)/notmuch-version.el \
 	$(dir)/notmuch-jump.el \
-	$(dir)/notmuch-company.el
+	$(dir)/notmuch-company.el \
+	$(dir)/notmuch-draft.el
 
 $(dir)/notmuch-version.el: $(dir)/Makefile.local version.stamp
 $(dir)/notmuch-version.el: $(srcdir)/$(dir)/notmuch-version.el.tmpl
diff --git a/emacs/notmuch-draft.el b/emacs/notmuch-draft.el
new file mode 100644
index 0000000..11d906b
--- /dev/null
+++ b/emacs/notmuch-draft.el
@@ -0,0 +1,162 @@
+;;; notmuch-draft.el --- functions for postponing and editing drafts
+;;
+;; Copyright © Mark Walters
+;; Copyright © David Bremner
+;;
+;; This file is part of Notmuch.
+;;
+;; Notmuch is free software: you can redistribute it and/or modify it
+;; under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+;;
+;; Notmuch is distributed in the hope that it will be useful, but
+;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+;; General Public License for more details.
+;;
+;; You should have received a copy of the GNU General Public License
+;; along with Notmuch.  If not, see <https://www.gnu.org/licenses/>.
+;;
+;; Authors: Mark Walters <markwalters1009@gmail.com>
+;;	    David Bremner <david@tethera.net>
+
+;;; Code:
+
+(require 'notmuch-maildir-fcc)
+
+(declare-function notmuch-show-get-message-id "notmuch-show" (&optional bare))
+
+(defgroup notmuch-draft nil
+  "Saving and editing drafts in Notmuch."
+  :group 'notmuch)
+
+(defcustom notmuch-draft-tags '("+draft")
+  "List of tags changes to apply to a draft message when it is saved in the database.
+
+Tags starting with \"+\" (or not starting with either \"+\" or
+\"-\") in the list will be added, and tags starting with \"-\"
+will be removed from the message being stored.
+
+For example, if you wanted to give the message a \"draft\" tag
+but not the (normally added by default) \"inbox\" tag, you would
+set:
+    (\"+draft\" \"-inbox\")"
+  :type '(repeat string)
+  :group 'notmuch-draft)
+
+(defcustom notmuch-draft-folder "drafts"
+  "Folder to save draft messages in.
+
+This should be specified relative to the root of the notmuch
+database. It will be created if necessary."
+  :type 'string
+  :group 'notmuch-draft)
+
+(defcustom notmuch-draft-quoted-tags '()
+  "Mml tags to quote.
+
+This should be a list of mml tags to quote before saving. You do
+not need to include \"secure\" as that is handled separately.
+
+If you include \"part\" then attachments will not be saved with
+the draft -- if not then they will be saved with the draft. The
+former means the attachments may not still exist when you resume
+the message, the latter means that the attachments as they were
+when you postponed will be sent with the resumed message.
+
+Note you may get strange results if you change this between
+postponing and resuming a message."
+  :type '(repeat string)
+  :group 'notmuch-send)
+
+(defvar notmuch-draft-id nil
+  "Message-id of the most recent saved draft of this message")
+(make-variable-buffer-local 'notmuch-draft-id)
+
+(defun notmuch-draft--mark-deleted ()
+  "Tag the last saved draft deleted.
+
+Used when a new version is saved, or the message is sent."
+  (when notmuch-draft-id
+    (notmuch-tag notmuch-draft-id '("+deleted"))))
+
+(defun notmuch-draft-quote-some-mml ()
+  "Quote the mml tags in `notmuch-draft-quoted-tags`."
+  (save-excursion
+    ;; First we deal with any secure tag separately.
+    (message-goto-body)
+    (when (looking-at "<#secure[^\n]*>\n")
+      (let ((secure-tag (match-string 0)))
+	(delete-region (match-beginning 0) (match-end 0))
+	(message-add-header (concat "X-Notmuch-Emacs-Secure: " secure-tag))))
+    ;; This is copied from mml-quote-region but only quotes the
+    ;; specified tags.
+    (when notmuch-draft-quoted-tags
+      (let ((re (concat "<#!*/?\\("
+			(mapconcat 'identity notmuch-draft-quoted-tags "\\|")
+			"\\)")))
+	(message-goto-body)
+	(while (re-search-forward re nil t)
+	  ;; Insert ! after the #.
+	  (goto-char (+ (match-beginning 0) 2))
+	  (insert "!"))))))
+
+(defun notmuch-draft-save ()
+  "Save the current draft message in the notmuch database.
+
+This saves the current message in the database with tags
+`notmuch-draft-tags` (in addition to any default tags
+applied to newly inserted messages)."
+  (interactive)
+  (let (;; We need the message id as we need it for tagging. Note
+	;; message-make-message-id gives the id inside a "<" ">" pair,
+	;; but notmuch doesn't want that form, so remove them.
+	(id (concat "draft-" (substring (message-make-message-id) 1 -1))))
+    (with-temporary-notmuch-message-buffer
+     ;; We insert a Date header and a Message-ID header, the former
+     ;; so that it is easier to search for the message, and the
+     ;; latter so we have a way of accessing the saved message (for
+     ;; example to delete it at a later time). We check that the
+     ;; user has these in `message-deletable-headers` (the default)
+     ;; as otherwise they are doing something strange and we
+     ;; shouldn't interfere. Note, since we are doing this in a new
+     ;; buffer we don't change the version in the compose buffer.
+     (if (member 'Message-ID message-deletable-headers)
+	 (progn
+	   (message-remove-header "Message-ID")
+	   (message-add-header (concat "Message-ID: <" id ">")))
+       (message "You have customized emacs so Message-ID is not a deletable header, so not changing it")
+       (setq id nil))
+     (if (member 'Date message-deletable-headers)
+	 (progn
+	   (message-remove-header "Date")
+	   (message-add-header (concat "Date: " (message-make-date))))
+       (message "You have customized emacs so Date is not a deletable header, so not changing it"))
+     (message-add-header "X-Notmuch-Emacs-Draft: True")
+     (notmuch-draft-quote-some-mml)
+     (notmuch-maildir-setup-message-for-saving)
+     (notmuch-maildir-notmuch-insert-current-buffer
+      notmuch-draft-folder 't notmuch-draft-tags))
+    ;; We are now back in the original compose buffer. Note the
+    ;; function notmuch-call-notmuch-process (called by
+    ;; notmuch-maildir-notmuch-insert-current-buffer) signals an error
+    ;; on failure, so to get to this point it must have
+    ;; succeeded. Also, notmuch-draft-id is still the id of the
+    ;; previous draft, so it is safe to mark it deleted.
+    (notmuch-draft--mark-deleted)
+    (setq notmuch-draft-id (concat "id:" id))
+    (set-buffer-modified-p nil)))
+
+(defun notmuch-draft-postpone ()
+  "Save the draft message in the notmuch database and exit buffer."
+  (interactive)
+  (notmuch-draft-save-draft)
+  (kill-buffer))
+
+(add-hook 'message-send-hook 'notmuch-draft--mark-deleted)
+
+
+(provide 'notmuch-draft)
+
+;;; notmuch-draft.el ends here
diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index f333655..b68cdf2 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -33,6 +33,8 @@
 (declare-function notmuch-show-insert-body "notmuch-show" (msg body depth))
 (declare-function notmuch-fcc-header-setup "notmuch-maildir-fcc" ())
 (declare-function notmuch-maildir-message-do-fcc "notmuch-maildir-fcc" ())
+(declare-function notmuch-draft-postpone "notmuch-draft" ())
+(declare-function notmuch-draft-save "notmuch-draft" ())
 
 ;;
 
@@ -289,6 +291,8 @@ mutiple parts get a header."
 
 (define-key notmuch-message-mode-map (kbd "C-c C-c") #'notmuch-mua-send-and-exit)
 (define-key notmuch-message-mode-map (kbd "C-c C-s") #'notmuch-mua-send)
+(define-key notmuch-message-mode-map (kbd "C-c C-p") #'notmuch-draft-postpone)
+(define-key notmuch-message-mode-map (kbd "C-x C-s") #'notmuch-draft-save)
 
 (defun notmuch-mua-pop-to-buffer (name switch-function)
   "Pop to buffer NAME, and warn if it already exists and is
diff --git a/test/T630-emacs-draft.sh b/test/T630-emacs-draft.sh
new file mode 100755
index 0000000..e39690c
--- /dev/null
+++ b/test/T630-emacs-draft.sh
@@ -0,0 +1,42 @@
+#!/usr/bin/env bash
+test_description="Emacs Draft Handling"
+. ./test-lib.sh || exit 1
+
+add_email_corpus
+
+notmuch config set search.exclude_tags deleted
+
+test_begin_subtest "Saving a draft indexes it"
+test_emacs '(notmuch-mua-mail)
+	    (message-goto-subject)
+	    (insert "draft-test-0001")
+	    (notmuch-draft-save)
+	    (test-output)'
+count1=$(notmuch count tag:draft)
+count2=$(notmuch count subject:draft-test-0001)
+test_expect_equal "$count1=$count2" "1=1"
+
+test_begin_subtest "Saving a draft tags previous draft as deleted"
+test_emacs '(notmuch-mua-mail)
+	    (message-goto-subject)
+	    (insert "draft-test-0002")
+	    (notmuch-draft-save)
+	    (notmuch-draft-save)
+	    (test-output)'
+count1=$(notmuch count tag:draft)
+count2=$(notmuch count subject:draft-test-0002)
+
+test_expect_equal "$count1,$count2" "2,1"
+
+test_begin_subtest "Saving a signed draft adds header"
+test_emacs '(notmuch-mua-mail)
+	    (message-goto-subject)
+	    (insert "draft-test-0003")
+	    (mml-secure-message-sign)
+	    (notmuch-draft-save)
+	    (test-output)'
+header_count=$(notmuch show --format=raw subject:draft-test-0003 | grep -c ^X-Notmuch-Emacs-Secure)
+body_count=$(notmuch notmuch show --format=raw subject:draft-test-0003 | grep -c '^\<#secure')
+test_expect_equal "$header_count,$body_count" "1,0"
+
+test_done
-- 
2.10.2

[Patch v5 3/4] emacs: check drafts for encryption tags before saving

[David Bremner]

In general the user may not want to save plaintext copies of messages
that they are sending encrypted, so give them a chance to abort.
---
 emacs/notmuch-draft.el   | 40 ++++++++++++++++++++++++++++++++++++++++
 test/T630-emacs-draft.sh | 13 +++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/emacs/notmuch-draft.el b/emacs/notmuch-draft.el
index 11d906b..5a230e8 100644
--- a/emacs/notmuch-draft.el
+++ b/emacs/notmuch-draft.el
@@ -70,6 +70,21 @@ postponing and resuming a message."
   :type '(repeat string)
   :group 'notmuch-send)
 
+(defcustom notmuch-draft-save-plaintext 'ask
+  "Should notmuch save/postpone in plaintext messages that seem
+  like they are intended to be sent encrypted
+(i.e with an mml encryption tag in it)."
+  :type '(radio
+	  (const :tag "Never" nil)
+	  (const :tag "Ask every time" ask)
+	  (const :tag "Always" t))
+  :group 'notmuch-draft
+  :group 'notmuch-crypto)
+
+(defvar notmuch-draft-encryption-tag-regex
+  "<#\\(part encrypt\\|secure.*mode=.*encrypt>\\)"
+  "Regular expression matching mml tags indicating encryption of part or message")
+
 (defvar notmuch-draft-id nil
   "Message-id of the most recent saved draft of this message")
 (make-variable-buffer-local 'notmuch-draft-id)
@@ -102,6 +117,22 @@ Used when a new version is saved, or the message is sent."
 	  (goto-char (+ (match-beginning 0) 2))
 	  (insert "!"))))))
 
+(defun notmuch-draft--check-encryption-tag (&optional ask)
+  "Query user if there an mml tag that looks like it might indicate encryption.
+
+Returns t if there is no such tag, or the user confirms they mean
+it."
+  (save-excursion
+    (message-goto-body)
+      (or
+       ;; We are fine if no relevant tag is found, or
+       (not (re-search-forward notmuch-draft-encryption-tag-regex nil 't))
+       ;; The user confirms they means it.
+       (and ask
+	    (yes-or-no-p "(Customize `notmuch-draft-save-plaintext' to avoid this warning)
+This message contains mml tags that suggest it is intended to be encrypted.
+Really save and index an unencrypted copy? ")))))
+
 (defun notmuch-draft-save ()
   "Save the current draft message in the notmuch database.
 
@@ -109,6 +140,15 @@ This saves the current message in the database with tags
 `notmuch-draft-tags` (in addition to any default tags
 applied to newly inserted messages)."
   (interactive)
+  (case notmuch-draft-save-plaintext
+    ((ask)
+     (unless (notmuch-draft--check-encryption-tag t)
+       (error "Save aborted")))
+    ((t)
+     (ignore))
+    ((nil)
+     (unless (notmuch-draft--check-encryption-tag nil)
+       (error "Refusing to save draft with encryption tags (see `notmuch-draft-save-plaintext')"))))
   (let (;; We need the message id as we need it for tagging. Note
 	;; message-make-message-id gives the id inside a "<" ">" pair,
 	;; but notmuch doesn't want that form, so remove them.
diff --git a/test/T630-emacs-draft.sh b/test/T630-emacs-draft.sh
index e39690c..689ccfb 100755
--- a/test/T630-emacs-draft.sh
+++ b/test/T630-emacs-draft.sh
@@ -39,4 +39,17 @@ header_count=$(notmuch show --format=raw subject:draft-test-0003 | grep -c ^X-No
 body_count=$(notmuch notmuch show --format=raw subject:draft-test-0003 | grep -c '^\<#secure')
 test_expect_equal "$header_count,$body_count" "1,0"
 
+test_begin_subtest "Refusing to save an encrypted draft"
+test_emacs '(notmuch-mua-mail)
+	    (message-goto-subject)
+	    (insert "draft-test-0004")
+	    (mml-secure-message-sign-encrypt)
+	    (let ((notmuch-draft-save-plaintext nil))
+		     (notmuch-draft-save))
+	    (test-output)'
+count1=$(notmuch count tag:draft)
+count2=$(notmuch count subject:draft-test-0004)
+
+test_expect_equal "$count1,$count2" "3,0"
+
 test_done
-- 
2.10.2

[Patch v5 4/4] emacs: resume messages

[David Bremner]

Provide functionality to resume editing a mesage previously saved with
notmuch-draft-save, including decoding the X-Notmuch-Emacs-Secure
header.

Resume gets the raw file from notmuch and using the emacs function
mime-to-mml reconstructs the message (including attachments).

'e' is bound to resume a draft from show or tree mode.
---
 emacs/notmuch-draft.el   | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
 emacs/notmuch-show.el    | 10 ++++++++
 emacs/notmuch-tree.el    |  1 +
 test/T630-emacs-draft.sh | 15 ++++++++++++
 4 files changed, 87 insertions(+)

diff --git a/emacs/notmuch-draft.el b/emacs/notmuch-draft.el
index 5a230e8..1528d79 100644
--- a/emacs/notmuch-draft.el
+++ b/emacs/notmuch-draft.el
@@ -117,6 +117,27 @@ Used when a new version is saved, or the message is sent."
 	  (goto-char (+ (match-beginning 0) 2))
 	  (insert "!"))))))
 
+(defun notmuch-draft-unquote-some-mml ()
+  "Unquote the mml tags in `notmuch-draft-quoted-tags`."
+  (save-excursion
+    (when notmuch-draft-quoted-tags
+      (let ((re (concat "<#!+/?\\("
+			(mapconcat 'identity notmuch-draft-quoted-tags "\\|")
+			"\\)")))
+	(message-goto-body)
+	(while (re-search-forward re nil t)
+	  ;; Remove one ! from after the #.
+	  (goto-char (+ (match-beginning 0) 2))
+	  (delete-char 1))))
+    (let (secure-tag)
+      (save-restriction
+	(message-narrow-to-headers)
+	(setq secure-tag (message-fetch-field "X-Notmuch-Emacs-Secure" 't))
+	(message-remove-header "X-Notmuch-Emacs-Secure"))
+      (message-goto-body)
+      (when secure-tag
+	(insert secure-tag "\n")))))
+
 (defun notmuch-draft--check-encryption-tag (&optional ask)
   "Query user if there an mml tag that looks like it might indicate encryption.
 
@@ -194,6 +215,46 @@ applied to newly inserted messages)."
   (notmuch-draft-save-draft)
   (kill-buffer))
 
+(defun notmuch-draft-resume (id)
+  "Resume editing of message with id ID."
+  (let* ((tags (process-lines notmuch-command "search" "--output=tags"
+			      "--exclude=false" id))
+	 (draft (equal tags (notmuch-update-tags tags notmuch-draft-tags))))
+    (when (or draft
+	      (yes-or-no-p "Message does not appear to be a draft: really resume? "))
+      (switch-to-buffer (get-buffer-create (concat "*notmuch-draft-" id "*")))
+      (setq buffer-read-only nil)
+      (erase-buffer)
+      (let ((coding-system-for-read 'no-conversion))
+	(call-process notmuch-command nil t nil "show" "--format=raw" id))
+      (mime-to-mml)
+      (goto-char (point-min))
+      (when (re-search-forward "^$" nil t)
+	(replace-match mail-header-separator t t))
+      ;; Remove the Date and Message-ID headers (unless the user has
+      ;; explicitly customized emacs to tell us not to) as they will
+      ;; be replaced when the message is sent.
+      (save-restriction
+	(message-narrow-to-headers)
+	(when (member 'Message-ID message-deletable-headers)
+	  (message-remove-header "Message-ID"))
+	(when (member 'Date message-deletable-headers)
+	  (message-remove-header "Date"))
+	;; The X-Notmuch-Emacs-Draft header is a more reliable
+	;; indication of whether the message really is a draft.
+	(setq draft (> (message-remove-header "X-Notmuch-Emacs-Draft") 0)))
+      ;; If the message is not a draft we should not unquote any mml.
+      (when draft
+	(notmuch-draft-unquote-some-mml))
+      (notmuch-message-mode)
+      (message-goto-body)
+      (set-buffer-modified-p nil)
+      ;; If the resumed message was a draft then set the draft
+      ;; message-id so that we can delete the current saved draft if the
+      ;; message is resaved or sent.
+      (setq notmuch-draft-id (when draft id)))))
+
+
 (add-hook 'message-send-hook 'notmuch-draft--mark-deleted)
 
 
diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index fcf7e6e..79e4435 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -38,6 +38,7 @@
 (require 'notmuch-mua)
 (require 'notmuch-crypto)
 (require 'notmuch-print)
+(require 'notmuch-draft)
 
 (declare-function notmuch-call-notmuch-process "notmuch" (&rest args))
 (declare-function notmuch-search-next-thread "notmuch" nil)
@@ -50,6 +51,7 @@
 		  (&optional query query-context target buffer-name open-target))
 (declare-function notmuch-tree-get-message-properties "notmuch-tree" nil)
 (declare-function notmuch-read-query "notmuch" (prompt))
+(declare-function notmuch-draft-resume "notmuch-draft" (id))
 
 (defcustom notmuch-message-headers '("Subject" "To" "Cc" "Date")
   "Headers that should be shown in a message, in this order.
@@ -1445,6 +1447,7 @@ reset based on the original query."
     (define-key map "|" 'notmuch-show-pipe-message)
     (define-key map "w" 'notmuch-show-save-attachments)
     (define-key map "V" 'notmuch-show-view-raw-message)
+    (define-key map "e" 'notmuch-show-resume-message)
     (define-key map "c" 'notmuch-show-stash-map)
     (define-key map "h" 'notmuch-show-toggle-visibility-headers)
     (define-key map "k" 'notmuch-tag-jump)
@@ -1982,6 +1985,13 @@ to show, nil otherwise."
     (setq buffer-read-only t)
     (view-buffer buf 'kill-buffer-if-not-modified)))
 
+(defun notmuch-show-resume-message ()
+  "Resume EDITING the current draft message."
+  (interactive)
+  (let ((id (notmuch-show-get-message-id)))
+    (when id
+      (notmuch-draft-resume id))))
+
 (put 'notmuch-show-pipe-message 'notmuch-doc
      "Pipe the contents of the current message to a command.")
 (put 'notmuch-show-pipe-message 'notmuch-prefix-doc
diff --git a/emacs/notmuch-tree.el b/emacs/notmuch-tree.el
index 8398eb1..4abcf60 100644
--- a/emacs/notmuch-tree.el
+++ b/emacs/notmuch-tree.el
@@ -273,6 +273,7 @@ FUNC."
     (define-key map "r" (notmuch-tree-close-message-pane-and #'notmuch-show-reply-sender))
     (define-key map "R" (notmuch-tree-close-message-pane-and #'notmuch-show-reply))
     (define-key map "V" (notmuch-tree-close-message-pane-and #'notmuch-show-view-raw-message))
+    (define-key map "e" (notmuch-tree-close-message-pane-and #'notmuch-show-resume-message))
 
     ;; The main tree view bindings
     (define-key map (kbd "RET") 'notmuch-tree-show-message)
diff --git a/test/T630-emacs-draft.sh b/test/T630-emacs-draft.sh
index 689ccfb..46fc356 100755
--- a/test/T630-emacs-draft.sh
+++ b/test/T630-emacs-draft.sh
@@ -52,4 +52,19 @@ count2=$(notmuch count subject:draft-test-0004)
 
 test_expect_equal "$count1,$count2" "3,0"
 
+test_begin_subtest "Resuming a signed draft"
+
+test_emacs '(notmuch-show "subject:draft-test-0003")
+	    (notmuch-show-resume-message)
+	    (test-output)'
+notmuch_dir_sanitize OUTPUT > OUTPUT.clean
+cat <<EOF | notmuch_dir_sanitize >EXPECTED
+From: Notmuch Test Suite <test_suite@notmuchmail.org>
+To: 
+Subject: draft-test-0003
+Fcc: MAIL_DIR/sent
+--text follows this line--
+<#secure method=pgpmime mode=sign>
+EOF
+test_expect_equal_file EXPECTED OUTPUT.clean
 test_done
-- 
2.10.2

Re: [Patch v5 3/4] emacs: check drafts for encryption tags before saving

[Mark Walters]

On Mon, 07 Nov 2016, David Bremner <david@tethera.net> wrote:
> In general the user may not want to save plaintext copies of messages
> that they are sending encrypted, so give them a chance to abort.
> ---
>  emacs/notmuch-draft.el   | 40 ++++++++++++++++++++++++++++++++++++++++
>  test/T630-emacs-draft.sh | 13 +++++++++++++
>  2 files changed, 53 insertions(+)
>
> diff --git a/emacs/notmuch-draft.el b/emacs/notmuch-draft.el
> index 11d906b..5a230e8 100644
> --- a/emacs/notmuch-draft.el
> +++ b/emacs/notmuch-draft.el
> @@ -70,6 +70,21 @@ postponing and resuming a message."
>    :type '(repeat string)
>    :group 'notmuch-send)
>  
> +(defcustom notmuch-draft-save-plaintext 'ask
> +  "Should notmuch save/postpone in plaintext messages that seem
> +  like they are intended to be sent encrypted
> +(i.e with an mml encryption tag in it)."
> +  :type '(radio
> +	  (const :tag "Never" nil)
> +	  (const :tag "Ask every time" ask)
> +	  (const :tag "Always" t))
> +  :group 'notmuch-draft
> +  :group 'notmuch-crypto)
> +
> +(defvar notmuch-draft-encryption-tag-regex
> +  "<#\\(part encrypt\\|secure.*mode=.*encrypt>\\)"
> +  "Regular expression matching mml tags indicating encryption of part or message")
> +
>  (defvar notmuch-draft-id nil
>    "Message-id of the most recent saved draft of this message")
>  (make-variable-buffer-local 'notmuch-draft-id)
> @@ -102,6 +117,22 @@ Used when a new version is saved, or the message is sent."
>  	  (goto-char (+ (match-beginning 0) 2))
>  	  (insert "!"))))))
>  
> +(defun notmuch-draft--check-encryption-tag (&optional ask)
> +  "Query user if there an mml tag that looks like it might indicate encryption.
> +
> +Returns t if there is no such tag, or the user confirms they mean
> +it."
> +  (save-excursion
> +    (message-goto-body)
> +      (or
> +       ;; We are fine if no relevant tag is found, or
> +       (not (re-search-forward notmuch-draft-encryption-tag-regex nil 't))
> +       ;; The user confirms they means it.
> +       (and ask
> +	    (yes-or-no-p "(Customize `notmuch-draft-save-plaintext' to avoid this warning)
> +This message contains mml tags that suggest it is intended to be encrypted.
> +Really save and index an unencrypted copy? ")))))
> +
>  (defun notmuch-draft-save ()
>    "Save the current draft message in the notmuch database.
>  
> @@ -109,6 +140,15 @@ This saves the current message in the database with tags
>  `notmuch-draft-tags` (in addition to any default tags
>  applied to newly inserted messages)."
>    (interactive)
> +  (case notmuch-draft-save-plaintext
> +    ((ask)
> +     (unless (notmuch-draft--check-encryption-tag t)
> +       (error "Save aborted")))
> +    ((t)
> +     (ignore))
> +    ((nil)
> +     (unless (notmuch-draft--check-encryption-tag nil)
> +       (error "Refusing to save draft with encryption tags (see `notmuch-draft-save-plaintext')"))))

What would you think of rejigging the logic here? I would prefer that
the first check was "is there an encryption tag" and then if there is
such a tag decide what to do. The reason I prefer that is that it makes
the common case clear.

I realise there are downsides too -- eg in your code you don't even
check for secure tags if they are going to  be ignored anyway.

If you prefer your way then lets leave it as is.

Best wishes

Mark

Re: [Patch v5 2/4] emacs: postpone a message

[David Bremner]

David Bremner <david@tethera.net> writes:

> From: Mark Walters <markwalters1009@gmail.com>

This really Mark's work, that I have split out into a separate file.

> +(defcustom notmuch-draft-tags '("+draft")
> +  "List of tags changes to apply to a draft message when it is saved in the database.

Here and a few other places the documentation uses "the database" to
mean the directory hierachy containing mail messages + the xapian
database.  At some point I would like to be able to distinguish between
the database and the maildir root (which doesn't need to be maildirs)
when talking about configuration. I don't really know better terminology
here, but I thought I would mention it in case someone else is inspired.

> +(defun notmuch-draft--mark-deleted ()

This -- naming convention is my contribution. Perhaps eventually we
could mark all private functions not intended to be called by users this
way. Since it is essentially cosmetic, I didn't want to do that
now. Indeed, one could quibble about the correctness of calling a
function private and then putting it in a public hook.

> +(defun notmuch-draft-quote-some-mml ()
> +  "Quote the mml tags in `notmuch-draft-quoted-tags`."
> +  (save-excursion
> +    ;; First we deal with any secure tag separately.
> +    (message-goto-body)
> +    (when (looking-at "<#secure[^\n]*>\n")
> +      (let ((secure-tag (match-string 0)))
> +	(delete-region (match-beginning 0) (match-end 0))
> +	(message-add-header (concat "X-Notmuch-Emacs-Secure: " secure-tag))))
> +    ;; This is copied from mml-quote-region but only quotes the
> +    ;; specified tags.
> +    (when notmuch-draft-quoted-tags
> +      (let ((re (concat "<#!*/?\\("
> +			(mapconcat 'identity notmuch-draft-quoted-tags "\\|")
> +			"\\)")))
One "hidden feature" is that regex characters in the quoted tags will be
interpreted. Possibly calling regexp-quote instead of identity would be
extra cautious here?

> +(defun notmuch-draft-save ()
> +  "Save the current draft message in the notmuch database.
> +
> +This saves the current message in the database with tags
> +`notmuch-draft-tags` (in addition to any default tags
> +applied to newly inserted messages)."
> +  (interactive)
> +  (let (;; We need the message id as we need it for tagging. Note
> +	;; message-make-message-id gives the id inside a "<" ">" pair,
> +	;; but notmuch doesn't want that form, so remove them.
> +	(id (concat "draft-" (substring (message-make-message-id) 1
> -1))))

what do you think of isolating this code and commentary in a private
function?

> +     (if (member 'Message-ID message-deletable-headers)
> +	 (progn
> +	   (message-remove-header "Message-ID")
> +	   (message-add-header (concat "Message-ID: <" id ">")))
> +       (message "You have customized emacs so Message-ID is not a deletable header, so not changing it")
> +       (setq id nil))

I'm not sure if it's just me, but I find the (if (progn ...)
else-clauses) a bit off-putting. An alternative would be to use cond
     
(cond
 ((member 'Message-ID message-deletable-headers)
  (message-remove-header "Message-id")
  (message-add-header (concat "Message-ID: <" id ">")))
 (t
  (message "You have customized emacs so Message-ID is not a deletable header, so not changing it")
  (setq id nil)))


> +(add-hook 'message-send-hook 'notmuch-draft--mark-deleted)

Can we avoid this by adding some code notmuch-mua-send-common?

> --- /dev/null
> +++ b/test/T630-emacs-draft.sh
> @@ -0,0 +1,42 @@
> +#!/usr/bin/env bash
> +test_description="Emacs Draft Handling"
> +. ./test-lib.sh || exit 1

Ok, now I remember I wrote this part, so someone else should review.

Re: [Patch v5 3/4] emacs: check drafts for encryption tags before saving

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

>> @@ -109,6 +140,15 @@ This saves the current message in the database with tags
>>  `notmuch-draft-tags` (in addition to any default tags
>>  applied to newly inserted messages)."
>>    (interactive)
>> +  (case notmuch-draft-save-plaintext
>> +    ((ask)
>> +     (unless (notmuch-draft--check-encryption-tag t)
>> +       (error "Save aborted")))
>> +    ((t)
>> +     (ignore))
>> +    ((nil)
>> +     (unless (notmuch-draft--check-encryption-tag nil)
>> +       (error "Refusing to save draft with encryption tags (see `notmuch-draft-save-plaintext')"))))
>
> What would you think of rejigging the logic here? I would prefer that
> the first check was "is there an encryption tag" and then if there is
> such a tag decide what to do. The reason I prefer that is that it makes
> the common case clear.
>
> I realise there are downsides too -- eg in your code you don't even
> check for secure tags if they are going to  be ignored anyway.

OK, I was mainly concerned with not prompting the user uneccesarily, but
the following seems to be equivalent:


-  (case notmuch-draft-save-plaintext
-    ((ask)
-     (unless (notmuch-draft--check-encryption-tag t)
-       (error "Save aborted")))
-    ((t)
-     (ignore))
-    ((nil)
-     (unless (notmuch-draft--check-encryption-tag nil)
+  (unless (notmuch-draft--check-encryption-tag
+          (eq notmuch-draft-save-plaintext 'ask))
+    (case notmuch-draft-save-plaintext
+      ((ask)
+       (error "Save aborted"))
+      ((t)
+       (ignore))
+      ((nil)
        (error "Refusing to save draft with encryption tags (see `notmuch-draft-save-plaintext')"))))

Re: [Patch v5 4/4] emacs: resume messages

[David Bremner]

David Bremner <david@tethera.net> writes:

> Provide functionality to resume editing a mesage previously saved with
> notmuch-draft-save, including decoding the X-Notmuch-Emacs-Secure
> header.

s/mesage/message/

> +(defun notmuch-draft-unquote-some-mml ()
> +  "Unquote the mml tags in `notmuch-draft-quoted-tags`."
> +  (save-excursion
> +    (when notmuch-draft-quoted-tags
> +      (let ((re (concat "<#!+/?\\("
> +			(mapconcat 'identity notmuch-draft-quoted-tags "\\|")
> +
Same issue here with regex quoting, I think.

> +    (let (secure-tag)
> +      (save-restriction
> +	(message-narrow-to-headers)
> +	(setq secure-tag (message-fetch-field "X-Notmuch-Emacs-Secure" 't))
> +	(message-remove-header "X-Notmuch-Emacs-Secure"))
> +      (message-goto-body)
> +      (when secure-tag
> +	(insert secure-tag "\n")))))

Can the setq inside the let be replaced with

(let ((secure-tag (message-fetch-field "X-Notmuch-Emacs-Secure" 't)))
     ...

Perhaps by pushing the let inside the save-restriction?

>  (require 'notmuch-mua)
>  (require 'notmuch-crypto)
>  (require 'notmuch-print)
> +(require 'notmuch-draft)

This line I added.

> +(defun notmuch-show-resume-message ()
> +  "Resume EDITING the current draft message."
> +  (interactive)
> +  (let ((id (notmuch-show-get-message-id)))
> +    (when id
> +      (notmuch-draft-resume id))))
> +

The error handling is not very clear to me
here. notmuch-show-get-message-id is not documented to return nil on
error. Should some docstring be changed here?

> +<#secure method=pgpmime mode=sign>
> +EOF
> +test_expect_equal_file EXPECTED OUTPUT.clean
>  test_done

The quoting of the secure tag here is not present in the original test,
but sure confused me for a few minutes.

Re: [Patch v5 2/4] emacs: postpone a message

[Mark Walters]

On Sat, 12 Nov 2016, David Bremner <david@tethera.net> wrote:
> David Bremner <david@tethera.net> writes:
>
>> From: Mark Walters <markwalters1009@gmail.com>
>
> This really Mark's work, that I have split out into a separate file.
>
>> +(defcustom notmuch-draft-tags '("+draft")
>> +  "List of tags changes to apply to a draft message when it is saved in the database.
>
> Here and a few other places the documentation uses "the database" to
> mean the directory hierachy containing mail messages + the xapian
> database.  At some point I would like to be able to distinguish between
> the database and the maildir root (which doesn't need to be maildirs)
> when talking about configuration. I don't really know better terminology
> here, but I thought I would mention it in case someone else is inspired.

I think we could use "mailstore" for the maildir root, "database" makes
sense for the xapian database. However, I don't have a good term for the
combined whole -- so I am  not sure what would make sense in this
particular case.

>
>> +(defun notmuch-draft--mark-deleted ()
>
> This -- naming convention is my contribution. Perhaps eventually we
> could mark all private functions not intended to be called by users this
> way. Since it is essentially cosmetic, I didn't want to do that
> now. Indeed, one could quibble about the correctness of calling a
> function private and then putting it in a public hook.

I like it, and I agree. I do intend to remove it from the public hook
later (see below).

>> +(defun notmuch-draft-quote-some-mml ()
>> +  "Quote the mml tags in `notmuch-draft-quoted-tags`."
>> +  (save-excursion
>> +    ;; First we deal with any secure tag separately.
>> +    (message-goto-body)
>> +    (when (looking-at "<#secure[^\n]*>\n")
>> +      (let ((secure-tag (match-string 0)))
>> +	(delete-region (match-beginning 0) (match-end 0))
>> +	(message-add-header (concat "X-Notmuch-Emacs-Secure: " secure-tag))))
>> +    ;; This is copied from mml-quote-region but only quotes the
>> +    ;; specified tags.
>> +    (when notmuch-draft-quoted-tags
>> +      (let ((re (concat "<#!*/?\\("
>> +			(mapconcat 'identity notmuch-draft-quoted-tags "\\|")
>> +			"\\)")))
> One "hidden feature" is that regex characters in the quoted tags will be
> interpreted. Possibly calling regexp-quote instead of identity would be
> extra cautious here?

Good catch: I have made it regexp-quote as you suggest.

>> +(defun notmuch-draft-save ()
>> +  "Save the current draft message in the notmuch database.
>> +
>> +This saves the current message in the database with tags
>> +`notmuch-draft-tags` (in addition to any default tags
>> +applied to newly inserted messages)."
>> +  (interactive)
>> +  (let (;; We need the message id as we need it for tagging. Note
>> +	;; message-make-message-id gives the id inside a "<" ">" pair,
>> +	;; but notmuch doesn't want that form, so remove them.
>> +	(id (concat "draft-" (substring (message-make-message-id) 1
>> -1))))
>
> what do you think of isolating this code and commentary in a private
> function?

I have done this.

>> +     (if (member 'Message-ID message-deletable-headers)
>> +	 (progn
>> +	   (message-remove-header "Message-ID")
>> +	   (message-add-header (concat "Message-ID: <" id ">")))
>> +       (message "You have customized emacs so Message-ID is not a deletable header, so not changing it")
>> +       (setq id nil))
>
> I'm not sure if it's just me, but I find the (if (progn ...)
> else-clauses) a bit off-putting. An alternative would be to use cond
>      
> (cond
>  ((member 'Message-ID message-deletable-headers)
>   (message-remove-header "Message-id")
>   (message-add-header (concat "Message-ID: <" id ">")))
>  (t
>   (message "You have customized emacs so Message-ID is not a deletable header, so not changing it")
>   (setq id nil)))

I am happy either way, so I have made the change

>> +(add-hook 'message-send-hook 'notmuch-draft--mark-deleted)
>
> Can we avoid this by adding some code notmuch-mua-send-common?

Yes. We should do the same for notmuch-message-mark-replied. However
getting it right looks slightly non-trivial. In particular, the user can
abort sending for various reasons (eg charset things, or they have
message-confirm-send set) and we should not do the marking if they do
abort. However, message-send-and-exit will have killed the buffer so we
need to store the relevant local variables before we run that command.

Having said that, I think that message-send-hook is run before the send
anyway, so putting both of the above in notmuch-mua-send-common would
not make the situation any worse.

My inclination was to fix it properly afterwards -- but we could do the
above now as a "no worse than now" change.

Best wishes

Mark






>
>> --- /dev/null
>> +++ b/test/T630-emacs-draft.sh
>> @@ -0,0 +1,42 @@
>> +#!/usr/bin/env bash
>> +test_description="Emacs Draft Handling"
>> +. ./test-lib.sh || exit 1
>
> Ok, now I remember I wrote this part, so someone else should review.
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v5 4/4] emacs: resume messages

[Mark Walters]

On Sat, 12 Nov 2016, David Bremner <david@tethera.net> wrote:
> David Bremner <david@tethera.net> writes:
>
>> Provide functionality to resume editing a mesage previously saved with
>> notmuch-draft-save, including decoding the X-Notmuch-Emacs-Secure
>> header.
>
> s/mesage/message/
>
>> +(defun notmuch-draft-unquote-some-mml ()
>> +  "Unquote the mml tags in `notmuch-draft-quoted-tags`."
>> +  (save-excursion
>> +    (when notmuch-draft-quoted-tags
>> +      (let ((re (concat "<#!+/?\\("
>> +			(mapconcat 'identity notmuch-draft-quoted-tags "\\|")
>> +
> Same issue here with regex quoting, I think.

Yes I will change this.

>> +    (let (secure-tag)
>> +      (save-restriction
>> +	(message-narrow-to-headers)
>> +	(setq secure-tag (message-fetch-field "X-Notmuch-Emacs-Secure" 't))
>> +	(message-remove-header "X-Notmuch-Emacs-Secure"))
>> +      (message-goto-body)
>> +      (when secure-tag
>> +	(insert secure-tag "\n")))))
>
> Can the setq inside the let be replaced with
>
> (let ((secure-tag (message-fetch-field "X-Notmuch-Emacs-Secure" 't)))
>      ...
>
> Perhaps by pushing the let inside the save-restriction?

We need to insert the secure-tag in the body, so outside of the
save-restriction. However, message-fetch-field requires the message to
be narrowed to the headers. So I don't see an easy way to avoid the setq.

>
>>  (require 'notmuch-mua)
>>  (require 'notmuch-crypto)
>>  (require 'notmuch-print)
>> +(require 'notmuch-draft)
>
> This line I added.
>
>> +(defun notmuch-show-resume-message ()
>> +  "Resume EDITING the current draft message."
>> +  (interactive)
>> +  (let ((id (notmuch-show-get-message-id)))
>> +    (when id
>> +      (notmuch-draft-resume id))))

Do you mean if we are not on a message? I think this may not be needed:
I think all I intended to check was that we are on a message, but that
seems to always be true in the show buffer (unlike threads in the search
buffer).

Best wishes

Mark


>
> The error handling is not very clear to me
> here. notmuch-show-get-message-id is not documented to return nil on
> error. Should some docstring be changed here?
>
>> +<#secure method=pgpmime mode=sign>
>> +EOF
>> +test_expect_equal_file EXPECTED OUTPUT.clean
>>  test_done
>
> The quoting of the secure tag here is not present in the original test,
> but sure confused me for a few minutes.
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v5 4/4] emacs: resume messages

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> On Sat, 12 Nov 2016, David Bremner <david@tethera.net> wrote:
>>> +(defun notmuch-show-resume-message ()
>>> +  "Resume EDITING the current draft message."
>>> +  (interactive)
>>> +  (let ((id (notmuch-show-get-message-id)))
>>> +    (when id
>>> +      (notmuch-draft-resume id))))
>
>>
>> The error handling is not very clear to me
>> here. notmuch-show-get-message-id is not documented to return nil on
>> error. Should some docstring be changed here?
>
> Do you mean if we are not on a message? I think this may not be needed:
> I think all I intended to check was that we are on a message, but that
> seems to always be true in the show buffer (unlike threads in the search
> buffer).

Well, two issues:

1) it's not clear that notmuch-show-returns nil on an error. When I
tested it in a random buffer it errored because it passed nill to
notmuch-id-to-query.

2) If we're not going to resume, we'd hope some error message would be
   printed, and it might be the job of this function to print it.