Re: [PATCH] emacs: mua: add a pre-send-check-hook
> 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
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`?
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
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
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
> 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
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
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 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
> > 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
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
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 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
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 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
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
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