[PATCH v1] emacs: `notmuch-search-find-stable-query-region' should never return an empty query.

[PATCH v1] emacs: `notmuch-search-find-stable-query-region' should never return an empty query.

[David Edmondson]

`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

Re: [PATCH v1] emacs: `notmuch-search-find-stable-query-region' should never return an empty query.

[Mark Walters]

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!) 

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

Re: [PATCH v1] emacs: `notmuch-search-find-stable-query-region' should never return an empty query.

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 3043 bytes --]

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.

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 310 bytes --]

Re: [PATCH v1] emacs: `notmuch-search-find-stable-query-region' should never return an empty query.

[Mark Walters]

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

[PATCH] emacs: make sure tagging on an empty query is harmless

[Mark Walters]

Currently notmuch-tag throws a "wrong-type-argument stringp nil" if
passed a nil query-string. Catch this and provide a more useful error
message. This fixes a case in notmuch-tree (if you try to tag when at
the end of the buffer).

Secondly, as pointed out by David (dme)
`notmuch-search-find-stable-query-region' can return the query string
() if there are no messages in the region. This gets passed to notmuch
tag, and due to interactions in the optimize_query code in
notmuch-tag.c becomes, in the case tag-change is -inbox, "( () ) and
(tag:inbox)". This query matches some strange collection of messages
which then get archived. This should probably be fixed, but in any
case make `notmuch-search-find-stable-query-region' return a nil
query-string in this case.

This avoids data-loss (random tag removal) in this case.
---

This is my attempt to solve the same problem as the parent. I prefer
not throwing an error in n.s.f.s.q.r as it is difficult for the caller
to catch cleanly. Throwing it in notmuch-tag is fine as the caller can
trivially check for query-string being nil before calling notmuch-tag
if it wants to deal with it gracefully.

If people do prefer an error in n.s.f.s.q.r as in the parent patch
then I think we should update the error message. The first hunk of
this should also be applied to catch nil queries to notmuch-tag gracefully.

Although this has been present for a while I think it is a dataloss
issue so a fix should go in for 0.18.1

Best wishes

Mark




 emacs/notmuch-tag.el |    2 ++
 emacs/notmuch.el     |    6 ++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
index 07c260e..f54aa9d 100644
--- a/emacs/notmuch-tag.el
+++ b/emacs/notmuch-tag.el
@@ -387,6 +387,8 @@ (defun notmuch-tag (query tag-changes)
 	  (unless (string-match-p "^[-+]\\S-+$" tag-change)
 	    (error "Tag must be of the form `+this_tag' or `-that_tag'")))
 	tag-changes)
+  (unless query
+    (error "Nothing to tag!"))
   (unless (null tag-changes)
     (run-hooks 'notmuch-before-tag-hook)
     (if (<= (length query) notmuch-tag-argument-limit)
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 6c0bc1b..1adea9c 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -428,14 +428,16 @@ (defun notmuch-search-find-stable-query-region (beg end &optional only-matched)
   "Return the stable query for the current region.
 
 If ONLY-MATCHED is non-nil, include only matched messages.  If it
-is nil, include both matched and unmatched messages."
+is nil, include both matched and unmatched messages. If there are
+no messages in the region then return nil."
   (let ((query-list nil) (all (not only-matched)))
     (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)))
-    (concat "(" (mapconcat 'identity query-list ") or (") ")")))
+    (when query-list
+      (concat "(" (mapconcat 'identity query-list ") or (") ")"))))
 
 (defun notmuch-search-find-authors ()
   "Return the authors for the current thread"
-- 
1.7.10.4

Re: [PATCH] emacs: make sure tagging on an empty query is harmless

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 3579 bytes --]

On Wed, May 21 2014, Mark Walters wrote:
> Currently notmuch-tag throws a "wrong-type-argument stringp nil" if
> passed a nil query-string. Catch this and provide a more useful error
> message. This fixes a case in notmuch-tree (if you try to tag when at
> the end of the buffer).
>
> Secondly, as pointed out by David (dme)
> `notmuch-search-find-stable-query-region' can return the query string
> () if there are no messages in the region. This gets passed to notmuch
> tag, and due to interactions in the optimize_query code in
> notmuch-tag.c becomes, in the case tag-change is -inbox, "( () ) and
> (tag:inbox)". This query matches some strange collection of messages
> which then get archived. This should probably be fixed, but in any
> case make `notmuch-search-find-stable-query-region' return a nil
> query-string in this case.
>
> This avoids data-loss (random tag removal) in this case.
> ---
>
> This is my attempt to solve the same problem as the parent. I prefer
> not throwing an error in n.s.f.s.q.r as it is difficult for the caller
> to catch cleanly. Throwing it in notmuch-tag is fine as the caller can
> trivially check for query-string being nil before calling notmuch-tag
> if it wants to deal with it gracefully.
>
> If people do prefer an error in n.s.f.s.q.r as in the parent patch
> then I think we should update the error message. The first hunk of
> this should also be applied to catch nil queries to notmuch-tag gracefully.

I'm fine with this approach.

> Although this has been present for a while I think it is a dataloss
> issue so a fix should go in for 0.18.1

As someone who has had to reconstruct a couple of times after being
bitten by this, I would agree.

> Best wishes
>
> Mark
>
>
>
>
>  emacs/notmuch-tag.el |    2 ++
>  emacs/notmuch.el     |    6 ++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
> index 07c260e..f54aa9d 100644
> --- a/emacs/notmuch-tag.el
> +++ b/emacs/notmuch-tag.el
> @@ -387,6 +387,8 @@ (defun notmuch-tag (query tag-changes)
>  	  (unless (string-match-p "^[-+]\\S-+$" tag-change)
>  	    (error "Tag must be of the form `+this_tag' or `-that_tag'")))
>  	tag-changes)
> +  (unless query
> +    (error "Nothing to tag!"))
>    (unless (null tag-changes)
>      (run-hooks 'notmuch-before-tag-hook)
>      (if (<= (length query) notmuch-tag-argument-limit)
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 6c0bc1b..1adea9c 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -428,14 +428,16 @@ (defun notmuch-search-find-stable-query-region (beg end &optional only-matched)
>    "Return the stable query for the current region.
>  
>  If ONLY-MATCHED is non-nil, include only matched messages.  If it
> -is nil, include both matched and unmatched messages."
> +is nil, include both matched and unmatched messages. If there are
> +no messages in the region then return nil."
>    (let ((query-list nil) (all (not only-matched)))
>      (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)))
> -    (concat "(" (mapconcat 'identity query-list ") or (") ")")))
> +    (when query-list
> +      (concat "(" (mapconcat 'identity query-list ") or (") ")"))))
>  
>  (defun notmuch-search-find-authors ()
>    "Return the authors for the current thread"
> -- 
> 1.7.10.4

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 310 bytes --]

Re: [PATCH] emacs: make sure tagging on an empty query is harmless

[Tomi Ollila]

On Wed, May 21 2014, Mark Walters <markwalters1009@gmail.com> wrote:

> Currently notmuch-tag throws a "wrong-type-argument stringp nil" if
> passed a nil query-string. Catch this and provide a more useful error
> message. This fixes a case in notmuch-tree (if you try to tag when at
> the end of the buffer).
>
> Secondly, as pointed out by David (dme)
> `notmuch-search-find-stable-query-region' can return the query string
> () if there are no messages in the region. This gets passed to notmuch
> tag, and due to interactions in the optimize_query code in
> notmuch-tag.c becomes, in the case tag-change is -inbox, "( () ) and
> (tag:inbox)". This query matches some strange collection of messages
> which then get archived. This should probably be fixed, but in any
> case make `notmuch-search-find-stable-query-region' return a nil
> query-string in this case.
>
> This avoids data-loss (random tag removal) in this case.
> ---

+1 from me for 0.18.1, too.

Tomi


>
> This is my attempt to solve the same problem as the parent. I prefer
> not throwing an error in n.s.f.s.q.r as it is difficult for the caller
> to catch cleanly. Throwing it in notmuch-tag is fine as the caller can
> trivially check for query-string being nil before calling notmuch-tag
> if it wants to deal with it gracefully.
>
> If people do prefer an error in n.s.f.s.q.r as in the parent patch
> then I think we should update the error message. The first hunk of
> this should also be applied to catch nil queries to notmuch-tag gracefully.
>
> Although this has been present for a while I think it is a dataloss
> issue so a fix should go in for 0.18.1
>
> Best wishes
>
> Mark
>
>
>
>
>  emacs/notmuch-tag.el |    2 ++
>  emacs/notmuch.el     |    6 ++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
> index 07c260e..f54aa9d 100644
> --- a/emacs/notmuch-tag.el
> +++ b/emacs/notmuch-tag.el
> @@ -387,6 +387,8 @@ (defun notmuch-tag (query tag-changes)
>  	  (unless (string-match-p "^[-+]\\S-+$" tag-change)
>  	    (error "Tag must be of the form `+this_tag' or `-that_tag'")))
>  	tag-changes)
> +  (unless query
> +    (error "Nothing to tag!"))
>    (unless (null tag-changes)
>      (run-hooks 'notmuch-before-tag-hook)
>      (if (<= (length query) notmuch-tag-argument-limit)
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 6c0bc1b..1adea9c 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -428,14 +428,16 @@ (defun notmuch-search-find-stable-query-region (beg end &optional only-matched)
>    "Return the stable query for the current region.
>  
>  If ONLY-MATCHED is non-nil, include only matched messages.  If it
> -is nil, include both matched and unmatched messages."
> +is nil, include both matched and unmatched messages. If there are
> +no messages in the region then return nil."
>    (let ((query-list nil) (all (not only-matched)))
>      (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)))
> -    (concat "(" (mapconcat 'identity query-list ") or (") ")")))
> +    (when query-list
> +      (concat "(" (mapconcat 'identity query-list ") or (") ")"))))
>  
>  (defun notmuch-search-find-authors ()
>    "Return the authors for the current thread"
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: make sure tagging on an empty query is harmless

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> Currently notmuch-tag throws a "wrong-type-argument stringp nil" if
> passed a nil query-string. Catch this and provide a more useful error
> message. This fixes a case in notmuch-tree (if you try to tag when at
> the end of the buffer).

pushed to release and master

d