[PATCH] emacs: hello: make --batch error gracefully

[PATCH] emacs: hello: make --batch error gracefully

[Mark Walters]

Recently notmuch-hello was converted to use batch count. However, it
seems that several people run different versions of notmuch-emacs and
notmuch-cli so this batch makes emacs fail with an error message if
--batch is not available in the CLI.
---
There have been two cases on irc of people getting backtraces when
hitting this problem so it might be worth adding an informative error
message.

Best wishes

Mark

 emacs/notmuch-hello.el |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 147c08c..fa46b7a 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -402,8 +402,13 @@ options will be handled as specified for
 					   (plist-get options :filter)))
 	 "\n")))
 
-    (call-process-region (point-min) (point-max) notmuch-command
-			 t t nil "count" "--batch")
+    (unless (= (call-process-region (point-min) (point-max) notmuch-command
+				    t t nil "count" "--batch") 0)
+      (notmuch-logged-error "notmuch CLI version mismatch error (count --batch)
+The most likely cause of this error is that the CLI is too old
+to support count --batch and needs to be upgraded to the same
+version as notmuch-emacs"))
+
     (goto-char (point-min))
 
     (notmuch-remove-if-not
-- 
1.7.9.1

Re: [PATCH] emacs: hello: make --batch error gracefully

[Tomi Ollila]

On Fri, Jul 05 2013, Mark Walters <markwalters1009@gmail.com> wrote:

> Recently notmuch-hello was converted to use batch count. However, it
> seems that several people run different versions of notmuch-emacs and
> notmuch-cli so this batch makes emacs fail with an error message if
> --batch is not available in the CLI.
> ---
> There have been two cases on irc of people getting backtraces when
> hitting this problem so it might be worth adding an informative error
> message.

This change takes care of the mismatching version problem now -- for
a short while in most cases but in the future we might face with new
incompabilities that would -- again -- need new solution. Some day
we might have a pile of these who everyone is shy to remove from the
code base ;/

Although I am not against applying this patch (if there are supporters
of this) I'd like to concentrate fixing this for example the following
way:

We'll add a global option to notmuch, e.g.

--compatibility-version=x.y

Whenever the caller chooses to use this option, notmuch checks whether
it can comply with the option -- it it can, execution continues, otherwise 
aborts.

The compatibility is determined so that the major 'x' needs to be same
and caller may have lower 'y' that notmuch is capable of handling.

For example. if notmuch compatibility version was 2.5

--compatibility-version=1.9  --  abort
--compatibility-version=2.3  --  continue
--compatibility-version=2.5  --  continue
--compatibility-version=2.8  --  abort
--compatibility-version=3.1  --  abort


I can work on this (or on something similar) if this is generally thought
as a good idea...

>
> Best wishes
>
> Mark

Tomi


>
>  emacs/notmuch-hello.el |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> index 147c08c..fa46b7a 100644
> --- a/emacs/notmuch-hello.el
> +++ b/emacs/notmuch-hello.el
> @@ -402,8 +402,13 @@ options will be handled as specified for
>  					   (plist-get options :filter)))
>  	 "\n")))
>  
> -    (call-process-region (point-min) (point-max) notmuch-command
> -			 t t nil "count" "--batch")
> +    (unless (= (call-process-region (point-min) (point-max) notmuch-command
> +				    t t nil "count" "--batch") 0)
> +      (notmuch-logged-error "notmuch CLI version mismatch error (count --batch)
> +The most likely cause of this error is that the CLI is too old
> +to support count --batch and needs to be upgraded to the same
> +version as notmuch-emacs"))
> +
>      (goto-char (point-min))
>  
>      (notmuch-remove-if-not
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: hello: make --batch error gracefully

[Mark Walters]

Hi
Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Fri, Jul 05 2013, Mark Walters <markwalters1009@gmail.com> wrote:
>
>> Recently notmuch-hello was converted to use batch count. However, it
>> seems that several people run different versions of notmuch-emacs and
>> notmuch-cli so this batch makes emacs fail with an error message if
>> --batch is not available in the CLI.
>> ---
>> There have been two cases on irc of people getting backtraces when
>> hitting this problem so it might be worth adding an informative error
>> message.
>
> This change takes care of the mismatching version problem now -- for
> a short while in most cases but in the future we might face with new
> incompabilities that would -- again -- need new solution. Some day
> we might have a pile of these who everyone is shy to remove from the
> code base ;/

I think we want the unless clause anyway: we do not want to try parsing
something and getting stacktrace errors whatever happens. The error
message will need updating so I should probably add a FIXME to the
code. Perhaps the patch roughly as is could be applied to 0.16 and the
fixed error message together with your ideas from below to master?

> Although I am not against applying this patch (if there are supporters
> of this) I'd like to concentrate fixing this for example the following
> way:

This looks good. Some thoughts below.

> We'll add a global option to notmuch, e.g.
>
> --compatibility-version=x.y
>
> Whenever the caller chooses to use this option, notmuch checks whether
> it can comply with the option -- it it can, execution continues, otherwise 
> aborts.

I think the return value could indicate what the problem was (ie too new
or too old): so frontends could decide to work around it (perhaps 22 and
23 to come after the format-version return values). This would mean that
callers would have an easier way of telling if --compatability-version
is supported at all to ease the transition.

I wondered whether it overlapped with Austin's format-version stuff but
I think it is sufficiently different but some clear documentation as to
which means what could be helpful.

Best wishes

Mark

> The compatibility is determined so that the major 'x' needs to be same
> and caller may have lower 'y' that notmuch is capable of handling.
>
> For example. if notmuch compatibility version was 2.5
>
> --compatibility-version=1.9  --  abort
> --compatibility-version=2.3  --  continue
> --compatibility-version=2.5  --  continue
> --compatibility-version=2.8  --  abort
> --compatibility-version=3.1  --  abort
>
>
> I can work on this (or on something similar) if this is generally thought
> as a good idea...
>
>>
>> Best wishes
>>
>> Mark
>
> Tomi
>
>
>>
>>  emacs/notmuch-hello.el |    9 +++++++--
>>  1 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
>> index 147c08c..fa46b7a 100644
>> --- a/emacs/notmuch-hello.el
>> +++ b/emacs/notmuch-hello.el
>> @@ -402,8 +402,13 @@ options will be handled as specified for
>>  					   (plist-get options :filter)))
>>  	 "\n")))
>>  
>> -    (call-process-region (point-min) (point-max) notmuch-command
>> -			 t t nil "count" "--batch")
>> +    (unless (= (call-process-region (point-min) (point-max) notmuch-command
>> +				    t t nil "count" "--batch") 0)
>> +      (notmuch-logged-error "notmuch CLI version mismatch error (count --batch)
>> +The most likely cause of this error is that the CLI is too old
>> +to support count --batch and needs to be upgraded to the same
>> +version as notmuch-emacs"))
>> +
>>      (goto-char (point-min))
>>  
>>      (notmuch-remove-if-not
>> -- 
>> 1.7.9.1
>>
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: hello: make --batch error gracefully

[Tomi Ollila]

On Sun, Jul 07 2013, Mark Walters <markwalters1009@gmail.com> wrote:

> Hi
> Tomi Ollila <tomi.ollila@iki.fi> writes:
>
>> On Fri, Jul 05 2013, Mark Walters <markwalters1009@gmail.com> wrote:
>>
>>> Recently notmuch-hello was converted to use batch count. However, it
>>> seems that several people run different versions of notmuch-emacs and
>>> notmuch-cli so this batch makes emacs fail with an error message if
>>> --batch is not available in the CLI.
>>> ---
>>> There have been two cases on irc of people getting backtraces when
>>> hitting this problem so it might be worth adding an informative error
>>> message.
>>
>> This change takes care of the mismatching version problem now -- for
>> a short while in most cases but in the future we might face with new
>> incompabilities that would -- again -- need new solution. Some day
>> we might have a pile of these who everyone is shy to remove from the
>> code base ;/
>
> I think we want the unless clause anyway: we do not want to try parsing
> something and getting stacktrace errors whatever happens. The error
> message will need updating so I should probably add a FIXME to the
> code. Perhaps the patch roughly as is could be applied to 0.16 and the
> fixed error message together with your ideas from below to master?

Like said below (even) the current ok by me -- just that whatever is
chosen will be there into foreseeable future... ;)

>> Although I am not against applying this patch (if there are supporters
>> of this) I'd like to concentrate fixing this for example the following
>> way:
>
> This looks good. Some thoughts below.
>
>> We'll add a global option to notmuch, e.g.
>>
>> --compatibility-version=x.y
>>
>> Whenever the caller chooses to use this option, notmuch checks whether
>> it can comply with the option -- it it can, execution continues, otherwise 
>> aborts.
>
> I think the return value could indicate what the problem was (ie too new
> or too old): so frontends could decide to work around it (perhaps 22 and
> 23 to come after the format-version return values). This would mean that
> callers would have an easier way of telling if --compatability-version
> is supported at all to ease the transition.
>
> I wondered whether it overlapped with Austin's format-version stuff but
> I think it is sufficiently different but some clear documentation as to
> which means what could be helpful.

Yes, I had Austin's format-version options into mind when quickly drafting
the suggestion (we'd use the same 21&22 (or whatever) return values too...)

Now that I spend a week with only N9 as a computing device I've also
thought how much of a maintenance burden we want to take with this...
... and thought as an alternative that during byte compilation of emacs
files we pick notmuch cli version to the .elc files -- and just popup
a warning message (on first notmuch-hello invocation) if the versions 
differ...

I'll investigate what kind of emacs macro functionality could be used
to do this (more fun than solving sudoku's that is :D)

> Best wishes
>
> Mark

Tomi


>
>> The compatibility is determined so that the major 'x' needs to be same
>> and caller may have lower 'y' that notmuch is capable of handling.
>>
>> For example. if notmuch compatibility version was 2.5
>>
>> --compatibility-version=1.9  --  abort
>> --compatibility-version=2.3  --  continue
>> --compatibility-version=2.5  --  continue
>> --compatibility-version=2.8  --  abort
>> --compatibility-version=3.1  --  abort
>>
>>
>> I can work on this (or on something similar) if this is generally thought
>> as a good idea...
>>
>>>
>>> Best wishes
>>>
>>> Mark
>>
>> Tomi
>>
>>
>>>
>>>  emacs/notmuch-hello.el |    9 +++++++--
>>>  1 files changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
>>> index 147c08c..fa46b7a 100644
>>> --- a/emacs/notmuch-hello.el
>>> +++ b/emacs/notmuch-hello.el
>>> @@ -402,8 +402,13 @@ options will be handled as specified for
>>>  					   (plist-get options :filter)))
>>>  	 "\n")))
>>>  
>>> -    (call-process-region (point-min) (point-max) notmuch-command
>>> -			 t t nil "count" "--batch")
>>> +    (unless (= (call-process-region (point-min) (point-max) notmuch-command
>>> +				    t t nil "count" "--batch") 0)
>>> +      (notmuch-logged-error "notmuch CLI version mismatch error (count --batch)
>>> +The most likely cause of this error is that the CLI is too old
>>> +to support count --batch and needs to be upgraded to the same
>>> +version as notmuch-emacs"))
>>> +
>>>      (goto-char (point-min))
>>>  
>>>      (notmuch-remove-if-not
>>> -- 
>>> 1.7.9.1
>>>
>>> _______________________________________________
>>> notmuch mailing list
>>> notmuch@notmuchmail.org
>>> http://notmuchmail.org/mailman/listinfo/notmuch
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: hello: make --batch error gracefully

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:
> -    (call-process-region (point-min) (point-max) notmuch-command
> -			 t t nil "count" "--batch")
> +    (unless (= (call-process-region (point-min) (point-max) notmuch-command
> +				    t t nil "count" "--batch") 0)
> +      (notmuch-logged-error "notmuch CLI version mismatch error (count --batch)
> +The most likely cause of this error is that the CLI is too old
> +to support count --batch and needs to be upgraded to the same
> +version as notmuch-emacs"))
> +

I had a look at this, and I agree the current failure mode is not nice
(it just says something mysterious about nil not being a string). On the
other hand, I think we should use the two argument form of
notmuch-logged-error, and the the first argument should say only what we
know. Something like

(notmuch-logged-error "notmuch count --batch failed"
"notmuch count --batch failed
Please check that the notmuch CLI is new enough to  support `count
--batch'. In general we recommend running matching versions of the CLI
and emacs interface.")

d

Re: [PATCH] emacs: hello: make --batch error gracefully

[David Bremner]

David Bremner <david@tethera.net> writes:

> I had a look at this, and I agree the current failure mode is not nice
> (it just says something mysterious about nil not being a string). On the
> other hand, I think we should use the two argument form of
> notmuch-logged-error, and the the first argument should say only what we
> know. Something like
>
> (notmuch-logged-error "notmuch count --batch failed"
> "notmuch count --batch failed
> Please check that the notmuch CLI is new enough to  support `count
> --batch'. In general we recommend running matching versions of the CLI
> and emacs interface.")

I pushed an amended version along these lines; it turns out the first
argument does not need to be repeated.

d