From: Mark Walters <markwalters1009@gmail.com>
To: Jameson Graef Rollins <jrollins@finestructure.net>,
	Notmuch Mail <notmuch@notmuchmail.org>
Subject: Re: [PATCH 7/8] emacs: modify show tag functions to use new
	notmuch-tag interface
In-Reply-To: <87d37hl6kc.fsf@servo.finestructure.net>
References: <1333354853-25729-1-git-send-email-jrollins@finestructure.net>
	<1333845338-22960-1-git-send-email-jrollins@finestructure.net>
	<1333845338-22960-2-git-send-email-jrollins@finestructure.net>
	<1333845338-22960-3-git-send-email-jrollins@finestructure.net>
	<1333845338-22960-4-git-send-email-jrollins@finestructure.net>
	<1333845338-22960-5-git-send-email-jrollins@finestructure.net>
	<1333845338-22960-6-git-send-email-jrollins@finestructure.net>
	<1333845338-22960-7-git-send-email-jrollins@finestructure.net>
	<1333845338-22960-8-git-send-email-jrollins@finestructure.net>
	<87zkam6fn3.fsf@qmul.ac.uk>
	<87d37hl6kc.fsf@servo.finestructure.net>
User-Agent: Notmuch/0.12+111~g5c30f66 (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Mon, 09 Apr 2012 19:22:48 +0100
Message-ID: <874nssn4sn.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Mon, 09 Apr 2012, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> On Sat, Apr 07 2012, Mark Walters <markwalters1009@gmail.com> wrote:
>> I think this is what is making the two tests fail: they count the number
>> of invocations of notmuch and in case there is one invocation of notmuch
>> show and one of notmuch tag -unread message-id, where before it was just
>> the single notmuch show.
>
> Good call, Mark.  After a bit of testing it looks like that is what's
> going on.  I was confused, since I had thought that the call to
> notmuch-show should have involved two notmuch calls originally as well,
> one for retrieving the message and the other removing the unread tag.
> However, it appears the messages in those tests don't have unread tags
> after all.  Not sure why, but that explains it.
>
> So I guess the upshot is that moving all the common prompting and tag
> validation stuff into notmuch-tag means that in certain cases there will
> be extra notmuch calls, even if no tags are changed.  Is that a problem?
>
> What I can do, though, is add extra validation to notmuch-tag to not
> actually call notmuch tag, or any of the pre- and post- tagging hooks,
> if no tags are changing.  This will still require one call to notmuch to
> retrieve the current set of tags for the query, but at least it wont tag
> or call the hooks if nothing is changing.  That seems reasonable to me,
> but please let me know if you think it's not.
>
> I've pasted below a new version of notmuch-tag that addresses these
> issues.  Let me know what you think, and I'll resubmit the series.
>
> jamie.
>
>
> (defun notmuch-tag (query &optional tag-changes)
>   "Add/remove tags in TAG-CHANGES to messages matching QUERY.
>
> QUERY should be a string containing the search-terms.
> TAG-CHANGES can take multiple forms.  If TAG-CHANGES is a list of
> strings of the form \"+tag\" or \"-tag\" then those are the tag
> changes applied.  If TAG-CHANGES is a string then it is
> interpreted as a single tag change.  If TAG-CHANGES is the string
> \"-\" or \"+\", or null, then the user is prompted to enter the
> tag changes.
>
> Note: Other code should always use this function alter tags of
> messages instead of running (notmuch-call-notmuch-process \"tag\" ..)
> directly, so that hooks specified in notmuch-before-tag-hook and
> notmuch-after-tag-hook will be run."
>   ;; Perform some validation
>   (if (string-or-null-p tag-changes)
>       (if (or (string= tag-changes "-") (string= tag-changes "+") (null tag-changes))
> 	  (setq tag-changes (notmuch-read-tag-changes tag-changes query))
> 	(setq tag-changes (list tag-changes))))
>   (mapc (lambda (tag-change)
> 	  (unless (string-match-p "^[-+]\\S-+$" tag-change)
> 	    (error "Tag must be of the form `+this_tag' or `-that_tag'")))
> 	tag-changes)
>   (let* ((current-tags (notmuch-tag-completions (list query)))
> 	 (new-tags (notmuch-update-tags current-tags tag-changes)))
>     (if (equal current-tags new-tags)
> 	;; if no tags are changing, return nil
> 	nil
>       (run-hooks 'notmuch-before-tag-hook)
>       (apply 'notmuch-call-notmuch-process "tag"
> 	     (append tag-changes (list "--" query)))
>       (run-hooks 'notmuch-after-tag-hook)
>       ;; otherwise, return the list of actual changed tags
>       tag-changes)))

Does this actually do the right thing if tagging more than one message?
It looks to me like it would go wrong if you tried +inbox to a thread
where some messages already have tag inbox (but I could be confused)?

Also, I was going to say that I was not sure there was much point in
optimising in the emacs code when the cli does anyway, but there is a
question with xapian locking: with the orginally posted patch you can't
use n or p in show view while the database is locked (eg a background
notmuch new) as you get "A Xapian exception occurred opening database:
Unable to get write lock on ..."

Possibly, you could pass a current-tags variable to notmuch tag (and it
would not add anything in that list or delete anything not in the
list). But the 2 code paths might be viewed as being too different to be
worth unifying. Or possibly have a "tag-single-message" command? 

Best wishes

Mark