From: Mark Walters <markwalters1009@gmail.com>
To: David Edmondson <dme@dme.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH v1] emacs: `notmuch-search-find-stable-query-region'
	should never return an empty query.
In-Reply-To: <cunfvk43dbm.fsf@hotblack-desiato.hh.sledj.net>
References: <1400515756-31779-1-git-send-email-dme@dme.org>
	<87lhtxzb4m.fsf@qmul.ac.uk>
	<cunfvk43dbm.fsf@hotblack-desiato.hh.sledj.net>
User-Agent: Notmuch/0.15.2+615~g78e3a93 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Wed, 21 May 2014 01:36:41 +0100
Message-ID: <87iop0ynl2.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


On Tue, 20 May 2014, David Edmondson <dme@dme.org> wrote:
> On Mon, May 19 2014, Mark Walters wrote:
>> On Mon, 19 May 2014, David Edmondson <dme@dme.org> wrote:
>>> `notmuch-search-find-stable-query-region' is expected to examine the
>>> region between `beg' and `end' to generate a query that can be used to
>>> include all threads in that region. If the region contains no threads,
>>> it should throw an error rather than generating an empty query.
>>
>> Hi
>>
>> This seems a very definite bug (when testing I managed to archive a
>> whole chunk of random messages!) 
>
> Do you understand why? I couldn't see a path from this problem to that
> failure mode.

Ok I now understand some of why it is a bug and it is a pretty gross
mix: emacs does something odd, then notmuch-tag.c then xapian. It only
happens in some cases:

WARNING the following will archive some random messages: have a dump or
backup or something!

if you do "a" at the end of the buffer then the emacs code creates the
trivial query: () 
Then runs notmuch tag -inbox -- ()

in notmuch-tag.c we are clever and optimise the query (to avoid trying
to remove the tag from messages that don't have it) and this becomes the
query-string

( () ) and ( tag:inbox )

for reasons which aren't clear to me this matches some but not all
of the messages with tag:inbox. This is despite the fact that searching for
( () )  or () by itself yields no results.

You can test this bit just with notmuch search or count

notmuch count "(()) and tag:inbox"

Since this email is rather long I will reply to the rest separately.

Best wishes

Mark






>
>> However, I think I would prefer not to signal an error and just do
>> nothing. How about making notmuch-tag check for a nil query (and do
>> nothing it's nil). Then rather than an error n.s.f.s.q.r can just return
>> nil (this still needs to be special cased as otherwise we get "()" as
>> the query.
>
> Looking around for similar issues (like
> `notmuch-show-get-message-properties'), it seems that we would have to
> add checks in a lot of places for functions such as this returning `nil'
> when they cannot find some state or context.
>
> Throwing an error makes it clear to the user that nothing is going to
> happen - it's not just silent failure.
>
> (If it's not clear - that was all "I like that it calls `error' :-).
>
>> Doing this would also fix a bug I found (when seeing what we did
>> elsewhere based on the above) in notmuch-tree: trying to change tags at
>> the end of the buffer gives an error).
>>
>> Best wishes
>>
>> Mark
>>
>>
>>
>>> ---
>>>
>>> Whilst logging calls to 'notmuch' from the UI, I noticed that it would generate
>>>   notmuch tag -inbox -- ()
>>> if I hit 'a' at the very end of a search buffer. That seems at least
>>> useless and possibly bad, so flag an error in this case instead.
>>>
>>> Oh, the first bit is just cleanup.
>>>
>>>  emacs/notmuch.el | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
>>> index 8aa0104..74103a6 100644
>>> --- a/emacs/notmuch.el
>>> +++ b/emacs/notmuch.el
>>> @@ -429,12 +429,15 @@ matched and unmatched messages in the current thread."
>>>  
>>>  If ONLY-MATCHED is non-nil, include only matched messages.  If it
>>>  is nil, include both matched and unmatched messages."
>>> -  (let ((query-list nil) (all (not only-matched)))
>>> +  (let ((all (not only-matched))
>>> +	query-list)
>>>      (dolist (queries (notmuch-search-properties-in-region :query beg end))
>>>        (when (first queries)
>>>  	(push (first queries) query-list))
>>>        (when (and all (second queries))
>>>  	(push (second queries) query-list)))
>>> +    (unless query-list
>>> +      (error "No threads in region."))
>>>      (concat "(" (mapconcat 'identity query-list ") or (") ")")))
>>>  
>>>  (defun notmuch-search-find-authors ()
>>> -- 
>>> 2.0.0.rc0
>>>
>>> _______________________________________________
>>> notmuch mailing list
>>> notmuch@notmuchmail.org
>>> http://notmuchmail.org/mailman/listinfo/notmuch