Re: [PATCH v1] emacs: `notmuch-search-find-stable-query-region' should never return an empty query.
Carl Worth cwo...@cworth.org writes: Mark Walters markwalters1...@gmail.com writes: 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: ... Anyway, I'm glad to see the notmuch emacs interface getting patched for this problem. We should ensure that the Xapian parsing bug is reported as well. *And*, since Mark identified bugs in three different layers, we should fix that third layer as well. It seems we could make notmuch-tag.c more robust by using native Xapian interfaces to construct a query AND query data structure rather than using string manipulation in an attempt to construct this same query. That would prevent this ambiguity, and avoid any other cases of emacs (or other users) running across a similar bug here. -Carl -- carl.d.wo...@intel.com pgpqcsHyu7MQy.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH v1] emacs: `notmuch-search-find-stable-query-region' should never return an empty query.
On Tue, 20 May 2014, David Edmondson wrote:
> On Mon, May 19 2014, Mark Walters wrote:
>> On Mon, 19 May 2014, David Edmondson 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 at notmuchmail.org
>>> http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v1] emacs: `notmuch-search-find-stable-query-region' should never return an empty query.
Mark Walters markwalters1...@gmail.com writes: 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! Thank you!! I'm quite sure that I've been bitten by this bug a couple of times. (Actually, it's likely bitten me more than the couple of times I happened to notice it.) When I noticed it, I had only a few messages in my inbox, (had just been reading/archiving some), pressed '=' to refresh, and several of them disappeared. It was extremely disconcerting that notmuch would lose data like that on me. I hadn't reported the bug because I hadn't found any reliable way to replicate it, nor could I be sure that it wasn't something unique to my setup, (such that I had actually archived those messages, but from some other script or view, etc.). But given your description, I'm confident that the data loss I watched before my eyes fits that description. ( () ) 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 use NOTMUCH_DEBUG_QUERY to get some insight into this. So the query that works as expected: $ NOTMUCH_DEBUG_QUERY=1 notmuch count (()) Query string is: (()) Exclude query is: Xapian::Query() Final query is: Xapian::Query() 0 And then the query that is misbehaving: $ NOTMUCH_DEBUG_QUERY=1 notmuch count (()) and tag:inbox Query string is: (()) and tag:inbox Exclude query is: Xapian::Query() Final query is: Xapian::Query((Tmail AND (Zand:(pos=1) FILTER Kinbox))) 14 So you can see that the query is actually matching a term Zand, (that is, matching on emails for which the word and has been indexed). It's a funny bug. Note that using () and tag:inbox does not trigger the same behavior: $ notmuch count () and tag:inbox A Xapian exception occurred: Syntax: expression AND expression Query string was: () and tag:inbox 0 I would have much preferred getting that exception instead of the query we were getting before. Anyway, I'm glad to see the notmuch emacs interface getting patched for this problem. We should ensure that the Xapian parsing bug is reported as well. -Carl pgpDHzyKTi8qh.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH v1] emacs: `notmuch-search-find-stable-query-region' should never return an empty query.
On Mon, May 19 2014, Mark Walters wrote:
> On Mon, 19 May 2014, David Edmondson 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.
> 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 at notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 310 bytes
Desc: not available
URL:
Re: [PATCH v1] emacs: `notmuch-search-find-stable-query-region' should never return an empty query.
On Mon, May 19 2014, Mark Walters wrote: On Mon, 19 May 2014, David Edmondson d...@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. 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 signature.asc Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v1] emacs: `notmuch-search-find-stable-query-region' should never return an empty query.
On Tue, 20 May 2014, David Edmondson d...@dme.org wrote: On Mon, May 19 2014, Mark Walters wrote: On Mon, 19 May 2014, David Edmondson d...@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 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH v1] emacs: `notmuch-search-find-stable-query-region' should never return an empty query.
On Mon, 19 May 2014, David Edmondson 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!)
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.
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 at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH v1] emacs: `notmuch-search-find-stable-query-region' should never return an empty query.
`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.
---
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
[PATCH v1] emacs: `notmuch-search-find-stable-query-region' should never return an empty query.
`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. --- 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
Re: [PATCH v1] emacs: `notmuch-search-find-stable-query-region' should never return an empty query.
On Mon, 19 May 2014, David Edmondson d...@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!) 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. 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 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
