Date: Sat, 24 Nov 2012 19:38:15 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: markwalters1009 <markwalters1009@gmail.com>
Subject: Re: [PATCH v2 7/7] emacs: make emacs use message-ids for tagging
Message-ID: <20121125003815.GK4562@mit.edu>
References: <1353763256-32336-1-git-send-email-markwalters1009@gmail.com>
	<1353763256-32336-8-git-send-email-markwalters1009@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1353763256-32336-8-git-send-email-markwalters1009@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth markwalters1009 on Nov 24 at  1:20 pm:
> From: Mark Walters <markwalters1009@gmail.com>
> 
> This makes emacs use the new --queries=true in search mode and use
> this for tagging.  This fixes the race condition in tagging from
> search mode so mark the tests fixed.
> ---
>  emacs/notmuch.el |   28 +++++++++++++++++++++++++---
>  test/emacs       |    2 --
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 64b9474..6e8ef83 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -473,7 +473,8 @@ BEG."
>    (let (output)
>      (notmuch-search-foreach-result beg end
>        (lambda (pos)
> -	(push (plist-get (notmuch-search-get-result pos) property) output)))
> +	(let ((value (plist-get (notmuch-search-get-result pos) property)))
> +	  (when value (push value output)))))

Why is this necessary?  (Assuming it is, it could use a comment, and
probably an update to the docstring.)

>      output))
>  
>  (defun notmuch-search-find-thread-id (&optional bare)
> @@ -483,6 +484,7 @@ If BARE is set then do not prefix with \"thread:\""
>    (let ((thread (plist-get (notmuch-search-get-result) :thread)))
>      (when thread (concat (unless bare "thread:") thread))))
>  
> +

Unintentional?

>  (defun notmuch-search-find-thread-id-region (beg end)
>    "Return a list of threads for the current region"
>    (mapcar (lambda (thread) (concat "thread:" thread))
> @@ -492,6 +494,23 @@ If BARE is set then do not prefix with \"thread:\""
>    "Return a search string for threads for the current region"
>    (mapconcat 'identity (notmuch-search-find-thread-id-region beg end) " or "))
>  
> +;; The following two functions are similar to the previous two but
> +;; they only match messages that were in the the thread when the
> +;; initial search was run. This means that they can be used where it
> +;; is important to avoid races: e.g. when tagging.
> +(defun notmuch-search-find-queries-region (beg end &optional only-matching)
> +  (interactive)
> +  "Return a list of queries for the current region"
> +  (append (notmuch-search-properties-in-region :matching_msg_query beg end)
> +	  (unless only-matching
> +	    (notmuch-search-properties-in-region :nonmatching_msg_query beg end))))

Two minor performance nits: Using nconc instead of append will avoid
copying the first list and swapping the two arguments will avoid
needlessly traversing the non-matching list when only-matching is nil.

> +
> +(defun notmuch-search-find-queries-region-search (beg end &optional only-matching)
> +  "Return a search string for messages in threads in the current region"
> +  (mapconcat 'identity

#'identity

> +	     (notmuch-search-find-queries-region beg end only-matching)
> +	     " or "))
> +
>  (defun notmuch-search-find-authors ()
>    "Return the authors for the current thread"
>    (plist-get (notmuch-search-get-result) :authors))
> @@ -575,7 +594,7 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
>  
>  (defun notmuch-search-tag-region (beg end &optional tag-changes)
>    "Change tags for threads in the given region."
> -  (let ((search-string (notmuch-search-find-thread-id-region-search beg end)))
> +  (let ((search-string (notmuch-search-find-queries-region-search beg end)))
>      (setq tag-changes (funcall 'notmuch-tag search-string tag-changes))
>      (notmuch-search-foreach-result beg end
>        (lambda (pos)
> @@ -851,7 +870,9 @@ non-authors is found, assume that all of the authors match."
>  
>  See `notmuch-tag' for information on the format of TAG-CHANGES."
>    (interactive)
> -  (apply 'notmuch-tag notmuch-search-query-string tag-changes))
> +  (apply 'notmuch-tag (notmuch-search-find-queries-region-search
> +		       (point-min) (point-max) t)
> +	 tag-changes))
>  
>  (defun notmuch-search-buffer-title (query)
>    "Returns the title for a buffer with notmuch search results."
> @@ -948,6 +969,7 @@ Other optional parameters are used as follows:
>  		     "notmuch-search" buffer
>  		     notmuch-command "search"
>  		     "--format=json"
> +		     "--output=with-queries"

--output=with-queries or --queries=true?

>  		     (if oldest-first
>  			 "--sort=oldest-first"
>  		       "--sort=newest-first")
> diff --git a/test/emacs b/test/emacs
> index 3788439..132768f 100755
> --- a/test/emacs
> +++ b/test/emacs
> @@ -123,7 +123,6 @@ output=$(notmuch search $os_x_darwin_thread | notmuch_search_sanitize)
>  test_expect_equal "$output" "thread:XXX   2009-11-18 [4/4] Jjgod Jiang, Alexander Botero-Lowry; [notmuch] Mac OS X/Darwin compatibility issues (inbox unread)"
>  
>  test_begin_subtest "Tag all matching messages from search view"
> -test_subtest_known_broken
>  notmuch tag +test-tag-race from:cworth
>  test_emacs "(notmuch-search \"tag:test-tag-race\")
>  	    (notmuch-test-wait)"
> @@ -135,7 +134,6 @@ notmuch tag -test-tag-race '*'
>  notmuch tag -test-tag-race-2 '*'
>  
>  test_begin_subtest "Change tags from search view: another message arriving after thread lookup"
> -test_subtest_known_broken
>  typsos_id="878we4qdqf.fsf@yoom.home.cworth.org"
>  typsos_thread=$(notmuch search --output=threads id:$typsos_id)
>  test_emacs "(notmuch-search \"$typsos_thread\")