Re: [PATCH] emacs: mua: add a pre-send-check-hook

[Mark Walters]

> For consistency with Emacs' own elisp, perhaps rename
> notmuch-mua-pre-send-check-hooks to
> notmuch-mua-pre-send-check-functions?

Hi

Yes you are correct.  (Annoyingly I had thought about this and thought
it was OK since I didn't need to pass an argument. I had missed the
return value case).

Best wishes

Mark
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v4] emacs: add notmuch-address-post-completion-hook

[Mark Walters]

On Sat, 12 Nov 2016, David Bremner  wrote:
> Tomi Ollila  writes:
>
>> Like someone (whose message I cannot find just now) mentioned in another
>> thread, just now it is right time to mention here too...
>>
>> https://www.gnu.org/software/emacs/manual/html_node/elisp/Hooks.html
>>
>> ... that when hook name ends with `-hook` it is supposed to be "normal hook"
>> -- a function which does not take arguments nor return values.
>>
>> So, I'd like to suggest that this variable is renamed to 
>> notmuch-address-completion-functions
>>
>
> Well, I guess the -functions convention should be followed, but
> notmuch-address-completion-functions seems a bit vague.

Maybe notmuch-address-post-completion-functions ?

Alternatively maybe we can end in -hook-functions to indicate it is a
function which is like a hook?

Best wishes

Mark
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [Paul Wise] Bug#843127: notmuch: race condition in `notmuch new`?

[Austin Clements]

Quoth David Bremner on Nov 04 at  1:26 pm:
> 
> Paul Wise wrote:
> 
> > Last night I got this error from my `notmuch new --quiet` cron job. The
> > file that the error message complains about is now in the cur directory
> > of the maildir at the following path.
> >
> > /path/to/mail/cur/1478190211.H80553P18378.chianamo:2,
> >
> > I wonder if this some kind of race condition in `notmuch new` processing.
> > Perhaps it should be using inotify to find out about file movements?
> >
> > Unexpected error with file 
> > /path/to/mail/new/1478190211.H80553P18378.chianamo
> > add_file: Something went wrong trying to read or write a file
> > Error opening /path/to/mail/new/1478190211.H80553P18378.chianamo: No such 
> > file or directory
> > Note: A fatal error was encountered: Something went wrong trying to read or 
> > write a file
> 
> I agree it looks like a race condition. inotify sounds a bit
> overcomplicated and perhaps non-portable? It should probably just
> tolerate disappearing files better, consider that a warning.

Inotify really *is* the solution. This is a symptom of a much bigger
problem: scandir makes no guarantees in the presence of concurrent
directory modification. If you delete or rename a file while notmuch
new is running, it may think *completely unrelated* files in the same
directory were also deleted. Even if scandir were atomic, if you move
a mail from one directory to another between notmuch scanning the
destination directory and notmuch scanning the source directory, it'll
think the mail has been deleted and potentially remove it from the DB.

The "recommended" solution is to scandir is to start an inotify watch
before the scan and redo (or update) the scan if there are any
changes. For notmuch, it would make sense to extend that to watching
all directories to make sure it can catch renames during the scan.

A possible alternative, though I haven't worked out the details, might
be to keep a close eye on the directory mtimes. Roughly, for each
directory, check the mtime before scanning, wait if necessary until
the mtime != the current time, do the scan and process the files
optimistically. Once all directories are processed, re-check all of
the mtimes and if any have changed, do something like starting over
but hopefully more intelligent.

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v4] emacs: add notmuch-address-post-completion-hook

[David Bremner]

Tomi Ollila  writes:

> Like someone (whose message I cannot find just now) mentioned in another
> thread, just now it is right time to mention here too...
>
> https://www.gnu.org/software/emacs/manual/html_node/elisp/Hooks.html
>
> ... that when hook name ends with `-hook` it is supposed to be "normal hook"
> -- a function which does not take arguments nor return values.
>
> So, I'd like to suggest that this variable is renamed to 
> notmuch-address-completion-functions
>

Well, I guess the -functions convention should be followed, but
notmuch-address-completion-functions seems a bit vague.

d
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] cli: consider files vanishing during notmuch new non-fatal

[David Bremner]

Brian Sniffen  writes:

> That's hard, given dovecot pointed at the same maildir: it quickly
> moves files from new to cur. That makes notmuch insert pretty useless,
> and I rely on notmuch new to approach correctness.

I don't think this discussion is related to notmuch insert at all. If
you have found a race condition (or some other concurrency issue) in
notmuch-insert please report that seperately.

>
> But maybe I misunderstand: is the idea that it will return an error
>but keep processing?  Or stop on that error?

The whole discussion started because under certain circumstances it will
stop processing. The proposed patch makes it continue processing, but
report an error at the end.

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] cli: consider files vanishing during notmuch new non-fatal

[Brian Sniffen]

> On Nov 12, 2016, at 11:10 AM, David Bremner  wrote:
> 
> Brian Sniffen  writes:
> 
>>> 
>>> OK, but the patch proposed works both for people who want to be notified
>>> of this problem, and those that don't (with appropriate shell wrapping
>>> checking the return code).  
>> 
>> I think it will loop; how do I guarantee termination and indexing of all 
>> present messages if deletions cause errors?
> 
> stop deleting things? You can't guarantee termination and indexing of
> all present messages by ignoring deletions either.

That's hard, given dovecot pointed at the same maildir: it quickly moves files 
from new to cur. That makes notmuch insert pretty useless, and I rely on 
notmuch new to approach correctness. 

But maybe I misunderstand: is the idea that it will return an error but keep 
processing?  Or stop on that error?



___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] cli: consider files vanishing during notmuch new non-fatal

[Jani Nikula]

On Sat, 12 Nov 2016, Brian Sniffen  wrote:
>> 
>> OK, but the patch proposed works both for people who want to be notified
>> of this problem, and those that don't (with appropriate shell wrapping
>> checking the return code).  
>
> I think it will loop; how do I guarantee termination and indexing of
> all present messages if deletions cause errors?

Please note that we're talking about deletions and renames *between* the
scandir(3) call and going through the results it returns, during a
single invocation of 'notmuch new'. On the next run, scandir(3) won't
return the entry, and we'll think it's gone.

(Of course, if you keep deleting/renaming files and running 'notmuch
new' simultaneously all the time, you'll hit this on some other files on
the consequent runs, but then you asked for it...)

BR,
Jani.
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v4] emacs: add notmuch-address-post-completion-hook

[Tomi Ollila]

On Fri, Nov 04 2016, David Bremner  wrote:

> This hook can be used to update the message based on the results of
> address completion. For example using message-templ or gnus-alias to set
> the From address based on the To address just completed.
>
> The post-completion command is added to the notmuch-company backend to
> ensure that the hook is also called company completion is started
> without going through notmuch-address-expand-name. See the docstring of
> `company-backends' for more information.
> ---
>
> Updates from Mark's second review
>  emacs/notmuch-address.el | 14 +-
>  emacs/notmuch-company.el |  1 +
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
> index b2e1fba..36c796f 100644
> --- a/emacs/notmuch-address.el
> +++ b/emacs/notmuch-address.el
> @@ -98,6 +98,17 @@ to know how address selection is made by default."
>:group 'notmuch-send
>:group 'notmuch-external)
>  
> +(defcustom notmuch-address-completion-hook nil
> +  "Functions called after completing address.
> +
> +The completed address is passed as an argument to each function.
> +Note that this hook will be invoked for completion in headers
> +matching `notmuch-address-completion-headers-regexp'.
> +"
> +  :type 'hook
> +  :group 'notmuch-address
> +  :group 'notmuch-hooks)
> +
>  (defun notmuch-address-selection-function (prompt collection initial-input)
>"Call (`completing-read'
>PROMPT COLLECTION nil nil INITIAL-INPUT 'notmuch-address-history)"
> @@ -206,7 +217,8 @@ external commands."
> (progn
>   (push chosen notmuch-address-history)
>   (delete-region beg end)
> - (insert chosen))
> + (insert chosen)
> + (run-hook-with-args 'notmuch-address-completion-hook chosen))

Like someone (whose message I cannot find just now) mentioned in another
thread, just now it is right time to mention here too...

https://www.gnu.org/software/emacs/manual/html_node/elisp/Hooks.html

... that when hook name ends with `-hook` it is supposed to be "normal hook"
-- a function which does not take arguments nor return values.

So, I'd like to suggest that this variable is renamed to 
notmuch-address-completion-functions


Tomi


>   (message "No matches.")
>   (ding
> (t nil)))
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] cli: consider files vanishing during notmuch new non-fatal

[David Bremner]

David Bremner  writes:

> Brian Sniffen  writes:
>
>>> 
>>> OK, but the patch proposed works both for people who want to be notified
>>> of this problem, and those that don't (with appropriate shell wrapping
>>> checking the return code).  
>>
>> I think it will loop; how do I guarantee termination and indexing of all 
>> present messages if deletions cause errors?
>>
>> -Brian
>
> stop deleting things? You can't guarantee termination and indexing of
> all present messages by ignoring deletions either.
>
> d

Sorry, that was written in haste. Of course if that's your goal ignoring
deletions is ok, but renames will still get you, and we have no way of
knowing the difference.  In any case, I was more thinking that people
who want to ignore deletions could check for the specific error code and
consider that not-an-error.

d
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] cli: consider files vanishing during notmuch new non-fatal

[Brian Sniffen]

> 
> OK, but the patch proposed works both for people who want to be notified
> of this problem, and those that don't (with appropriate shell wrapping
> checking the return code).  

I think it will loop; how do I guarantee termination and indexing of all 
present messages if deletions cause errors?

-Brian
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] cli: consider files vanishing during notmuch new non-fatal

[David Bremner]

Brian Sniffen  writes:

>> 
>> OK, but the patch proposed works both for people who want to be notified
>> of this problem, and those that don't (with appropriate shell wrapping
>> checking the return code).  
>
> I think it will loop; how do I guarantee termination and indexing of all 
> present messages if deletions cause errors?
>
> -Brian

stop deleting things? You can't guarantee termination and indexing of
all present messages by ignoring deletions either.

d
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] cli: consider files vanishing during notmuch new non-fatal

[David Bremner]

Paul Wise  writes:

> On Sat, 2016-11-05 at 14:57 +0200, Jani Nikula wrote:
>
>> Add a new exit code for when files vanished, so the caller has a
>> chance to detect the race and re-run notmuch new to recover.
>
> I don't think this is the right approach for two reasons:
>
> The exit code you have chosen is still a failure so I will still get
> notified for a minor issue. I use chronic to detect fail scenarios.
>
> This is a pretty normal scenario when you have a mail program open and
> are auto-running `notmuch new` on a scheduled basis or when new mail
> arrives. notmuch should just ignore the error and continue as normal.
>

OK, but the patch proposed works both for people who want to be notified
of this problem, and those that don't (with appropriate shell wrapping
checking the return code).  That seems better than hiding it for
everyone. And certainly an improvement on the status quo. A possible
future enhancement would be a flag like notmuch insert has to control
the treatment of these errors.

d
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v5 4/4] emacs: resume messages

[David Bremner]

David Bremner  writes:

> Provide functionality to resume editing a mesage previously saved with
> notmuch-draft-save, including decoding the X-Notmuch-Emacs-Secure
> header.

s/mesage/message/

> +(defun notmuch-draft-unquote-some-mml ()
> +  "Unquote the mml tags in `notmuch-draft-quoted-tags`."
> +  (save-excursion
> +(when notmuch-draft-quoted-tags
> +  (let ((re (concat "<#!+/?\\("
> + (mapconcat 'identity notmuch-draft-quoted-tags "\\|")
> +
Same issue here with regex quoting, I think.

> +(let (secure-tag)
> +  (save-restriction
> + (message-narrow-to-headers)
> + (setq secure-tag (message-fetch-field "X-Notmuch-Emacs-Secure" 't))
> + (message-remove-header "X-Notmuch-Emacs-Secure"))
> +  (message-goto-body)
> +  (when secure-tag
> + (insert secure-tag "\n")

Can the setq inside the let be replaced with

(let ((secure-tag (message-fetch-field "X-Notmuch-Emacs-Secure" 't)))
 ...

Perhaps by pushing the let inside the save-restriction?

>  (require 'notmuch-mua)
>  (require 'notmuch-crypto)
>  (require 'notmuch-print)
> +(require 'notmuch-draft)

This line I added.

> +(defun notmuch-show-resume-message ()
> +  "Resume EDITING the current draft message."
> +  (interactive)
> +  (let ((id (notmuch-show-get-message-id)))
> +(when id
> +  (notmuch-draft-resume id
> +

The error handling is not very clear to me
here. notmuch-show-get-message-id is not documented to return nil on
error. Should some docstring be changed here?

> +<#secure method=pgpmime mode=sign>
> +EOF
> +test_expect_equal_file EXPECTED OUTPUT.clean
>  test_done

The quoting of the secure tag here is not present in the original test,
but sure confused me for a few minutes.
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v5 3/4] emacs: check drafts for encryption tags before saving

[David Bremner]

Mark Walters  writes:

>> @@ -109,6 +140,15 @@ This saves the current message in the database with tags
>>  `notmuch-draft-tags` (in addition to any default tags
>>  applied to newly inserted messages)."
>>(interactive)
>> +  (case notmuch-draft-save-plaintext
>> +((ask)
>> + (unless (notmuch-draft--check-encryption-tag t)
>> +   (error "Save aborted")))
>> +((t)
>> + (ignore))
>> +((nil)
>> + (unless (notmuch-draft--check-encryption-tag nil)
>> +   (error "Refusing to save draft with encryption tags (see 
>> `notmuch-draft-save-plaintext')"
>
> What would you think of rejigging the logic here? I would prefer that
> the first check was "is there an encryption tag" and then if there is
> such a tag decide what to do. The reason I prefer that is that it makes
> the common case clear.
>
> I realise there are downsides too -- eg in your code you don't even
> check for secure tags if they are going to  be ignored anyway.

OK, I was mainly concerned with not prompting the user uneccesarily, but
the following seems to be equivalent:


-  (case notmuch-draft-save-plaintext
-((ask)
- (unless (notmuch-draft--check-encryption-tag t)
-   (error "Save aborted")))
-((t)
- (ignore))
-((nil)
- (unless (notmuch-draft--check-encryption-tag nil)
+  (unless (notmuch-draft--check-encryption-tag
+  (eq notmuch-draft-save-plaintext 'ask))
+(case notmuch-draft-save-plaintext
+  ((ask)
+   (error "Save aborted"))
+  ((t)
+   (ignore))
+  ((nil)
(error "Refusing to save draft with encryption tags (see 
`notmuch-draft-save-plaintext')"
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v5 2/4] emacs: postpone a message

[David Bremner]

David Bremner  writes:

> From: Mark Walters 

This really Mark's work, that I have split out into a separate file.

> +(defcustom notmuch-draft-tags '("+draft")
> +  "List of tags changes to apply to a draft message when it is saved in the 
> database.

Here and a few other places the documentation uses "the database" to
mean the directory hierachy containing mail messages + the xapian
database.  At some point I would like to be able to distinguish between
the database and the maildir root (which doesn't need to be maildirs)
when talking about configuration. I don't really know better terminology
here, but I thought I would mention it in case someone else is inspired.

> +(defun notmuch-draft--mark-deleted ()

This -- naming convention is my contribution. Perhaps eventually we
could mark all private functions not intended to be called by users this
way. Since it is essentially cosmetic, I didn't want to do that
now. Indeed, one could quibble about the correctness of calling a
function private and then putting it in a public hook.

> +(defun notmuch-draft-quote-some-mml ()
> +  "Quote the mml tags in `notmuch-draft-quoted-tags`."
> +  (save-excursion
> +;; First we deal with any secure tag separately.
> +(message-goto-body)
> +(when (looking-at "<#secure[^\n]*>\n")
> +  (let ((secure-tag (match-string 0)))
> + (delete-region (match-beginning 0) (match-end 0))
> + (message-add-header (concat "X-Notmuch-Emacs-Secure: " secure-tag
> +;; This is copied from mml-quote-region but only quotes the
> +;; specified tags.
> +(when notmuch-draft-quoted-tags
> +  (let ((re (concat "<#!*/?\\("
> + (mapconcat 'identity notmuch-draft-quoted-tags "\\|")
> + "\\)")))
One "hidden feature" is that regex characters in the quoted tags will be
interpreted. Possibly calling regexp-quote instead of identity would be
extra cautious here?

> +(defun notmuch-draft-save ()
> +  "Save the current draft message in the notmuch database.
> +
> +This saves the current message in the database with tags
> +`notmuch-draft-tags` (in addition to any default tags
> +applied to newly inserted messages)."
> +  (interactive)
> +  (let (;; We need the message id as we need it for tagging. Note
> + ;; message-make-message-id gives the id inside a "<" ">" pair,
> + ;; but notmuch doesn't want that form, so remove them.
> + (id (concat "draft-" (substring (message-make-message-id) 1
> -1

what do you think of isolating this code and commentary in a private
function?

> + (if (member 'Message-ID message-deletable-headers)
> +  (progn
> +(message-remove-header "Message-ID")
> +(message-add-header (concat "Message-ID: <" id ">")))
> +   (message "You have customized emacs so Message-ID is not a deletable 
> header, so not changing it")
> +   (setq id nil))

I'm not sure if it's just me, but I find the (if (progn ...)
else-clauses) a bit off-putting. An alternative would be to use cond
 
(cond
 ((member 'Message-ID message-deletable-headers)
  (message-remove-header "Message-id")
  (message-add-header (concat "Message-ID: <" id ">")))
 (t
  (message "You have customized emacs so Message-ID is not a deletable header, 
so not changing it")
  (setq id nil)))


> +(add-hook 'message-send-hook 'notmuch-draft--mark-deleted)

Can we avoid this by adding some code notmuch-mua-send-common?

> --- /dev/null
> +++ b/test/T630-emacs-draft.sh
> @@ -0,0 +1,42 @@
> +#!/usr/bin/env bash
> +test_description="Emacs Draft Handling"
> +. ./test-lib.sh || exit 1

Ok, now I remember I wrote this part, so someone else should review.
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v2] emacs: add compatability functions for emacs 23

[David Bremner]

Tomi Ollila  writes:


> But this compatIbility change is not just emacs 23 -- iirc there were some
> changes required to get emacs 24.1, 24.2, and 24.3 to work. It might be
> easier to keep testing using emacs 23 until we deprecate everything before
> emacs 24.4 (released October 20, 2014) -- or supporting emacs23 gets
> nontrivial while supporting older emacs24 releases is still trivial (*)

This might refer to the defadvice on ido-completing-read in
notmuch-mua.el. In any case that could also go is a seperate
compatability file.

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3] completion: complete mimetype: search prefix

[David Bremner]

Jani Nikula  writes:

> Use /etc/mime.types if available, parsed using a sed one-liner, and
> fall back to a handful of common types otherwise.
>
> ---

pushed to master
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch