Re: [notmuch] [PATCH] Support for deletion (patch included)
authorracin <racin@free.fr>
Thu, 25 Feb 2010 15:51:59 +0000 (16:51 +0100)
committerW. Trevor King <wking@tremily.us>
Fri, 7 Nov 2014 17:36:17 +0000 (09:36 -0800)
a1/12eede70ca81bb106f2065069deb91de00547a [new file with mode: 0644]

diff --git a/a1/12eede70ca81bb106f2065069deb91de00547a b/a1/12eede70ca81bb106f2065069deb91de00547a
new file mode 100644 (file)
index 0000000..f608b6c
--- /dev/null
@@ -0,0 +1,488 @@
+Return-Path: <racin@free.fr>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+       by olra.theworths.org (Postfix) with ESMTP id 6A6F3431FAE\r
+       for <notmuch@notmuchmail.org>; Thu, 25 Feb 2010 07:52:18 -0800 (PST)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -1.787\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-1.787 tagged_above=-999 required=5 tests=[AWL=0.812,\r
+       BAYES_00=-2.599] autolearn=unavailable\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+       by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+       with ESMTP id vYZiTuO94b5f for <notmuch@notmuchmail.org>;\r
+       Thu, 25 Feb 2010 07:52:15 -0800 (PST)\r
+Received: from smtp5-g21.free.fr (smtp5-g21.free.fr [212.27.42.5])\r
+       by olra.theworths.org (Postfix) with ESMTP id A63D8431FBC\r
+       for <notmuch@notmuchmail.org>; Thu, 25 Feb 2010 07:52:13 -0800 (PST)\r
+Received: from smtp5-g21.free.fr (localhost [127.0.0.1])\r
+       by smtp5-g21.free.fr (Postfix) with ESMTP id 928A5D480BF;\r
+       Thu, 25 Feb 2010 16:52:02 +0100 (CET)\r
+Received: from zimbra1-e1.priv.proxad.net (zimbra1-e1.priv.proxad.net\r
+       [172.20.243.151])\r
+       by smtp5-g21.free.fr (Postfix) with ESMTP id E61D6D4808C;\r
+       Thu, 25 Feb 2010 16:51:59 +0100 (CET)\r
+Date: Thu, 25 Feb 2010 16:51:59 +0100 (CET)\r
+From: racin@free.fr\r
+To: Carl Worth <cworth@cworth.org>\r
+Message-ID:\r
+ <1857273639.5354781267113119409.JavaMail.root@zimbra1-e1.priv.proxad.net>\r
+In-Reply-To:\r
+ <829811857.5353531267112884804.JavaMail.root@zimbra1-e1.priv.proxad.net>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain; charset=utf-8\r
+Content-Transfer-Encoding: quoted-printable\r
+X-Originating-IP: [132.166.135.72]\r
+X-Mailer: Zimbra 5.0 (ZimbraWebClient - FF3.0\r
+       (Linux)/5.0.15_GA_2815.UBUNTU8_64)\r
+X-Authenticated-User: racin@free.fr\r
+Cc: notmuch@notmuchmail.org\r
+Subject: Re: [notmuch] [PATCH] Support for deletion (patch included)\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.13\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+       <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+       <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+       <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Thu, 25 Feb 2010 15:52:18 -0000\r
+\r
+Carl: The patch in the mail has problems; apparently I have to manually add=\r
+ scissorlines to the mail for it\r
+to be processed by git-am. I thought this was automatically added. (I hate =\r
+the git UI -- nothing is consistent,\r
+concepts have different names, the definition of scissor lines is as precis=\r
+e as "A line that mainly consists of scissors (either ">8" or "8<") and per=\r
+foration (dash "-") --, but I guess we can get used to it after a while...)\r
+\r
+I'll send you a proper patch as soon as I can. Meanwhile, I'm sure you have=\r
+ comments on this updated patch!\r
+\r
+Matthieu\r
+\r
+=C3=80: "Carl Worth" <cworth@cworth.org>\r
+Cc: notmuch@notmuchmail.org\r
+Envoy=C3=A9: Jeudi 25 F=C3=A9vrier 2010 00h00:04 GMT +00:00 GMT - Grande-Br=\r
+etagne, Irlande, Portugal\r
+Objet: Re: [notmuch] [PATCH] Support for deletion (patch included)\r
+\r
+Hi Carl,\r
+\r
+> Could you also write a commit message describing what the patch does?\r
+> The easiest way for me to apply that would be if you would create a git\r
+> commit, then run "git format-patch origin/master" and mail the resulting\r
+> files, (the "git send-email" command can be used here, or you can insert\r
+> the files into a mail-composition buffer and modify them as needed).\r
+>=20\r
+\r
+OK, here it is (comments below). I had trouble splitting the patches into a=\r
+ patch series; I\r
+found git add -p, but isn't there a better interface for selecting patches?\r
+\r
+>From bdee9558d93bffb97c80632f522288e059deb7c2 Mon Sep 17 00:00:00 2001\r
+From: Matthieu Lemerre <racin@racin.rez-gif.supelec.fr>\r
+Date: Thu, 25 Feb 2010 00:24:24 +0100\r
+Subject: [PATCH 1/2] Add and use notmuch-show-forall-in-thread macro\r
+\r
+---\r
+ notmuch.el |   17 +++++++++++------\r
+ 1 files changed, 11 insertions(+), 6 deletions(-)\r
+\r
+diff --git a/notmuch.el b/notmuch.el\r
+index 6482170..5d7342a 100644\r
+--- a/notmuch.el\r
++++ b/notmuch.el\r
+@@ -321,17 +321,22 @@ pseudoheader summary"\r
+ =09=09=09 (cons (notmuch-show-get-message-id) nil)))\r
+ =09  (notmuch-show-set-tags (sort (set-difference tags toremove :test 'str=\r
+ing=3D) 'string<))))))\r
+=20\r
+-(defun notmuch-show-archive-thread-maybe-mark-read (markread)\r
+-  (save-excursion\r
++(defmacro notmuch-show-forall-in-thread (&rest body)\r
++  "Executes BODY with point in all messages of the current thread."\r
++  `(save-excursion\r
+     (goto-char (point-min))\r
+     (while (not (eobp))\r
+-      (if markread\r
+-=09  (notmuch-show-remove-tag "unread" "inbox")\r
+-=09(notmuch-show-remove-tag "inbox"))\r
++      ,@body\r
+       (if (not (eobp))\r
+ =09  (forward-char))\r
+       (if (not (re-search-forward notmuch-show-message-begin-regexp nil t)=\r
+)\r
+-=09  (goto-char (point-max)))))\r
++=09  (goto-char (point-max))))))\r
++\r
++(defun notmuch-show-archive-thread-maybe-mark-read (markread)\r
++  (notmuch-show-forall-in-thread\r
++      (if markread\r
++=09  (notmuch-show-remove-tag "unread" "inbox")\r
++=09(notmuch-show-remove-tag "inbox")))\r
+   (let ((parent-buffer notmuch-show-parent-buffer))\r
+     (kill-this-buffer)\r
+     (if parent-buffer\r
+--=20\r
+1.6.5\r
+\r
+\r
+This first patch is helpful for factorizing out code. Basically, it allows =\r
+to\r
+apply a "message-only" command to all the thread.\r
+\r
+>From 0073152e3fa7dd11d88de28e87eec7762cdbbbeb Mon Sep 17 00:00:00 2001\r
+From: Matthieu Lemerre <racin@racin.rez-gif.supelec.fr>\r
+Date: Thu, 25 Feb 2010 00:25:51 +0100\r
+Subject: [PATCH 2/2] Add support for deletion in the emacs interface\r
+\r
+---\r
+ notmuch.el |   56 +++++++++++++++++++++++++++++++++++++++++++++++++-------\r
+ 1 files changed, 49 insertions(+), 7 deletions(-)\r
+\r
+diff --git a/notmuch.el b/notmuch.el\r
+index 5d7342a..0285573 100644\r
+--- a/notmuch.el\r
++++ b/notmuch.el\r
+@@ -92,6 +92,8 @@\r
+     (define-key map "x" 'notmuch-show-archive-thread-then-exit)\r
+     (define-key map "A" 'notmuch-show-mark-read-then-archive-thread)\r
+     (define-key map "a" 'notmuch-show-archive-thread)\r
++    (define-key map "d" 'notmuch-show-delete-thread)\r
++    (define-key map "D" 'notmuch-show-delete-message)\r
+     (define-key map "p" 'notmuch-show-previous-message)\r
+     (define-key map "N" 'notmuch-show-mark-read-then-next-open-message)\r
+     (define-key map "n" 'notmuch-show-next-message)\r
+@@ -380,6 +382,23 @@ buffer."\r
+   (notmuch-show-archive-thread)\r
+   (kill-this-buffer))\r
+=20\r
++(defun notmuch-show-delete-message ()\r
++  "Delete current message (sets its deleted tag)."\r
++  (interactive)\r
++  (notmuch-show-add-tag "deleted"))\r
++\r
++(defun notmuch-show-delete-thread()\r
++  "Delete each message in thread."\r
++  (interactive)\r
++  (notmuch-show-forall-in-thread\r
++   (notmuch-show-delete-message)))\r
++\r
++(defun notmuch-show-delete-thread-and-exit()\r
++  "Delete each message in thread, then exit back to search results."\r
++  (interactive)\r
++  (notmuch-show-delete-thread)\r
++  (kill-this-buffer))\r
++\r
+ (defun notmuch-show-mark-read-then-archive-then-exit ()\r
+   "Remove unread tags from thread, then archive and exit to search results=\r
+."\r
+   (interactive)\r
+@@ -1227,6 +1246,7 @@ matching this search term are shown if non-nil. "\r
+     (define-key map [mouse-1] 'notmuch-search-show-thread)\r
+     (define-key map "*" 'notmuch-search-operate-all)\r
+     (define-key map "a" 'notmuch-search-archive-thread)\r
++    (define-key map "d" 'notmuch-search-delete-thread)\r
+     (define-key map "-" 'notmuch-search-remove-tag)\r
+     (define-key map "+" 'notmuch-search-add-tag)\r
+     (define-key map (kbd "RET") 'notmuch-search-show-thread)\r
+@@ -1235,6 +1255,7 @@ matching this search term are shown if non-nil. "\r
+ (fset 'notmuch-search-mode-map notmuch-search-mode-map)\r
+=20\r
+ (defvar notmuch-search-query-string)\r
++(defvar notmuch-search-history nil)\r
+ (defvar notmuch-search-oldest-first t\r
+   "Show the oldest mail first in the search-mode")\r
+=20\r
+@@ -1446,6 +1467,13 @@ This function advances the next thread when finished=\r
+."\r
+   (notmuch-search-remove-tag "inbox")\r
+   (forward-line))\r
+=20\r
++(defun notmuch-search-delete-thread ()\r
++  "Mark the currently selected thread as deleted (set its \"deleted\" tag)=\r
+.\r
++This function advances the next thread when finished."\r
++  (interactive)\r
++  (notmuch-search-add-tag "deleted")\r
++  (forward-line))\r
++\r
+ (defun notmuch-search-process-sentinel (proc msg)\r
+   "Add a message to let user know when \"notmuch search\" exits"\r
+   (let ((buffer (process-buffer proc))\r
+@@ -1520,10 +1548,22 @@ characters as well as `_.+-'.\r
+ =09   (append action-split (list notmuch-search-query-string) nil))))\r
+=20\r
+ ;;;###autoload\r
+-(defun notmuch-search (query &optional oldest-first)\r
+-  "Run \"notmuch search\" with the given query string and display results.=\r
+"\r
+-  (interactive "sNotmuch search: ")\r
+-  (let ((buffer (get-buffer-create (concat "*notmuch-search-" query "*")))=\r
+)\r
++(defun notmuch-search (query &optional oldest-first include-deleted)\r
++  "Run \"notmuch search\" with the given query string and display results.\r
++\r
++With prefix argument, include deleted items.\r
++"\r
++  (interactive (let* ((prefix current-prefix-arg)\r
++=09=09      (query (if prefix\r
++=09=09=09=09 (read-string "Notmuch search (including deleted): "\r
++=09=09=09=09=09      notmuch-search-query-string\r
++=09=09=09=09=09      'notmuch-search-history)\r
++=09=09=09       (read-string "Notmuch search: " nil\r
++=09=09=09=09=09    'notmuch-search-history))))\r
++=09=09 (list query nil prefix)))\r
++  (let ((real-query (if include-deleted query=20\r
++=09=09      (concat "not tag:deleted and (" query ")")))\r
++=09(buffer (get-buffer-create (concat "*notmuch-search-" query "*"))))\r
+     (switch-to-buffer buffer)\r
+     (notmuch-search-mode)\r
+     (set 'notmuch-search-query-string query)\r
+@@ -1539,7 +1579,7 @@ characters as well as `_.+-'.\r
+ =09(let ((proc (start-process-shell-command\r
+ =09=09     "notmuch-search" buffer notmuch-command "search"\r
+ =09=09     (if oldest-first "--sort=3Doldest-first" "--sort=3Dnewest-first=\r
+")\r
+-=09=09     (shell-quote-argument query))))\r
++=09=09     (shell-quote-argument real-query))))\r
+ =09  (set-process-sentinel proc 'notmuch-search-process-sentinel)\r
+ =09  (set-process-filter proc 'notmuch-search-process-filter))))\r
+     (run-hooks 'notmuch-search-hook)))\r
+@@ -1587,7 +1627,7 @@ search."\r
+=20\r
+ Runs a new search matching only messages that match both the\r
+ current search results AND the additional query string provided."\r
+-  (interactive "sFilter search: ")\r
++  (interactive "sFilter search:")\r
+   (let ((grouped-query (if (string-match-p notmuch-search-disjunctive-rege=\r
+xp query) (concat "( " query " )") query)))\r
+     (notmuch-search (concat notmuch-search-query-string " and " grouped-qu=\r
+ery) notmuch-search-oldest-first)))\r
+=20\r
+@@ -1630,7 +1670,9 @@ current search results AND that are tagged with the g=\r
+iven tag."\r
+=20\r
+ (fset 'notmuch-folder-mode-map notmuch-folder-mode-map)\r
+=20\r
+-(defcustom notmuch-folders (quote (("inbox" . "tag:inbox") ("unread" . "ta=\r
+g:unread")))\r
++(defcustom notmuch-folders (quote (("inbox" . "tag:inbox")=20\r
++=09=09=09=09   ("unread" . "tag:unread")\r
++=09=09=09=09   ("deleted" . "tag:deleted")))\r
+   "List of searches for the notmuch folder view"\r
+   :type '(alist :key-type (string) :value-type (string))\r
+   :group 'notmuch)\r
+--=20\r
+1.6.5\r
+\r
+\r
+This second patch is the reworked patch, after your comments.\r
+\r
+> A couple of minor comments on the patch:\r
+>=20\r
+> >      (define-key map "a" 'notmuch-search-archive-thread)\r
+> > +    (define-key map "d" 'notmuch-search-mark-as-deleted)\r
+>=20\r
+> For consistency, let's name this notmuch-search-delete-thread.\r
+>=20\r
+> And we'll probably want a notmuch-show-delete-message function as well,\r
+> no?\r
+\r
+Did both. I have however a question: how to handle in the UI the\r
+thread/message distinction? For now, I set "d" in notmuch-show buffer\r
+to act as "delete the current thread", and "D" as "delete the current\r
+message". I thought that as "d" meant in notmuch summary "delete the\r
+current thread", the bindings would be more consistent.  But now I\r
+wonder if this may be counter intuitive, and if we should use "d" for\r
+"delete current message" and "D" for "delete current thread". What do\r
+people think?\r
+\r
+It would be nice to have, in general a convenient way to handle the\r
+thread/message distinction in the UI (e.g. for applying other tags).\r
+\r
+>=20\r
+> > +(defvar notmuch-search-history nil)\r
+>=20\r
+> Excellent! I've wanted custom search history for a while, and just\r
+> didn't know how to do it with (interactive "s"). It looks easy enough\r
+> with read-string as you're doing here. But this is independent\r
+> functionality, so would be preferred as an independent patch/commit.\r
+\r
+In fact I needed this; that way when you do "C-u s", the previous\r
+search is shown, and to include deleted items you just have to do "C-u\r
+s RET".  So I made them in a single commit; but I can split them if\r
+you really want to.\r
+\r
+>=20\r
+> >    (forward-line))\r
+> > =20\r
+> > +\r
+> > +(defun notmuch-search-mark-as-deleted ()\r
+> > +  "Mark the currently selected thread as deleted (set its \"deleted\" =\r
+tag).\r
+> > +This function advances the next thread when finished."\r
+> > +  (interactive)\r
+> > +  (notmuch-search-add-tag "deleted")\r
+> > +  (forward-line))\r
+> > +\r
+> > +\r
+> >  (defun notmuch-search-process-sentinel (proc msg)\r
+>=20\r
+> Watch that extra whitespace. The convention is a single line of\r
+> whitespace between each function.\r
+>=20\r
+\r
+My bad. I did not see this in my review. Corrected this in the updated patc=\r
+h.\r
+\r
+> And should we also archive the thread before removing the deleted tag?\r
+\r
+I don't understand this. Do you mean: should we also archive the\r
+thread before setting the deleted tag? I don't known why we should\r
+bother to do it, as the tag is not show by the next results.=20\r
+\r
+I would tend to think that archiving should be an explicit action,\r
+that should call a hook. For instance, I'd like my mailflow to be like\r
+this:\r
+\r
+- get mail with offlineimap =3D> a maildir directory, indexed by notmuch\r
+\r
+- mails stay in inbox until I archive it. That way I can still access\r
+  it with IMAP.\r
+\r
+- archiving puts an "archived" tag (and removes the inbox and unread\r
+  tags), and move the mail to another directory (or uses git to\r
+  archive the mail).\r
+\r
+- deleting puts a deleted tag (but the messages stays inbox as long as\r
+  I do not explicitely remove this).\r
+\r
+The main reason is that many mail servers only allow small quotas, so\r
+you have to carefully select which mail should stay inbox (only the\r
+mail that you need external access).\r
+\r
+In brief, this mail flow needs to delete threads but not to archive\r
+it. I think that we could provide hooks, so that you can archive\r
+mails when you delete them if you like to.\r
+\r
+By the way in my patch commentary, you said that you planned to change\r
+the "inbox" and "unread" treatments to be more explicit. Do you still\r
+plan to do that? I can maybe also provide a patch.\r
+\r
+> > +With prefix argument, include deleted items.\r
+>=20\r
+> That's a pretty good interface I think.\r
+>=20\r
+> Another approach would be to do something like what sup does---that\r
+> would be to scan the search terms and if it contains "tag:deleted" and\r
+> all then don't prepend the "not tag:deleted and" to the search\r
+> string. The assumption there is that if the user is explicitly\r
+> mentioning the deleted tag, then we should just rely on the user to\r
+> explicitly describe how the tag should be treated.\r
+>=20\r
+> That's perhaps not an unreasonable heuristic, and might be done even in\r
+> addition to the prefix-argument approach. But that could be an\r
+> additional commit, and I won't require it.\r
+\r
+Hmm, that's what I did want to do initially. In that case, I think\r
+that notmuch should be able to output parse trees from notmuch search\r
+strings as JSON; this would help do the parsing in the various\r
+implementations.\r
+\r
+I also had this idea that may solve the "tag should be\r
+orthogonal/command-line UI problem". We observe that "archived" is\r
+contrary to "inbox", "read" contrary to "unread", "deleted" contrary\r
+to "undeleted", and so on.\r
+\r
+So it may be a good idea to have "pseudo-tags", that would help in the\r
+interface (searching archived mails with search not tag:inbox is not\r
+very intuitive...), as synonyms for "not tag:countrary-tag"\r
+\r
+Then for each "pseudo-tag", notmuch could hard-code which is included\r
+by default. Adding the other tags is easy: just add "and tag:deleted"\r
+or "and tag:read" or "and tag:archived". Moreover, this would simplify\r
+UI implementation as well, without requiring parsing of the strings.\r
+\r
+Another, even simpler possibility would be to add a "--all" flag to\r
+notmuch. Without it, it would omit some of the tags (like the deleted\r
+ones), which would be convenient as a text interface. With it, it\r
+would show all tags, which would be convenient for use with scripts\r
+and UI, and mor in the "tags are orthogonal" spirit. Again, this would\r
+not require to parse search strings, and imho would be much cleaner.\r
+\r
+> > +  (interactive (let* ((prefix current-prefix-arg)\r
+> > +                      (query (if prefix\r
+> > +                                 (read-string "Notmuch search (includi=\r
+ng deleted): "\r
+> > +                                              notmuch-search-query-str=\r
+ing\r
+> > +                                              'notmuch-search-history)\r
+> > +                               (read-string "Notmuch search: " nil\r
+> > +                                            'notmuch-search-history)))=\r
+)\r
+>=20\r
+> Why is the second (initial-input) argument non-nil in one case, but nil\r
+> in the other? The documentation for `read-string' says the argument is\r
+> deprecated and should be nil in all new code.\r
+\r
+The idea is that you do not have default search terms if you do\r
+regular searchs. But if you want to include deleted items, then you\r
+have the last search string. This way, you handle the common case when\r
+you serach for a term, do not find it, then decide to try again with\r
+deleted items.\r
+\r
+Now, maybe the C-u interface is not such a good idea, and a toggle\r
+button would be better. Don't know, but I find this interface\r
+convenient.\r
+\r
+>=20\r
+> > +                 (list query nil prefix)))\r
+> > +  (let ((real-query (if include-deleted query\r
+> > +                      (concat "not tag:deleted and (" query ")")))\r
+> > +        (buffer (get-buffer-create (concat "*notmuch-search-" query\r
+> > "*"))))\r
+>=20\r
+> Does the include-deleted case actually work? I don't see anything in the\r
+> code that sets this variable. (I'm just reviewing here--I haven't tested\r
+> it manually).\r
+\r
+This is the way interactive work.\r
+(defun notmuch-search (query &optional oldest-first include-deleted)\r
+...\r
++  (interactive (let* ((prefix current-prefix-arg)\r
+...\r
++=09=09 (list query nil prefix)))\r
+\r
+The (list query nil prefix) at the end of the (interactive) form will\r
+set query to query, nil to oldest-first, and prefix to\r
+include-deleted.\r
+\r
+>=20\r
+> > @@ -1351,7 +1374,6 @@ search."\r
+> > =20\r
+> >  Runs a new search matching only messages that match both the\r
+> >  current search results AND the additional query string provided."\r
+> > -  (interactive "sFilter search: ")\r
+> >    (let ((grouped-query (if (string-match-p notmuch-search-disjunctive-=\r
+regexp query) (concat "( " query " )") query)))\r
+> >      (notmuch-search (concat notmuch-search-query-string " and " groupe=\r
+d-query) notmuch-search-oldest-first)))\r
+>=20\r
+> Is this just an accidental chunk in the patch? I don't see why this\r
+> function should become non-interactive now.\r
+\r
+My bad, corrected that also.\r
+\r
+Matthieu\r
+_______________________________________________\r
+notmuch mailing list\r
+notmuch@notmuchmail.org\r
+http://notmuchmail.org/mailman/listinfo/notmuch\r